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

[Stefano Brivio <sbrivio@redhat.com>]

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.

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

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

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