On Fri, Feb 02, 2024 at 12:47:42PM +0100, Theo Buehler wrote:
> On Fri, Feb 02, 2024 at 12:42:59PM +0100, Claudio Jeker wrote:
> > This bug was found by the mrt regress test. The problem is that
> > an empty ASPATH has lenght 0 and ibuf_open(0) fails (currently this is the
> > case but I had this changed in my work tree so I did not notice that).
> 
> ok
> 
> I meant to point this out during review, but I think I forgot: do we
> want to overflow check this multiplication or (entirely for completeness)
> or do we not care since we will error after having done (probably a lot)
> of work?

I don't think an overflow of the size_t is possible. The length of the
attribute is limited to a uint16_t in rde_attr_parse() and so this can not
overflow. Btw. the same is true for mrt_extract_attr() which it the only
other code calling aspath_inflate.
 
> > Use a simple hack to allocate an extra byte so ibuf_open() succeeds.
> > With this the mrt regress is happy again. If we change the ibuf_open()
> > behaviour this can be reverted.
> 
> cool.
> 
> > 
> > -- 
> > :wq Claudio
> > 
> > Index: util.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> > diff -u -p -r1.80 util.c
> > --- util.c	30 Jan 2024 13:50:09 -0000	1.80
> > +++ util.c	2 Feb 2024 11:38:27 -0000
> > @@ -547,8 +547,12 @@ aspath_inflate(struct ibuf *in)
> >  	uint16_t	 short_as;
> >  	uint8_t		 seg_type, seg_len;
> >  
> > -	/* allocate enough space for the worst case */
> > -	if ((out = ibuf_open(ibuf_size(in) * 2)) == NULL)
> > +	/*
> > +	 * allocate enough space for the worst case,
> > +	 * add 1 byte for the empty ASPATH case since we can't
> > +	 * allocate an ibuf of 0 length.
> > +	 */
> > +	if ((out = ibuf_open(ibuf_size(in) * 2 + 1)) == NULL)
> >  		return (NULL);
> >  
> >  	/* then copy the aspath */
> > 
> 

-- 
:wq Claudio