On Wed, Feb 21, 2024 at 03:15:26PM +0100, Claudio Jeker wrote:
> 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).

I am fine with using -p to disable spl processing entirely. I asked to
remove the option as it was in the previous diff since it only disabled
JSON output, which made no sense to me (and we're starting to run out of
letters).

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

This is basically your aspa.c r1.11 code with an additional twist with
the cmp since we don't compare integers only. We should think about
improving this but for now I think it is best to keep them in sync as
best we can.

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

I'd say we should do this.

> Other option is to track the allocation size independent of the prefixesz.
> 
> -- 
> :wq Claudio
>