From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: handle 0-sized objects better
To: tech@openbsd.org
Date: Mon, 9 Sep 2024 17:42:16 +0200

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.

[...]

> 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.

> +
>  	if (set->max < nelms || set->max - nelms < set->nmemb) {
>  		uint32_t *s;
>  		size_t new_size;
>