On Tue, Jan 23, 2024 at 04:08:50PM +0100, Theo Buehler wrote:
> On Mon, Jan 22, 2024 at 03:15:38PM +0100, Claudio Jeker wrote:
> > I made all those ibuf and imsg API changes to improve the parser in bgpd.
> > Mainly rde_update_dispatch() and all the function it calls. While the code
> > is careful some errors snuck in (e.g. because of type overflows).
> > 
> > This should all be prevented by using the ibuf API since the handling
> > becomes simpler.
> > 
> > Now this is a deep rabbit hole and the overall bgpd change I have is over
> > 5000 lines long. To make review somewhat simpler I tried to split the diff
> > into chunks by introducing some hacks here and there to make progress
> > without rewriting everything.
> > 
> > This diff just rewrite rde_update_dispatch() to use ibufs. Because of this
> > rde_update_err(), rde_get_mp_nexthop(), nlri_get_prefix, and friends are
> > switched to use ibufs. For rde_attr_parse() an absolute minimum change was
> > done.
> > 
> > The problem is that bgpctl() uses nlri_get_prefix() itself and so some
> > code in there had to be adjusted. I kept the changes to a minimum for now
> > and sprinkled some XXX in places where I know I sinned. Those will be
> > cleaned up in the near future by pulling in more pending changes.
> 
> I'm fine with these XXX except that I don't really like the one in
> mrt_extract_prefix(). The other comments inline are purely cosmetic.
> 
> ok tb
> 
> > + trunc:
> > +		printf("truncated message");
> >  		return;
> 
> Maybe this could be moved to the end of the function like you did
> elsewhere

Fixed.
 
> > @@ -1032,23 +1032,25 @@ mrt_extract_addr(void *msg, u_int len, s
> >  }
> >  
> >  int
> > -mrt_extract_prefix(void *msg, u_int len, uint8_t aid,
> > +mrt_extract_prefix(void *m, u_int len, uint8_t aid,
> >      struct bgpd_addr *prefix, uint8_t *prefixlen, int verbose)
> >  {
> > +	struct ibuf buf, *msg = &buf;
> >  	int r;
> >  
> > +	ibuf_from_buffer(msg, m, len); /* XXX */
> >  	switch (aid) {
> >  	case AID_INET:
> > -		r = nlri_get_prefix(msg, len, prefix, prefixlen);
> > +		r = nlri_get_prefix(msg, prefix, prefixlen);
> >  		break;
> >  	case AID_INET6:
> > -		r = nlri_get_prefix6(msg, len, prefix, prefixlen);
> > +		r = nlri_get_prefix6(msg, prefix, prefixlen);
> >  		break;
> >  	case AID_VPN_IPv4:
> > -		r = nlri_get_vpn4(msg, len, prefix, prefixlen, 0);
> > +		r = nlri_get_vpn4(msg, prefix, prefixlen, 0);
> >  		break;
> >  	case AID_VPN_IPv6:
> > -		r = nlri_get_vpn6(msg, len, prefix, prefixlen, 0);
> > +		r = nlri_get_vpn6(msg, prefix, prefixlen, 0);
> >  		break;
> >  	default:
> >  		if (verbose)
> > @@ -1057,6 +1059,7 @@ mrt_extract_prefix(void *msg, u_int len,
> >  	}
> >  	if (r == -1 && verbose)
> >  		printf("failed to parse prefix of AID %d\n", aid);
> > +	r = len - ibuf_size(msg); /* XXX */
> 
> Should this really be done unconditionally and hide failures of
> nlri_get_*? I would have expected something equivalent to
> 
> 	if (r != -1)
> 		r = len - ibuf_size(msg); /* XXX */
> 
> Even if this will be cleaned up in the next few days, I think this
> would make more sense.
 
Indeed, thanks for catching this. In my full tree the function just
returns 0 and -1. So I had to hack this in after the fact to make the mrt
regress work.

> >  			case AID_VPN_IPv4:
> > -				pos = nlri_get_vpn4(data, alen, &prefix,
> > -				     &prefixlen, 1);
> > +				if (nlri_get_vpn4(buf, &prefix,
> > +				    &prefixlen, 1) == -1)
> > +					goto bad_len;
> >  				 break;
> 
> kill the ' ' before the 'b' in break
> 
> >  			case AID_VPN_IPv6:
> > -				 pos = nlri_get_vpn6(data, alen, &prefix,
> > -				     &prefixlen, 1);
> > +				if (nlri_get_vpn6(buf, &prefix,
> > +				    &prefixlen, 1) == -1)
> > +					goto bad_len;
> >  				 break;
> 
> same here

Both fixed.
 
> > +	size_t		 attr_len, hlen, plen;
> > +	uint8_t		 flags, type;
> > +
> > +        ibuf_from_ibuf(&attrbuf, buf);
> 
> spaces -> tab

Fixed.
 
Thanks for the review.
-- 
:wq Claudio