The INT_MIN / -1 crash problem was partially addressed in 2006 and commit 9fc6f4e1ae107f44807c4906105e1f7eb052ecb1.
http://archives.postgresql.org/pgsql-patches/2006-06/msg00102.php However, the fix is incomplete and incorrect for some cases. 64-bit crash ============ Below is an example that crashes PostgreSQL on Windows, using the 64-bit binary from http://www.postgresql.org/download/windows/. SELECT ((-9223372036854775808)::int8) % (-1); Note that the previous discussion concluded that int8 (64-bit) division didn't crash, which is incorrect. http://archives.postgresql.org/pgsql-patches/2006-06/msg00104.php My guess is that the previous test was carried out on a 32-bit Windows, where there's no 64-bit division instruction. In that case, the generated code calls a runtime function (e.g., lldiv), which doesn't trap. However, on a 64-bit system, the compiler generates the idivq instruction, which leads to a crash. Note that the problem is not limited to division. The following multiplication crashes as well: SELECT ((-9223372036854775808)::int8) * ((-1)::int8); The reason is that the multiplication overflow check uses a division. 32-bit crash ============ The previous fix doesn't fix all possible crashes, even on a 32-bit Windows. Below is an example to crash a 32-bit PostgreSQL: SELECT ((-2147483648)::int4) % ((-1)::int2); Portability =========== The previous fix uses #ifdef WIN32 ... #endif, which is not portable, as noted below: http://archives.postgresql.org/pgsql-patches/2006-06/msg00103.php Using a SIGFPE handler is also dangerous (e.g., causing an infinite loop sometimes): https://www.securecoding.cert.org/confluence/display/seccode/SIG35-C.+Do+not+return+from+SIGSEGV,+SIGILL,+or+SIGFPE+signal+handlers Strictly speaking, using postcondition checking to detect signed division overflows (actually, all signed integer overflows) violates the C language standard, because signed integer overflow is undefined behavior in C and we cannot first compute the result and then check for overflow. Compilers can do a lot of funny and crazy things (e.g., generating traps or even optimizing away overflow checks). BTW, PostgreSQL currently uses gcc's workaround option -fwrapv to disable offending optimizations based on integer overflows. The problems are: (1) it doesn't always work (e.g., for division), and (2) many other C compilers do not even support this workaround option and can perform offending optimizations. Patching ======== Below is a patch that fixes division crashes. It removes #ifdef WIN32 and tries to use portable checks. I'll send more (e.g., for fixing multiplication crashes) if this looks good. I understand that the existing integer overflow checks tried to avoid dependency on constants like INT64_MIN. But I'm not sure how to perform simpler and portable overflow checks without using such constants. Also, I could use the SHRT_MIN rather than introducing INT16_MIN; I just feel like using INT16_MIN with int16 is clearer and less confusing. diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c index 9f752ed..d7867cb 100644 --- a/src/backend/utils/adt/int.c +++ b/src/backend/utils/adt/int.c @@ -732,30 +732,18 @@ int4div(PG_FUNCTION_ARGS) PG_RETURN_NULL(); } -#ifdef WIN32 - /* - * Win32 doesn't throw a catchable exception for SELECT -2147483648 / - * (-1); -- INT_MIN + * Overflow check. The only possible overflow case is for arg1 = + * INT32_MIN, arg2 = -1, where the correct result is -INT32_MIN, which + * can't be represented on a two's-complement machine. */ - if (arg2 == -1 && arg1 == INT_MIN) + if (arg1 == INT32_MIN && arg2 == -1) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("integer out of range"))); -#endif result = arg1 / arg2; - /* - * Overflow check. The only possible overflow case is for arg1 = INT_MIN, - * arg2 = -1, where the correct result is -INT_MIN, which can't be - * represented on a two's-complement machine. Most machines produce - * INT_MIN but it seems some produce zero. - */ - if (arg2 == -1 && arg1 < 0 && result <= 0) - ereport(ERROR, - (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), - errmsg("integer out of range"))); PG_RETURN_INT32(result); } @@ -877,18 +865,17 @@ int2div(PG_FUNCTION_ARGS) PG_RETURN_NULL(); } - result = arg1 / arg2; - - /* - * Overflow check. The only possible overflow case is for arg1 = - * SHRT_MIN, arg2 = -1, where the correct result is -SHRT_MIN, which can't - * be represented on a two's-complement machine. Most machines produce - * SHRT_MIN but it seems some produce zero. + /* Overflow check. The only possible overflow case is for arg1 = + * INT16_MIN, arg2 = -1, where the correct result is -INT16_MIN, which + * can't be represented on a two's-complement machine. */ - if (arg2 == -1 && arg1 < 0 && result <= 0) + if (arg1 == INT16_MIN && arg2 == -1) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("smallint out of range"))); + + result = arg1 / arg2; + PG_RETURN_INT16(result); } @@ -1065,18 +1052,18 @@ int42div(PG_FUNCTION_ARGS) PG_RETURN_NULL(); } - result = arg1 / arg2; - /* - * Overflow check. The only possible overflow case is for arg1 = INT_MIN, - * arg2 = -1, where the correct result is -INT_MIN, which can't be - * represented on a two's-complement machine. Most machines produce - * INT_MIN but it seems some produce zero. + * Overflow check. The only possible overflow case is for arg1 = + * INT32_MIN, arg2 = -1, where the correct result is -INT32_MIN, which + * can't be represented on a two's-complement machine. */ - if (arg2 == -1 && arg1 < 0 && result <= 0) + if (arg1 == INT32_MIN && arg2 == -1) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("integer out of range"))); + + result = arg1 / arg2; + PG_RETURN_INT32(result); } diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c index 59c110b..83531ad 100644 --- a/src/backend/utils/adt/int8.c +++ b/src/backend/utils/adt/int8.c @@ -598,18 +598,18 @@ int8div(PG_FUNCTION_ARGS) PG_RETURN_NULL(); } - result = arg1 / arg2; - /* * Overflow check. The only possible overflow case is for arg1 = * INT64_MIN, arg2 = -1, where the correct result is -INT64_MIN, which - * can't be represented on a two's-complement machine. Most machines - * produce INT64_MIN but it seems some produce zero. + * can't be represented on a two's-complement machine. */ - if (arg2 == -1 && arg1 < 0 && result <= 0) + if (arg1 == INT64_MIN && arg2 == -1) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("bigint out of range"))); + + result = arg1 / arg2; + PG_RETURN_INT64(result); } @@ -838,18 +838,18 @@ int84div(PG_FUNCTION_ARGS) PG_RETURN_NULL(); } - result = arg1 / arg2; - /* * Overflow check. The only possible overflow case is for arg1 = * INT64_MIN, arg2 = -1, where the correct result is -INT64_MIN, which - * can't be represented on a two's-complement machine. Most machines - * produce INT64_MIN but it seems some produce zero. + * can't be represented on a two's-complement machine. */ - if (arg2 == -1 && arg1 < 0 && result <= 0) + if (arg1 == INT64_MIN && arg2 == -1) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("bigint out of range"))); + + result = arg1 / arg2; + PG_RETURN_INT64(result); } @@ -1026,18 +1026,18 @@ int82div(PG_FUNCTION_ARGS) PG_RETURN_NULL(); } - result = arg1 / arg2; - /* * Overflow check. The only possible overflow case is for arg1 = * INT64_MIN, arg2 = -1, where the correct result is -INT64_MIN, which - * can't be represented on a two's-complement machine. Most machines - * produce INT64_MIN but it seems some produce zero. + * can't be represented on a two's-complement machine. */ - if (arg2 == -1 && arg1 < 0 && result <= 0) + if (arg1 == INT64_MIN && arg2 == -1) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("bigint out of range"))); + + result = arg1 / arg2; + PG_RETURN_INT64(result); } diff --git a/src/include/c.h b/src/include/c.h index a6c0e6e..547a188 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -294,6 +294,29 @@ typedef unsigned long long int uint64; #define UINT64CONST(x) ((uint64) x) #endif +#ifndef INT16_MAX +#define INT16_MAX 32767 +#endif + +#ifndef INT16_MIN +#define INT16_MIN (-INT16_MAX-1) +#endif + +#ifndef INT32_MAX +#define INT32_MAX 2147483647 +#endif + +#ifndef INT32_MIN +#define INT32_MIN (-INT32_MAX-1) +#endif + +#ifndef INT64_MAX +#define INT64_MAX INT64CONST(9223372036854775807) +#endif + +#ifndef INT64_MIN +#define INT64_MIN (-INT64_MAX-1) +#endif /* Select timestamp representation (float8 or int64) */ #ifdef USE_INTEGER_DATETIMES -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
