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

On Wed, Feb 21, 2024 at 03:31:41PM +0100, Theo Buehler wrote:
> 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).

I'm happy to have something like -x to exclude / include experimental
bits. So we don't constantly add flags and remove them again. With ASPA we
got hit rather bad IIRC since we had to go back and forth a few times to
get it right.
 
> > 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.

Ha ha ha, I did not remember to write such a clever thing.
Current aspa.c uses reallocarray() so maybe that's an option as well.
 
> > 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
> > 
> 

-- 
:wq Claudio