From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Overflow hazard in pgbench
Date: Mon, 28 Jun 2021 10:13:11 +0200 (CEST)
Message-ID: <alpine.DEB.2.22.394.2106272024180.482873@pseudo> (raw)
In-Reply-To: <73927.1624815543@sss.pgh.pa.us>
References: <73927.1624815543@sss.pgh.pa.us>
Hello Tom,
> moonjelly just reported an interesting failure [1].
I noticed. I was planning to have a look at it, thanks for digging!
> It seems that with the latest bleeding-edge gcc, this code is
> misoptimized:
> 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,
Hmmm, so it is not possible to detect these with standard arithmetic as
done by this code. Note that the code was written in 2016, ISTM pre C99
Postgres. I'm unsure about what a C compiler could assume on the previous
standard wrt integer arithmetic.
> whereupon the second if-block is provably unreachable.
Indeed.
> The failure still represents a gcc bug, because we're using -fwrapv
> which should disable that assumption.
Ok, I'll report it.
I also see a good point with pgbench tests exercising edge cases.
> However, not all compilers have that switch, so it'd be better to code
> this in a spec-compliant way.
Ok.
> I suggest applying the attached in branches that have the required
> functions.
The latest gcc, recompiled yesterday, is still wrong, as shown by
moonjelly current status.
The proposed patch does fix the issue in pgbench TAP test. I'd suggest to
add unlikely() on all these conditions, as already done elsewhere. See
attached version.
I confirm that check-world passed with gcc head and its broken -fwrapv.
I also recompiled after removing manually -fwrapv: Make check, pgbench TAP
tests and check-world all passed. I'm not sure that edge case are well
enough tested in postgres to be sure that all is ok just from these runs
though.
--
Fabien.
Attachments:avoid-pgbench-overflow-hazard-2.patch (text/x-diff, 901B) downloaddiff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.cindex e61055b6b7..8d0cabdab2 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);
@@ -2459,12 +2460,13 @@ evalStandardFunc(CState *st,
return false;
/* check random range */
- if (imin > imax)+ if (unlikely(imin > imax))
{
pg_log_error("empty range given to random");
return false;
}
- else if (imax - imin < 0 || (imax - imin) + 1 < 0)+ else if (unlikely(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");
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: coelho@cri.ensmp.fr, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: Overflow hazard in pgbench
In-Reply-To: <alpine.DEB.2.22.394.2106272024180.482873@pseudo>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox