>> /* check the negative equivalent will fit without
overflowing */
>> if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
>> goto out_of_range;
>> +
>> + /*
>> + * special case the minimum integer because its negation
cannot be
>> + * represented
>> + */
>> + if (tmp == ((uint16) PG_INT16_MAX) + 1)
>> + return PG_INT16_MIN;
>> return -((int16) tmp);
>
> My first impression is that there appears to be two overflow checks, one
of
> which sends us to out_of_range, and another that just returns a special
> result. Why shouldn't we add a pg_neg_s16_overflow() and replace this
> whole chunk with something like this?
>
> if (unlikely(pg_neg_s16_overflow(tmp, &tmp)))
> goto out_of_range;
> else
> return tmp;
tmp is an uint16 here, it seems like you might have read it as an
int16? We would need some helper method like
static inline bool
pg_neg_u16_overflow(uint16 a, int16 *result);
and then we could replace that whole chunk with something like
if (unlikely(pg_neg_u16_overflow(tmp, &result)))
goto out_of_range;
else
return result;
that pattern shows up a lot in this file, but I was worried that it
wasn't useful as a general purpose function. Happy to add it
though if you still feel otherwise.
>> + return ((uint32) INT32_MAX) + 1;
>>
>> + return ((uint64) INT64_MAX) + 1;
>
> nitpick: Any reason not to use PG_INT32_MAX/PG_INT64_MAX for these?
Carelessness, sorry about that, it's been fixed in the attached patch.
>> I believe this is a copy-and-paste from 841b4a2d5, which added this:
>>
>> + *result = (date * INT64CONST(86400000000)) + time;
>> + /* check for major overflow */
>> + if ((*result - time) / INT64CONST(86400000000) != date)
>> + return -1;
>> + /* check for just-barely overflow (okay except time-of-day wraps) */
>> + if ((*result < 0) ? (date >= 0) : (date < 0))
>> + return -1;
>>
>> I think you could replace the whole thing by using overflow-aware
>> multiplication and addition primitives in the result calculation.
>> Lines 2-4 basically check for mult overflow and 5-7 for addition
>> overflow.
>
> Ah, I see. Joe's patch does that in one place. It's probably worth doing
> that in the other places this "just-barefly overflow" comment appears
IMHO.
>
> I was still confused by the comment about 1999, but I tracked it down to
>commit 542eeba [0]. IIUC it literally means that we need special handling
>for that date because POSTGRES_EPOCH_JDATE is 2000-01-01.
>
> [0]
https://postgr.es/m/CABUevEx5zUO%3DKRUg06a9qnQ_e9EvTKscL6HxAM_L3xj71R7AQw%40mail.gmail.com
> Yeah, I think so, and I think we probably don't need any special care
> if we switch to direct tests of overflow-aware primitives. (Though
>it'd be worth checking that '1999-12-31 24:00:00'::timestamp still
> works. It doesn't look like I actually added a test case for that.)
The only other place I found this comment was in
`make_timestamp_internal`. I've updated that function and added some
tests. I also manually verified that the behavior matches before and
after this patch.
>> BTW, while I approve of trying to get rid of our need for -fwrapv,
>> I'm quite scared of actually doing it.
>
> I think that's a quite fair concern. One potentially relevant datapoint is
> that we actually don't have -fwrapv equivalent on all platforms, and I
don't
>recall a lot of complaints from windows users. Of course it's quite
possible
> that they'd never notice...
>
> I think this is a good argument for enabling -ftrapv in development
> builds. That gives us at least a *chance* of seeing these issues.
+1, I wouldn't recommend removing -fwrapv immediately after this
commit. However, if we can enable -ftrapv in development builds, then
we can find overflows much more easily.
> Whatever cases you may have discovered by running the regression tests
will
> be at best the tip of the iceberg.
Agreed.
> Is there any chance of using static
> analysis to find all the places of concern?
I'm not personally familiar with any static analysis tools, but I can
try and do some research. Andres had previously suggested SQLSmith. I
think any kind of fuzz testing with -ftrapv enabled will reveal a lot
of issues. Honestly just grepping for +,-,* in certain directories
(like backend/utils/adt) would probably be fairly fruitful for anyone
with the patience. My previous overflow patch was the result of looking
through all the arithmetic in datetime.c.
Thanks,
Joe Koshakow
From 4fe3a3aaafc1b38351ee7ad952ad6b4a063523d5 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <kosh...@gmail.com>
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH] Remove dependence on integer wrapping
This commit updates various parts of the code to no longer rely on
integer wrapping for correctness. Not all compilers support -fwrapv, so
it's best not to rely on it.
---
src/backend/utils/adt/cash.c | 7 +++-
src/backend/utils/adt/numeric.c | 5 +--
src/backend/utils/adt/numutils.c | 35 +++++++++++++++++
src/backend/utils/adt/timestamp.c | 28 ++-----------
src/include/common/int.h | 48 +++++++++++++++++++++++
src/test/regress/expected/timestamp.out | 13 ++++++
src/test/regress/expected/timestamptz.out | 13 ++++++
src/test/regress/sql/timestamp.sql | 4 ++
src/test/regress/sql/timestamptz.sql | 4 ++
9 files changed, 128 insertions(+), 29 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..f6f095a57b 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -352,8 +352,11 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
+ /*
+ * make the amount positive for digit-reconstruction loop, we can
+ * leave INT64_MIN unchanged
+ */
+ pg_neg_s64_overflow(value, &value);
/* set up formatting data */
signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
sign_posn = lconvert->n_sign_posn;
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 5510a203b0..4ea2d9b0b4 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8110,15 +8110,14 @@ int64_to_numericvar(int64 val, NumericVar *var)
/* int64 can require at most 19 decimal digits; add one for safety */
alloc_var(var, 20 / DEC_DIGITS);
+ uval = pg_abs_s64(val);
if (val < 0)
{
var->sign = NUMERIC_NEG;
- uval = -val;
}
else
{
var->sign = NUMERIC_POS;
- uval = val;
}
var->dscale = 0;
if (val == 0)
@@ -11222,7 +11221,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
* Now we can proceed with the multiplications.
*/
neg = (exp < 0);
- mask = abs(exp);
+ mask = pg_abs_s32(exp);
init_var(&base_prod);
set_var_from_var(base, &base_prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..12bef9d63c 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -193,6 +193,13 @@ pg_strtoint16_safe(const char *s, Node *escontext)
/* check the negative equivalent will fit without overflowing */
if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint16) PG_INT16_MAX) + 1)
+ return PG_INT16_MIN;
return -((int16) tmp);
}
@@ -336,6 +343,13 @@ slow:
/* check the negative equivalent will fit without overflowing */
if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint16) PG_INT16_MAX) + 1)
+ return PG_INT16_MIN;
return -((int16) tmp);
}
@@ -598,6 +612,13 @@ slow:
/* check the negative equivalent will fit without overflowing */
if (tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1)
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint32) PG_INT32_MAX) + 1)
+ return PG_INT32_MIN;
return -((int32) tmp);
}
@@ -717,6 +738,13 @@ pg_strtoint64_safe(const char *s, Node *escontext)
/* check the negative equivalent will fit without overflowing */
if (unlikely(tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1))
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint64) PG_INT64_MAX) + 1)
+ return PG_INT64_MIN;
return -((int64) tmp);
}
@@ -860,6 +888,13 @@ slow:
/* check the negative equivalent will fit without overflowing */
if (tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1)
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint64) PG_INT64_MAX) + 1)
+ return PG_INT64_MIN;
return -((int64) tmp);
}
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index e4715605a2..c419c46540 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -617,19 +617,8 @@ make_timestamp_internal(int year, int month, int day,
time = (((hour * MINS_PER_HOUR + min) * SECS_PER_MINUTE)
* USECS_PER_SEC) + (int64) rint(sec * USECS_PER_SEC);
- result = date * USECS_PER_DAY + time;
- /* check for major overflow */
- if ((result - time) / USECS_PER_DAY != date)
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range: %d-%02d-%02d %d:%02d:%02g",
- year, month, day,
- hour, min, sec)));
-
- /* check for just-barely overflow (okay except time-of-day wraps) */
- /* caution: we want to allow 1999-12-31 24:00:00 */
- if ((result < 0 && date > 0) ||
- (result > 0 && date < -1))
+ if (pg_mul_s64_overflow(date, USECS_PER_DAY, &result) ||
+ pg_add_s64_overflow(result, time, &result))
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("timestamp out of range: %d-%02d-%02d %d:%02d:%02g",
@@ -2009,17 +1998,8 @@ tm2timestamp(struct pg_tm *tm, fsec_t fsec, int *tzp, Timestamp *result)
date = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - POSTGRES_EPOCH_JDATE;
time = time2t(tm->tm_hour, tm->tm_min, tm->tm_sec, fsec);
- *result = date * USECS_PER_DAY + time;
- /* check for major overflow */
- if ((*result - time) / USECS_PER_DAY != date)
- {
- *result = 0; /* keep compiler quiet */
- return -1;
- }
- /* check for just-barely overflow (okay except time-of-day wraps) */
- /* caution: we want to allow 1999-12-31 24:00:00 */
- if ((*result < 0 && date > 0) ||
- (*result > 0 && date < -1))
+ if (pg_mul_s64_overflow(date, USECS_PER_DAY, result) ||
+ pg_add_s64_overflow(*result, time, result))
{
*result = 0; /* keep compiler quiet */
return -1;
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 7fc046e78a..5258bd9476 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -154,6 +154,23 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
#endif
}
+static inline uint32
+pg_abs_s32(int32 a)
+{
+ if (unlikely(a == PG_INT32_MIN))
+ {
+ return ((uint32) PG_INT32_MAX) + 1;
+ }
+ else if (a < 0)
+ {
+ return (uint32) -a;
+ }
+ else
+ {
+ return (uint32) a;
+ }
+}
+
/*
* INT64
*/
@@ -258,6 +275,37 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
#endif
}
+static inline bool
+pg_neg_s64_overflow(int64 a, int64 *result)
+{
+ if (unlikely(a == PG_INT64_MIN))
+ {
+ return true;
+ }
+ else
+ {
+ *result = -a;
+ return false;
+ }
+}
+
+static inline uint64
+pg_abs_s64(int64 a)
+{
+ if (unlikely(a == PG_INT64_MIN))
+ {
+ return ((uint64) PG_INT64_MAX) + 1;
+ }
+ else if (a < 0)
+ {
+ return (uint64) -a;
+ }
+ else
+ {
+ return (uint64) a;
+ }
+}
+
/*------------------------------------------------------------------------
* Overflow routines for unsigned integers
*------------------------------------------------------------------------
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index cf337da517..e287260051 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -2201,3 +2201,16 @@ select age(timestamp '-infinity', timestamp 'infinity');
select age(timestamp '-infinity', timestamp '-infinity');
ERROR: interval out of range
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamp '1999-12-31 24:00:00';
+ timestamp
+--------------------------
+ Sat Jan 01 00:00:00 2000
+(1 row)
+
+select make_timestamp(1999, 12, 31, 24, 0, 0);
+ make_timestamp
+--------------------------
+ Sat Jan 01 00:00:00 2000
+(1 row)
+
diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out
index bfb3825ff6..d01d174983 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -3286,3 +3286,16 @@ SELECT age(timestamptz '-infinity', timestamptz 'infinity');
SELECT age(timestamptz '-infinity', timestamptz '-infinity');
ERROR: interval out of range
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamptz '1999-12-31 24:00:00';
+ timestamptz
+------------------------------
+ Sat Jan 01 00:00:00 2000 PST
+(1 row)
+
+select make_timestamptz(1999, 12, 31, 24, 0, 0);
+ make_timestamptz
+------------------------------
+ Sat Jan 01 00:00:00 2000 PST
+(1 row)
+
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index 820ef7752a..748469576d 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -424,3 +424,7 @@ select age(timestamp 'infinity', timestamp 'infinity');
select age(timestamp 'infinity', timestamp '-infinity');
select age(timestamp '-infinity', timestamp 'infinity');
select age(timestamp '-infinity', timestamp '-infinity');
+
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamp '1999-12-31 24:00:00';
+select make_timestamp(1999, 12, 31, 24, 0, 0);
diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql
index ccfd90d646..c71d5489b4 100644
--- a/src/test/regress/sql/timestamptz.sql
+++ b/src/test/regress/sql/timestamptz.sql
@@ -668,3 +668,7 @@ SELECT age(timestamptz 'infinity', timestamptz 'infinity');
SELECT age(timestamptz 'infinity', timestamptz '-infinity');
SELECT age(timestamptz '-infinity', timestamptz 'infinity');
SELECT age(timestamptz '-infinity', timestamptz '-infinity');
+
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamptz '1999-12-31 24:00:00';
+select make_timestamptz(1999, 12, 31, 24, 0, 0);
--
2.34.1
