From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
cc: Fabien COELHO <coelho@cri.ensmp.fr>
Subject: Overflow hazard in pgbench
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <73792.1624815355.0@sss.pgh.pa.us>
Date: Sun, 27 Jun 2021 13:39:03 -0400
Message-ID: <73927.1624815543@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/73927.1624815543%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <73792.1624815355.1@sss.pgh.pa.us>

moonjelly just reported an interesting failure [1].  It seems that
with the latest bleeding-edge gcc, this code is misoptimized:

                /* check random range */
                if (imin > imax)
                {
                    pg_log_error("empty range given to random");
                    return false;
                }
                else if (imax - imin < 0 || (imax - imin) + 1 < 0)
                {
                    /* prevent int overflows in random functions */
                    pg_log_error("random range is too large");
                    return false;
                }

such that the second if-test doesn't fire.  Now, according to the C99
spec this code is broken, because the compiler is allowed to assume
that signed integer overflow doesn't happen, whereupon the second
if-block is provably unreachable.  The failure still represents a gcc
bug, because we're using -fwrapv which should disable that assumption.
However, not all compilers have that switch, so it'd be better to code
this in a spec-compliant way.  I suggest applying the attached in
branches that have the required functions.

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=moonjelly&dt=2021-06-26%2007%3A03%3A17

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="avoid-pgbench-overflow-hazard-1.patch";
	charset="us-ascii"
Content-ID: <73792.1624815355.2@sss.pgh.pa.us>
Content-Description: avoid-pgbench-overflow-hazard-1.patch

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e61055b6b7..c4023bfa27 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2450,7 +2450,8 @@ evalStandardFunc(CState *st,
 		case PGBENCH_RANDOM_ZIPFIAN:
 			{
 				int64		imin,
-							imax;
+							imax,
+							delta;
 
 				Assert(nargs >= 2);
 
@@ -2464,7 +2465,8 @@ evalStandardFunc(CState *st,
 					pg_log_error("empty range given to random");
 					return false;
 				}
-				else if (imax - imin < 0 || (imax - imin) + 1 < 0)
+				else if (pg_sub_s64_overflow(imax, imin, &delta) ||
+						 pg_add_s64_overflow(delta, 1, &delta))
 				{
 					/* prevent int overflows in random functions */
 					pg_log_error("random range is too large");

------- =_aaaaaaaaaa0--