From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 322F15A004F for ; Fri, 28 Jun 2024 09:19:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1719559186; bh=7LLW8boK5rZ5BcQqSILU4A7eK3RHwAnHLo6sg+NH+lw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=QF7M1GYDxoy0zzB2SlORMZkgvGtRPa+fIN2KdWmQEZJXGYS2zcJPGfnABmxxSqTpv 6sB9/XZFFsEW5rI5rKhcPLs7PImSJI9c1ma4NcohI7w2UvnXfzUqvFq480UkGjpsvH 4t5LM/2gbiSYDkiDGKHswkWzodRBHNrdeAzfOJj/Vupyxi1mTaEReGzwYf/o6KODgm 5lhJj9WDHioZM8RddCPVtS17HwbLRNsElawgqeGfYwB056WqaKweHUzLRbQyh+7KVu gez5svPKIGwXVdKPm4RItTgcEDdTiQ3eK8wCua8FJeKh4mJrc61baGzPp2Bm0qEfUf H1Lx9REg/BrEw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W9Rdt40y0z4wb7; Fri, 28 Jun 2024 17:19:46 +1000 (AEST) Date: Fri, 28 Jun 2024 17:15:09 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions Message-ID: References: <20240626234536.3306466-1-sbrivio@redhat.com> <20240626234536.3306466-4-sbrivio@redhat.com> <20240627095537.4d3f20f8@elisabeth> <20240627224645.6cad668d@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="P6bXGBFPovP5aiyi" Content-Disposition: inline In-Reply-To: <20240627224645.6cad668d@elisabeth> Message-ID-Hash: EYG57EW4T3BUZPDJ2QOSDFUITVADD356 X-Message-ID-Hash: EYG57EW4T3BUZPDJ2QOSDFUITVADD356 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, Matej Hrica 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: --P6bXGBFPovP5aiyi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 27, 2024 at 10:46:45PM +0200, Stefano Brivio wrote: > On Thu, 27 Jun 2024 09:55:46 +0200 > Stefano Brivio wrote: >=20 > > On Thu, 27 Jun 2024 11:13:14 +1000 > > David Gibson wrote: > >=20 > > > On Thu, Jun 27, 2024 at 01:45:35AM +0200, Stefano Brivio wrote: =20 > > > > Potential sum and subtraction overflows were reported by Coverity i= n a > > > > few places where we use size_t and ssize_t types. > > > >=20 > > > > Strictly speaking, those are not false positives even if I don't se= e a > > > > way to trigger those: for instance, if we receive up to n bytes fro= m a > > > > socket up, the return value from recv() is already constrained and > > > > can't overflow for the usage we make in tap_handler_passt(). =20 > > >=20 > > > Actually, I think they are false positives. In a bunch of cases the > > > reasoning for that does rely on assuming the kernel will never return > > > a value greater than the buffer size for read(), write() or similar. = =20 > >=20 > > No, that was exactly my point: return values are constrained by the > > kernel, but a static checker doesn't necessarily have to assume a kernel > > that's properly functioning. > >=20 > > In general, static checkers do, especially if POSIX has a clear > > definition of a system call, and for what matters to us, they should. > >=20 > > But here Coverity is ignoring that, and I'm not sure we should call it > > a false positive. It's kind of arbitrary really. I think Coverity in > > these cases just prefers to "blindly" apply CERT C INT32-C locally, > > which is not necessarily a bad choice, because "false positives" are > > not so much of a nuisance. > >=20 > > > So possibly just ASSERT()ing that would suffice. =20 > >=20 > > In some cases yes, but as we have built-ins in gcc and Clang that aim > > at keeping the cost of the checks down by, quoting gcc documentation, > > using "hardware instructions to implement these built-in functions where > > possible", and they already implement the operation, open-coding our own > > checks for assertions looks redundant and might result in slower code. >=20 > I tried, but in most cases, not even open-coding the whole thing, as an > ASSERT() or an early return, say: >=20 > if ((rc > 0 && lr->count < LONG_MIN + rc) || > (rc < 0 && lr->count > LONG_MAX + rc)) > return -1; I'm not talking about open-coding a general overflow checking add/sub, I'm talking about checking the much tighter range we actually have. That should let Coverity reason that an overflow isn't possible, and also check for other possible weirdness. > or its ASSERT() equivalent is enough. That's because, without using > built-in functions, we can't have (of course) "infinite precision" > types. From: >=20 > https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html >=20 > These built-in functions promote the first two operands into infinite > precision signed type and perform addition on those promoted operands >=20 > And it looks like Coverity wants to see operations executed in those > types: size_t or unsigned long long is not enough. >=20 > In two cases, I could make Coverity happy with some rather bulky > asserts, if I turn operations into size_t plus ssize_t, in size_t > domain. But the result looks really bulky. --=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 --P6bXGBFPovP5aiyi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZ+Yv0ACgkQzQJF27ox 2GcXVw/+Jzxriu+RcRK9jdfJkGODjGnshP5RCLQJ2UR09we+3YXVstm3Xx+EAju5 zMeNHJEXWJEkijYVDi7tFqSnjpkcnjjWXgtVer/xuYzSF1BinNFcakH9LDJLoPRQ Se0bCEVQ+CiMz0/hhVr6mPO3i76baaNp5iBOitKeR3vj7VBnx7faxOvWhifpiMPx xey3DOuvSVDAjIheb9hTV6Toz8l2m2uRXt4Oy32HMgpsGccGhZg50NQaqQmIXzsp mpFLiLX2FsviCf09n9wCB1lEorSWjWU9Grs1eKeA4pb7MHuBD9Cr7qbhBh1HWnTu nB73D+eE58pZURRj2vwzZt4STQHbNjsLPiTPa8Zk5J4GrVTW+KT2ExUhIoXNrfK7 aZP0If3L6iyASk1MpkeQ8Hg2e5SeviuiDRin5DmHY8bWpmY/wiNwfeRfYVS1xhOu C51QC7bucK+zDhrkiIEhBTZOSVBu8aZhWtC+Q50/gM0/nHYI+IKPu2vWoUoaMvwz AULH9+Vsvk2Tl0kEEWi9tOSWujIhByIoDs0txDzvkugtUnDSROiOfYI/RS7Cjk/F jizX6f+3K8OQdM+lD0jkuI9g0kLZwIzdF/lxWqC9hDe5VWdzpiEm3x+QGWUDy9mH WaOhKJwjnZXJX+od5tBHxjG71faeqsnC33q7tb6fkROhmTkEjmA= =YRXv -----END PGP SIGNATURE----- --P6bXGBFPovP5aiyi--