On Tue, 5 May 2026 16:22:43 +1000
David Gibson
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
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