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.

-- 
:wq Claudio