On Fri, Aug 09, 2024 at 03:50:06PM +0200, Claudio Jeker wrote:
> On Thu, Aug 08, 2024 at 10:10:40AM +0200, Theo Buehler wrote:
> > On Wed, Aug 07, 2024 at 09:27:55PM +0200, Claudio Jeker wrote:
> > > Had this sitting in my tree for about 6 month now (maybe it has been a
> > > year, who knows...). This uses the sane RTR ASPA PDU format that
> > > draft-ietf-sidrops-8210bis-14 finally adopted.
> > > 
> > > I tested this against a hacked up stayrtr version but testing right now is
> > > a bit hard.
> > 
> > This reads fine to me. The order check job mentioned can be added as a
> > follow-up.
> > 
> > A nit and a question:
> > 
> > > -- 
> > > :wq Claudio
> > > 
> > > Index: rtr_proto.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
> > > diff -u -p -r1.35 rtr_proto.c
> > > --- rtr_proto.c	9 Apr 2024 12:09:20 -0000	1.35
> > > +++ rtr_proto.c	7 Aug 2024 19:19:58 -0000
> > > @@ -31,7 +31,14 @@
> > >  struct rtr_header {
> > >  	uint8_t		version;
> > >  	uint8_t		type;
> > > -	uint16_t	session_id; /* or error code */
> > > +	union {
> > > +		uint16_t	session_id;
> > > +		uint16_t	errcode;
> > > +		struct {
> > > +			uint8_t flags;
> > > +			uint8_t zero;
> > 
> > nit: the space between type and variable should probably be a tab
> > 
> > > +		};
> > > +	};
> > >  	uint32_t	length;
> > >  } __packed;
> > >  
> > > @@ -108,11 +115,8 @@ struct rtr_routerkey {
> > >  #define FLAG_AFI_MASK	FLAG_AFI_V6
> > >  struct rtr_aspa {
> > >  	struct rtr_header	hdr;
> > > -	uint8_t			flags;
> > > -	uint8_t			afi_flags;
> > > -	uint16_t		cnt;
> > >  	uint32_t		cas;
> > > -	/* array of spas with cnt elements follows */
> > > +	/* array of spas filling the rest of the packet */
> > >  } __packed;
> > >  
> > >  struct rtr_endofdata {
> > > @@ -737,16 +741,15 @@ static int
> > >  rtr_parse_aspa(struct rtr_session *rs, struct ibuf *pdu)
> > >  {
> > >  	struct rtr_aspa rtr_aspa;
> > > -	struct aspa_tree *aspatree;
> > >  	struct aspa_set *aspa, *a;
> > > -	uint16_t cnt, i;
> > > +	uint32_t cnt, i;
> > > +	uint8_t flags;
> > >  
> > >  	if (ibuf_get(pdu, &rtr_aspa, sizeof(rtr_aspa)) == -1)
> > >  		goto badlen;
> > >  
> > > -	cnt = ntohs(rtr_aspa.cnt);
> > > -	if (ibuf_size(pdu) != cnt * sizeof(uint32_t))
> > > -		goto badlen;
> > > +	flags = rtr_aspa.hdr.flags;
> > > +	cnt = ibuf_size(pdu) / sizeof(uint32_t);
> > 
> > should there not be a check that ibuf_size(pdu) % sizeof(uint32_t) == 0?
> 
> I added that check. I think we also need some ibuf_size() == 0 check for
> the other parsers to ensure there was no junk at the end.

Actually all of this is already done in rtr_parse_header().
Including the ibuf_size(pdu) % sizeof(uint32_t) == 0. So no need to do
that again.

-- 
:wq Claudio