From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=DWDXXoma; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 5F1745A0265 for ; Tue, 05 May 2026 16:41:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1777992079; bh=v8SiZR/30IjIdv8WDyQXDCd6HyVvUr/p6KMTfmvn3V4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=DWDXXomaR75B8OInWxEE8DzG4bzvVtyTQnj9WMYRGDgdEcUasQJPwEtA+J5T5cc51 4BSshyyV/BiAg8K4fruBQXIfivBvcxkh7SKRdLmju8RX9Mo/hFnO4MIwpJ/OVnnZdc adKuWKu3mKMP3T59iKt6A3tGAALoddN2D9E+83P8vYAofGtFu3+3OQ4vfuTN7RPOAR d6aL5bJ/65e1DNe19vwC5qOB5Gihl3BXOvswRoULijwDPtaBOAEQj6H73D+uqmnCoG dhv+CHNH9m54UyK805aplUR8GLadQ0yKFUnlUYoQJvxJ575uMDIp3TuT31JWc4zxIv CTaRl8JS6mPjQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4g91RM6rtdz4wLR; Wed, 06 May 2026 00:41:19 +1000 (AEST) Date: Wed, 6 May 2026 00:41:15 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v7 18/18] fwd_rule: Fix static checkers warnings in fwd_rule_add() Message-ID: References: <20260504231142.1118652-1-sbrivio@redhat.com> <20260504231142.1118652-19-sbrivio@redhat.com> <20260505121340.3a548603@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="tisOhK8VCb9BSOIg" Content-Disposition: inline In-Reply-To: <20260505121340.3a548603@elisabeth> Message-ID-Hash: OW5K2WFLCI6FN6EWY4RYP2CTMPSEMNO6 X-Message-ID-Hash: OW5K2WFLCI6FN6EWY4RYP2CTMPSEMNO6 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Jon Maloy , Laurent Vivier X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --tisOhK8VCb9BSOIg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, May 05, 2026 at 12:13:41PM +0200, Stefano Brivio wrote: > On Tue, 5 May 2026 16:22:43 +1000 > David Gibson wrote: >=20 > > 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. > > >=20 > > > Signed-off-by: Stefano Brivio =20 > >=20 > > I'm assuming this does squash the warnings, but I think it does so in > > a somewhat confusing way. >=20 > 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. >=20 > > > --- > > > fwd_rule.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > >=20 > > > 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 s= truct 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; =20 > >=20 > > So there's actually two conditions that this is kind of relevant to: > >=20 > > 1) (fwd->sock_count > ARRAY_SIZE(fwd->socks)) on entry > >=20 > > That means something is horribly wrong before we were even called. > > So, I think that would be better as an assert(). > >=20 > > 2) (fwd->sock_count + num) overflows > >=20 > > That's a closer-to-real concern. I'm pretty sure we can't hit it for > > real, because num is necessarily <=3D 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. > >=20 > > 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 <=3D fwd->sock_count) { > > warn("Blah blah overflow"); > > return -EFAULT; /* or whatever */ > > } > >=20 > > > fwd->rulesocks[fwd->count] =3D &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; =20 > >=20 > > This way of organising the check is very confusing to me. I'm not > > really sure what it's trying to catch. >=20 > Same as above. I'm not sure which "above" you mean. > > We've already checked that > > last >=3D 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. >=20 > 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. >=20 > > > for (port =3D new->first; port <=3D new->last; port++) > > > fwd->rulesocks[fwd->count][port - new->first] =3D -1; > > > =20 > > > --=20 > > > 2.43.0 > > > =20 > >=20 >=20 > --=20 > Stefano >=20 --=20 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 --tisOhK8VCb9BSOIg Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmn6AYEACgkQzQJF27ox 2Gf5MA//em2fi+MYvcT30+skXsejTceuRX02JUuODZEGuU0ChYMlJUV/Yyt2/GwM PNv0hvfIKe4eQPqWMLfRdFZNSbCDMEj4s1cPy3UZrgdC35bALWxw5K2aa400+ntP 1+5Rb74AkiMUBMuILG6sf7W2/5UXOJJv3WemuDE4UgeuenyeO2NAxiL1gh1CRq3o 1wmA41mx25Jy9KYAZL4PSqJcQ1X/4r1EGW5XO7RR9Dga8A1/VKJ+VASdzI0pmpOR JsLSMzsRxPgxXgMs3b/iCSNE+KZWqcTtShW5rpp/CtsFfZcgCy4V0tI5k/i0E71q yLmBm6lJD6t4LjIU3RmSk1G+q/swIb7Nt16+tUVnhSYvTMFuIM0w/D+7N8K8vt5w 0UqP64DpA/8PfeEEaiisJdtjCH0qOQni/SZkBBFVaAE8odv7AesgffvdsJYtZ7R1 sFn03TJxOBzLRGH1aPQEMQyhsxMOh60bYW5hFEXcghacofSqadz35koZ71+oyWRL KpxY7DpJp4jXxbquHLw2Q7HunWfbF56I9L5sZ/w1DUyui4CWOLnxqLnz8TzfPH1Q 25t9l3begEQT6/3yYRGixYdw3AMJd443LkNM60hyhrkx5ocPAJGFigVHZFHnp2hB DeLqtA45FRzmTV5ruzqh1g1nXHtbtY3OUGuJdEdL4o5/vmhJcPY= =Ehe6 -----END PGP SIGNATURE----- --tisOhK8VCb9BSOIg--