Re: [PATCH v7 18/18] fwd_rule: Fix static checkers warnings in fwd_rule_add()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4564 bytes --]

On Tue, May 05, 2026 at 12:13:41PM +0200, Stefano Brivio wrote:
> On Tue, 5 May 2026 16:22:43 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, May 05, 2026 at 01:11:42AM +0200, Stefano Brivio wrote:
> > > The new checks are actually sufficient but not enough for Coverity
> > > Scan. Now that fwd->sock_count and new->last are affected or supplied
> > > by clients, we need explicit (albeit redundant) checks on them.
> > > 
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> > 
> > I'm assuming this does squash the warnings, but I think it does so in
> > a somewhat confusing way.
> 
> You don't need to assume that, you could try yourself without this
> patch and you'll see exactly two warnings with a lot of details.

I'm getting better, but I'm by no means well yet.  Some emails were
within my capacity.  Sorting out the new license key and doing a
Coverity run, not so much.

> 
> > > ---
> > >  fwd_rule.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/fwd_rule.c b/fwd_rule.c
> > > index b55e4df..03e8e80 100644
> > > --- a/fwd_rule.c
> > > +++ b/fwd_rule.c
> > > @@ -271,13 +271,22 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
> > >  		warn("Too many rules (maximum %d)", ARRAY_SIZE(fwd->rules));
> > >  		return -ENOSPC;
> > >  	}
> > > +
> > >  	if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) {
> > >  		warn("Rules require too many listening sockets (maximum %d)",
> > >  		     ARRAY_SIZE(fwd->socks));
> > >  		return -ENOSPC;
> > >  	}
> > > +	/* Redundant, to make static checkers happy */
> > > +	if (fwd->sock_count > ARRAY_SIZE(fwd->socks))
> > > +		return -ENOSPC;  
> > 
> > So there's actually two conditions that this is kind of relevant to:
> > 
> >  1) (fwd->sock_count > ARRAY_SIZE(fwd->socks)) on entry
> > 
> > That means something is horribly wrong before we were even called.
> > So, I think that would be better as an assert().
> > 
> >   2) (fwd->sock_count + num) overflows
> > 
> > That's a closer-to-real concern.  I'm pretty sure we can't hit it for
> > real, because num is necessarily <= 65536, so as long as (1) is true
> > this can't overflow.  But that relies on the specific value of
> > ARRAY_SIZE(fwd->socks), so it's kind of fragile.
> > 
> > I think an explicit check for this is a good idea, but it should
> > actually check for this, not just side-effects of it, so:
> > 	if (fwd->sock_count + num <= fwd->sock_count) {
> > 		warn("Blah blah overflow");
> > 		return -EFAULT; /* or whatever */
> > 	}
> > 
> > >  	fwd->rulesocks[fwd->count] = &fwd->socks[fwd->sock_count];
> > > +
> > > +	/* Redundant ('num' checked above), but not for static checkers */
> > > +	if (new->last > ARRAY_SIZE(fwd->socks) + new->first)
> > > +		return -ENOSPC;  
> > 
> > This way of organising the check is very confusing to me.  I'm not
> > really sure what it's trying to catch.
> 
> Same as above.

I'm not sure which "above" you mean.

> > We've already checked that
> > last >= first, so using num is safer to deal with at this
> > point than ARRAY_SIZE() + first, which could in principle overflow
> > even if sock_count + num is perfectly ok.
> 
> Using 'num' won't work. It shouldn't overflow anyway because the
> addition happens in 'int'.

It shouldn't overflow, but proving that requires knowing that:
	a. sock_count is bounded by ARRAY_SIZE(socks)
	b. first, last and num are bounded by 2^16
	c. ARRAY_SIZE(socks) + 2^16 won't overflow (in unsigned int)

I'm not sure which part coverity is missing.  (c) at least requires
knowledge which is not found in immediately adjacent code.

Oh... and... I just remembered that ARRAY_SIZE() is int, not unsigned
(or size_t).  I thought about changing that at some point, but it
seemed to cause more trouble than it was worth.  Does keep tripping me
though, since it seems like it logically ought to be unsigned.  Signed
overflows are much nastier (UB) that unsigned overflows.

> I'll try to change the rest if I find some time but it doesn't really
> look that critical to me.
> 
> > >  	for (port = new->first; port <= new->last; port++)
> > >  		fwd->rulesocks[fwd->count][port - new->first] = -1;
> > >  
> > > -- 
> > > 2.43.0
> > >   
> > 
> 
> -- 
> Stefano
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]