From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: handle 0-sized objects better
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 9 Sep 2024 17:57:26 +0200

On Mon, Sep 09, 2024 at 05:42:16PM +0200, Theo Buehler wrote:
> On Mon, Sep 09, 2024 at 04:39:12PM +0200, Claudio Jeker wrote:
> > memcpy(foo, NULL, 0) is sadly big no no. Also malloc(0) is not protable
> > and with that also calloc(0, foo) or reallocarray(foo, 0, bar).
> > 
> > This tries to fix some (most) of those. I know that some of the ASPA bits
> > need further work. Some of that will be in the next diff.
> > 
> > In aspath_prepend() I'm not sure if it is better to test for
> > asp->data == NULL instead of the uglier asp->len > shift
> > Now shift has to be 0 if asp->len == 0 so one could also check for
> > asp->len != 0.
> 
> asp->data is an array, so it's never NULL. I like asp->len > shift
> better than asp->len != 0 since it doesn't force me to think..
> 
> > In the pt_fill case the code in nlri_get_vpn4 ensures that labellen is
> > non-zero so I decided to fatal there.
> 
> I'm ok with this diff. Minor comments inline.
> 
> > 
> > -- 
> > :wq Claudio
> > 
> > Index: rde_attr.c
> > ===================================================================
> 
> [...]
> 
> > @@ -396,7 +397,7 @@ aspath_put(struct aspath *aspath)
> >  u_char *
> >  aspath_deflate(u_char *data, uint16_t *len, int *flagnew)
> >  {
> > -	uint8_t	*seg, *nseg, *ndata;
> > +	uint8_t		*seg, *nseg, *ndata = NULL;
> >  	uint32_t	 as;
> >  	int		 i;
> >  	uint16_t	 seg_size, olen, nlen;
> > @@ -415,6 +416,9 @@ aspath_deflate(u_char *data, uint16_t *l
> >  			fatalx("%s: would overflow", __func__);
> >  	}
> >  
> > +	if (nlen == 0)
> > +		goto done;
> 
> It's fine (and not new), but I'm always uneasy when it's the caller's
> responsibility to initialize an output parameter.

I think aspath_deflate() is one of those APIs that are a bit too clever.
Should probably simplify this and not reuse len... It seems we will have
to live with this shit forever so I should probably clean it up.

 
> [...]
> 
> > Index: rde_sets.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/rde_sets.c,v
> > diff -u -p -r1.12 rde_sets.c
> > --- rde_sets.c	28 Jul 2022 13:11:51 -0000	1.12
> > +++ rde_sets.c	9 Sep 2024 13:18:49 -0000
> > @@ -149,6 +149,9 @@ set_free(struct set_table *set)
> >  int
> >  set_add(struct set_table *set, void *elms, size_t nelms)
> >  {
> > +	if (nelms == 0)		/* nothing todo */
> > +		return 0;
> 
> I think this check is currently not reachable.

I think so too but lets be explicit.
 
> > +
> >  	if (set->max < nelms || set->max - nelms < set->nmemb) {
> >  		uint32_t *s;
> >  		size_t new_size;
> > 
> 

-- 
:wq Claudio