From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: fix chash ch_table_resize error handling
To: tech@openbsd.org
Date: Tue, 12 May 2026 18:02:35 +0200

On Tue, May 12, 2026 at 05:54:28PM +0200, Claudio Jeker wrote:
> On Tue, May 12, 2026 at 05:02:51PM +0200, Theo Buehler wrote:
> > On Tue, May 12, 2026 at 04:56:30PM +0200, Theo Buehler wrote:
> > > On Tue, May 12, 2026 at 11:46:03AM +0200, Claudio Jeker wrote:
> > > > Doing a double reallocarray call is tricky.
> > > > If the first reallocarray succeeds and the 2nd one fails then the pointer
> > > > for the first reallocarray needs to be updated and not freed.
> > > > 
> > > > Simply update the t->ch_tables pointer to the new location and return an
> > > > error here. That way the ch_table remains consisten and can still be used.
> > > 
> > > I knew something smelt off here...
> > > 
> > > ok tb
> > 
> > Wait. Doesn't that cause the t->ch_level++ to get out of sync?
> 
> I want to keep the old level, since we did not resize in the end.

Definitely. The next realloc of tables will simply be a noop.

> Now there is indeed an issue if the allocation failed on an empty table.
> This comes from that fact that ch_table_resize is abused to create a new
> table.

Yes, that's the the edge case I was trying to point out.

> Can I put this in now and the rethink the extendible hash bits and
> ch_table_resize()?

Sure.

I wouldn't mind if we did this in a less smart way and ended up with
code that does:

	tables = reallocarray(t->ch_tables, ...)
	if (tables == NULL)
		return -1;
	t->ch_tables = tables;

	metas = reallocarray(t->ch_metas, ...)
	if (metas == NULL)
		return -1;
	t->ch_metas = metas;

	if (first)
		t->ch_level++;

>  
> > > 
> > > > 
> > > > -- 
> > > > :wq Claudio
> > > > 
> > > > Index: chash.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/usr.sbin/bgpd/chash.c,v
> > > > diff -u -p -r1.10 chash.c
> > > > --- chash.c	7 May 2026 09:22:10 -0000	1.10
> > > > +++ chash.c	12 May 2026 09:32:07 -0000
> > > > @@ -548,7 +548,13 @@ ch_table_resize(const struct ch_type *ty
> > > >  		return -1;
> > > >  	metas = reallocarray(t->ch_metas, newsize, sizeof(*metas));
> > > >  	if (metas == NULL) {
> > > > -		free(tables);
> > > > +		/*
> > > > +		 * tables was correctly reallocated, so update that
> > > > +		 * pointer before failing hard. If the caller recovers
> > > > +		 * somehow the next reallocarray of ch_tables will simply
> > > > +		 * do nothing.
> > > > +		 */
> > > > +		t->ch_tables = tables;
> > > >  		return -1;
> > > >  	}
> > > >  
> > > > 
> > > 
> > 
> 
> -- 
> :wq Claudio