Re: 64-bit integer subtraction bug on some platforms

[Tom Lane]

Laurenz Albe  writes:
> On Wed, 2023-11-08 at 11:58 +, Dean Rasheed wrote:
>> This should overflow, since the correct result (+9223372036854775808)
>> is out of range. However, on platforms without integer overflow
>> builtins or 128-bit integers, pg_sub_s64_overflow() does the
>> following:
>> ...
>> which fails to spot the fact that overflow is also possible when a ==
>> 0. So on such platforms, it returns the wrong result.
>> 
>> Patch attached.

> The patch looks good to me.

+1: good catch, fix looks correct.

regards, tom lane

Re: 64-bit integer subtraction bug on some platforms

[Laurenz Albe]

On Wed, 2023-11-08 at 11:58 +, Dean Rasheed wrote:
> One of the new tests in the infinite interval patch has revealed a bug
> in our 64-bit integer subtraction code. Consider the following:
> 
> select 0::int8 - '-9223372036854775808'::int8;
> 
> This should overflow, since the correct result (+9223372036854775808)
> is out of range. However, on platforms without integer overflow
> builtins or 128-bit integers, pg_sub_s64_overflow() does the
> following:
> 
> if ((a < 0 && b > 0 && a < PG_INT64_MIN + b) ||
> (a > 0 && b < 0 && a > PG_INT64_MAX + b))
> {
> *result = 0x5EED;/* to avoid spurious warnings */
> return true;
> }
> *result = a - b;
> return false;
> 
> which fails to spot the fact that overflow is also possible when a ==
> 0. So on such platforms, it returns the wrong result.
> 
> Patch attached.

The patch looks good to me.

Yours,
Laurenz Albe

64-bit integer subtraction bug on some platforms

[Dean Rasheed]

One of the new tests in the infinite interval patch has revealed a bug
in our 64-bit integer subtraction code. Consider the following:

select 0::int8 - '-9223372036854775808'::int8;

This should overflow, since the correct result (+9223372036854775808)
is out of range. However, on platforms without integer overflow
builtins or 128-bit integers, pg_sub_s64_overflow() does the
following:

if ((a < 0 && b > 0 && a < PG_INT64_MIN + b) ||
(a > 0 && b < 0 && a > PG_INT64_MAX + b))
{
*result = 0x5EED;/* to avoid spurious warnings */
return true;
}
*result = a - b;
return false;

which fails to spot the fact that overflow is also possible when a ==
0. So on such platforms, it returns the wrong result.

Patch attached.

Regards,
Dean
diff --git a/src/include/common/int.h b/src/include/common/int.h
new file mode 100644
index 4508008..4871244
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -200,8 +200,12 @@ pg_sub_s64_overflow(int64 a, int64 b, in
 	*result = (int64) res;
 	return false;
 #else
+	/*
+	 * Note: overflow is also possible when a == 0 and b < 0 (specifically,
+	 * when b == PG_INT64_MIN).
+	 */
 	if ((a < 0 && b > 0 && a < PG_INT64_MIN + b) ||
-		(a > 0 && b < 0 && a > PG_INT64_MAX + b))
+		(a >= 0 && b < 0 && a > PG_INT64_MAX + b))
 	{
 		*result = 0x5EED;		/* to avoid spurious warnings */
 		return true;
diff --git a/src/test/regress/expected/int8.out b/src/test/regress/expected/int8.out
new file mode 100644
index 9542d62..fddc09f
--- a/src/test/regress/expected/int8.out
+++ b/src/test/regress/expected/int8.out
@@ -679,6 +679,8 @@ select -('-9223372036854775807'::int8);
 
 select -('-9223372036854775808'::int8);
 ERROR:  bigint out of range
+select 0::int8 - '-9223372036854775808'::int8;
+ERROR:  bigint out of range
 select '9223372036854775800'::int8 + '9223372036854775800'::int8;
 ERROR:  bigint out of range
 select '-9223372036854775800'::int8 + '-9223372036854775800'::int8;
diff --git a/src/test/regress/sql/int8.sql b/src/test/regress/sql/int8.sql
new file mode 100644
index 33f664d..fffb289
--- a/src/test/regress/sql/int8.sql
+++ b/src/test/regress/sql/int8.sql
@@ -132,6 +132,7 @@ select '9223372036854775808'::int8;
 
 select -('-9223372036854775807'::int8);
 select -('-9223372036854775808'::int8);
+select 0::int8 - '-9223372036854775808'::int8;
 
 select '9223372036854775800'::int8 + '9223372036854775800'::int8;
 select '-9223372036854775800'::int8 + '-9223372036854775800'::int8;