From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: add support for draft-ietf-sidrops-rpki-prefixlist
To: Job Snijders <job@openbsd.org>
Cc: tech@openbsd.org
Date: Wed, 21 Feb 2024 15:15:26 +0100

On Wed, Feb 21, 2024 at 01:29:31PM +0000, Job Snijders wrote:
> I went through a round of review with tb@, resulting in the below
> changeset. Most notable change was to remove qsort().
> 
> Claudio?

It seems you added a new getopt letter but the code is not used.
I assume the idea is to provide something to enable / disable this new
file format (we had some bad issues in previous releases where a format
change caused aborts in rpki-client).

Also the stats are not updated neither in output_ometric.c nor in the
corresponding block in main.c. One could argue that outputheader() only
needs to be adjusted when the other config files (like openbgpd) grow
support for this.
 
> +static void
> +insert_vsp(struct vsp *vsp, size_t idx, struct spl_pfx *pfx)
> +{
> +	if (idx < vsp->prefixesz)
> +		memmove(vsp->prefixes + idx + 1, vsp->prefixes + idx,
> +		    (vsp->prefixesz - idx) * sizeof(*vsp->prefixes));
> +	vsp->prefixes[idx] = *pfx;
> +	vsp->prefixesz++;
> +}
> +

> +	/*
> +	 * Merge all data from the new SPL at hand into 'vsp': loop over
> +	 * all SPL->pfxs, and insert them in the right place in
> +	 * vsp->prefixes while keeping the order of the array.
> +	 */
> +	for (i = 0, j = 0; i < spl->pfxsz; ) {
> +		cmp = -1;
> +		if (j == vsp->prefixesz ||
> +		    (cmp = spl_pfx_cmp(&spl->pfxs[i], &vsp->prefixes[j])) < 0) {
> +			insert_vsp(vsp, j, &spl->pfxs[i]);
> +			i++;
> +		} else if (cmp == 0)
> +			i++;
> +
> +		if (j < vsp->prefixesz)
> +			j++;
> +	}

This is some very clever code. Especially that insert_vsp alters
vsp->prefixesz took me a bit by surprise.

Now there is one problem with this:

> +       /* merge content of multiple SPLs */
> +       vsp->prefixes = recallocarray(vsp->prefixes, vsp->prefixesz,
> +           vsp->prefixesz + spl->pfxsz, sizeof(struct spl_pfx));
> +       if (vsp->prefixes == NULL)
> +               err(1, NULL);

On first round this is fine. vsp->prefixes is NULL and this is a calloc.
On round two this is fine. vsp->prefixesz covers the full allocation but
on round three this becomes a problem. vsp->prefixesz can be smaller than
the allocation done before because of duplicates.

The manpage has the following about recallocarray():
     If ptr is not NULL, oldnmemb must be a value such that
     oldnmemb * size is the size of the earlier allocation
     that returned ptr, otherwise the behavior is undefined.

So right now that code above hits the undefined behavior case.
Since vsp->prefixesz can be smaller than the earlier allocation.

Maybe it is enough to call reallocarray(vsp->prefixes, vsp->prefixesz,
sizeof(struct spl_pfx)) after the merge to adjust the allocation size.

Other option is to track the allocation size independent of the prefixesz.

-- 
:wq Claudio