Hi,
In [0] Andres suggested enabling -ftrapv in assert enabled builds. While
I vastly underestimated the complexity of updating `configure` to do
this, I was able to enable the flag locally. Enabling this flag causes
our existing regression tests to trap and fail in multiple different
spots. The attached patch resolves all of these overflows so that all
of our existing tests will pass with the -ftrapv flag enabled.
Some notes on the patch itself are:
I originally added the helper functions to int.h thinking I'd find
many more instances of overflow due to integer negation, however I
didn't find that many. So let me know if you think we'd be better
off without the functions.
I considered using #ifdef to rely on wrapping when -fwrapv was
enabled. This would save us some unnecessary branching when we could
rely on wrapping behavior, but it would mean that we could only enable
-ftrapv when -fwrapv was disabled, greatly reducing its utility.
The following comment was in the code for parsing timestamps:
/* check for just-barely overflow (okay except time-of-day wraps) */
/* caution: we want to allow 1999-12-31 24:00:00 */
I wasn't able to fully understand it even after staring at it for
a while. Is the comment suggesting that it is ok for the months field,
for example, to wrap around? That doesn't sound right to me I tested
the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same
before and after the patch.
Thanks,
Joe Koshakow
[0]
https://www.postgresql.org/message-id/20240213191401.jjhsic7et4tiahjs%40awork3.anarazel.de
From 319bc904858ad8fbcc687a923733defd3358c7b9 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 | 13 ++-------
src/include/common/int.h | 48 +++++++++++++++++++++++++++++++
5 files changed, 92 insertions(+), 16 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..36a7f523d8 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2009,17 +2009,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..8fb3d4e069 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) 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) INT64_MAX) + 1;
+ }
+ else if (a < 0)
+ {
+ return (uint64) -a;
+ }
+ else
+ {
+ return (uint64) a;
+ }
+}
+
/*------------------------------------------------------------------------
* Overflow routines for unsigned integers
*------------------------------------------------------------------------
--
2.34.1
