Re: [HACKERS] [PATCH] avoid buffer underflow in errfinish()
On Wed, Mar 27, 2013 at 9:03 AM, Heikki Linnakangas wrote: > Writing it like that suggests that autovac might sometimes be NULL, even if > deadlock_state == DS_BLOCKED_BY_AUTOVACUUM. From your explanation above, I > gather that's not possible (and I think you're right), so the NULL check is > unnecessary. If we think it might be NULL after all, the above makes sense. That makes sense. Thanks for the clarification! - xi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] avoid buffer underflow in errfinish()
A side question: at src/backend/storage/lmgr/proc.c:1150, is there a
null pointer deference for `autovac'?
There is a null pointer check `autovac != NULL', but the pointer is
already dereferenced earlier when initializing `autovac_pgxact'. Is
this null pointer check redundant, or should we move the dereference
`autovac->pgprocno' after the check? Thanks.
On Sat, Mar 23, 2013 at 6:38 PM, Xi Wang wrote:
> CHECK_STACK_DEPTH checks if errordata_stack_depth is negative.
> Move the dereference of &errordata[errordata_stack_depth] after
> the check to avoid out-of-bounds read.
> ---
> src/backend/utils/error/elog.c |4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
> index 3a211bf..47a0a8b 100644
> --- a/src/backend/utils/error/elog.c
> +++ b/src/backend/utils/error/elog.c
> @@ -393,13 +393,15 @@ void
> errfinish(int dummy,...)
> {
> ErrorData *edata = &errordata[errordata_stack_depth];
> - int elevel = edata->elevel;
> + int elevel;
> MemoryContext oldcontext;
> ErrorContextCallback *econtext;
>
> recursion_depth++;
> CHECK_STACK_DEPTH();
>
> + elevel = edata->elevel;
> +
> /*
> * Do processing in ErrorContext, which we hope has enough reserved
> space
> * to report an error.
> --
> 1.7.10.4
>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] avoid buffer underflow in errfinish()
CHECK_STACK_DEPTH checks if errordata_stack_depth is negative.
Move the dereference of &errordata[errordata_stack_depth] after
the check to avoid out-of-bounds read.
---
src/backend/utils/error/elog.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 3a211bf..47a0a8b 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -393,13 +393,15 @@ void
errfinish(int dummy,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
- int elevel = edata->elevel;
+ int elevel;
MemoryContext oldcontext;
ErrorContextCallback *econtext;
recursion_depth++;
CHECK_STACK_DEPTH();
+ elevel = edata->elevel;
+
/*
* Do processing in ErrorContext, which we hope has enough reserved
space
* to report an error.
--
1.7.10.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 0/3] Work around icc miscompilation
It depends on the context. In the patches, `a' is known to be
non-negative, so `INT_MAX - a' cannot overflow. If you ignore the
context and talk about the general case, then it can.
- xi
On Sat, Feb 23, 2013 at 12:25 PM, Tom Lane wrote:
> Greg Stark writes:
>> He's changing things to do
>
>> if (INT_MAX - a > b)
>> PG_THROW ("a+b would overflow")
>> else
>> x=a+b;
>
>> Why would a smarter compiler be licensed to conclude that it can
>> optimize away anything? "INT_MAX-a > b" is always well defined.
>
> Really? Can't "INT_MAX - a" overflow?
>
> regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 0/3] Work around icc miscompilation
On 1/24/13 10:48 AM, Tom Lane wrote: > The fundamental problem here is that the compiler, unless told otherwise > by a compilation switch, believes it is entitled to assume that no > integer overflow will happen anywhere in the program. Therefore, any > error check that is looking for overflow *should* get optimized away. > The only reason the compiler would fail to do that is if its optimizer > isn't quite smart enough to prove that the code is testing for an > overflow condition. So what you are proposing here is merely the next > move in an arms race with the compiler writers, and it will surely > break again in a future generation of compilers. Or even if these > particular trouble spots don't break, something else will. The only > reliable solution is to not let the compiler make that type of > assumption. What I am proposing here is the opposite: _not_ to enter an arm race with the compiler writers. Instead, make the code conform to the C standard, something both sides can agree on. Particularly, avoid using (signed) overflowed results to detect overflows, which the C standard clearly specifies as undefined behavior and many compilers are actively exploiting. We could use either unsigned overflows (which is well defined) or precondition testing (like `x > INT_MAX - y' in the patches). > So I think we should just reject all of these, and instead fix configure > to make sure it turns on icc's equivalent of -fwrapv. While I agree it's better to turn on icc's -fno-strict-overflow as a workaround, the fundamental problem here is that we are _not_ programming in the C language. Rather, we are programming in some C-with-signed-wrapraround dialect. The worst part of this C dialect is that it has never been specified clearly what it means by "signed wraparound": gcc's -fwrapv assumes signed wraparound for add/sub/mul, but not for div (e.g., INT_MIN/-1 traps on x86) nor shift (e.g., 1<<32 produces undef with clang). clang's -fwrapv also assumes workaround for pointer arithmetic, while gcc's does not. I have no idea what icc and pathscale's -fno-strict-overflow option does (probably the closest thing to -fwrapv). Sometimes it prevents such checks from being optimized away, sometimes it doesn't. Anyway, it would not be surprising if future C compilers break this dialect again. But they shouldn't break standard-conforming code. - xi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 0/3] Work around icc miscompilation
On 1/24/13 5:02 AM, Heikki Linnakangas wrote: > These patches look ok at a quick glance, but how do we ensure this kind > of problems don't crop back again in the future? Does icc give a warning > about these? Do we have a buildfarm animal that produces the warnings? > > If we fix these, can we stop using -frapv on gcc? Is there any way to > get gcc to warn about these? Thanks for reviewing. gcc has this -Wstrict-overflow option to warn against overflow checks that may be optimized away. The result in inaccurate: it may produce a large number of false warnings, and it may also miss many cases (esp. when gcc's value-range-propagation fails to compute variables' ranges). Not sure if other compilers have similar options. I find these broken checks using a static checker I'm developing, and only report cases that existing compilers do miscompile. If you are interested, I'll post a complete list of overflow checks in pgsql that invoke undefined behavior and thus may be killed by future compilers. I believe we can get rid of -fwrapv once we fix all such checks. - xi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 3/3] Fix overflow checking in repeat()
icc optimizes away `check + VARHDRSZ <= check' since signed integer
overflow is undefined behavior. Simplify the overflow check for
`VARHDRSZ + count * slen' as `count > (INT_MAX - VARHDRSZ) / slen'.
---
src/backend/utils/adt/oracle_compat.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/adt/oracle_compat.c
b/src/backend/utils/adt/oracle_compat.c
index d448088..a85a672 100644
--- a/src/backend/utils/adt/oracle_compat.c
+++ b/src/backend/utils/adt/oracle_compat.c
@@ -15,6 +15,8 @@
*/
#include "postgres.h"
+#include
+
#include "utils/builtins.h"
#include "utils/formatting.h"
#include "mb/pg_wchar.h"
@@ -1034,20 +1036,14 @@ repeat(PG_FUNCTION_ARGS)
count = 0;
slen = VARSIZE_ANY_EXHDR(string);
- tlen = VARHDRSZ + (count * slen);
/* Check for integer overflow */
- if (slen != 0 && count != 0)
- {
- int check = count * slen;
- int check2 = check + VARHDRSZ;
-
- if ((check / slen) != count || check2 <= check)
- ereport(ERROR,
-
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
-errmsg("requested length too large")));
- }
+ if (slen != 0 && count > (INT_MAX - VARHDRSZ) / slen)
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+errmsg("requested length too large")));
+ tlen = VARHDRSZ + (count * slen);
result = (text *) palloc(tlen);
SET_VARSIZE(result, tlen);
--
1.7.10.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 2/3] Fix x + 1 < x overflow checks
icc optimizes away x + 1 < x because signed integer overflow is
undefined behavior in C. Instead, simply check if x is INT_MAX.
---
src/backend/utils/adt/float.c |8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index b73e0d5..344b092 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2764,12 +2764,12 @@ width_bucket_float8(PG_FUNCTION_ARGS)
result = 0;
else if (operand >= bound2)
{
- result = count + 1;
/* check for overflow */
- if (result < count)
+ if (count == INT_MAX)
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of
range")));
+ result = count + 1;
}
else
result = ((float8) count * (operand - bound1) / (bound2
- bound1)) + 1;
@@ -2780,12 +2780,12 @@ width_bucket_float8(PG_FUNCTION_ARGS)
result = 0;
else if (operand <= bound2)
{
- result = count + 1;
/* check for overflow */
- if (result < count)
+ if (count == INT_MAX)
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of
range")));
+ result = count + 1;
}
else
result = ((float8) count * (bound1 - operand) / (bound1
- bound2)) + 1;
--
1.7.10.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 1/3] Fix x + y < x overflow checks
icc optimizes away the overflow check x + y < x (y > 0), because
signed integer overflow is undefined behavior in C. Instead, use
a safe precondition test x > INT_MAX - y.
---
src/backend/utils/adt/varbit.c |7 +--
src/backend/utils/adt/varlena.c | 10 ++
src/pl/plpgsql/src/pl_exec.c|4 ++--
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/varbit.c b/src/backend/utils/adt/varbit.c
index 1712c12..af69200 100644
--- a/src/backend/utils/adt/varbit.c
+++ b/src/backend/utils/adt/varbit.c
@@ -16,6 +16,8 @@
#include "postgres.h"
+#include
+
#include "access/htup_details.h"
#include "libpq/pqformat.h"
#include "nodes/nodeFuncs.h"
@@ -1138,12 +1140,13 @@ bit_overlay(VarBit *t1, VarBit *t2, int sp, int sl)
ereport(ERROR,
(errcode(ERRCODE_SUBSTRING_ERROR),
errmsg("negative substring length not
allowed")));
- sp_pl_sl = sp + sl;
- if (sp_pl_sl <= sl)
+ if (sl > INT_MAX - sp)
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
+ sp_pl_sl = sp + sl;
+
s1 = bitsubstring(t1, 1, sp - 1, false);
s2 = bitsubstring(t1, sp_pl_sl, -1, true);
result = bit_catenate(s1, t2);
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 95e41bf..c907f44 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -997,12 +997,13 @@ text_overlay(text *t1, text *t2, int sp, int sl)
ereport(ERROR,
(errcode(ERRCODE_SUBSTRING_ERROR),
errmsg("negative substring length not
allowed")));
- sp_pl_sl = sp + sl;
- if (sp_pl_sl <= sl)
+ if (sl > INT_MAX - sp)
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
+ sp_pl_sl = sp + sl;
+
s1 = text_substring(PointerGetDatum(t1), 1, sp - 1, false);
s2 = text_substring(PointerGetDatum(t1), sp_pl_sl, -1, true);
result = text_catenate(s1, t2);
@@ -2020,12 +2021,13 @@ bytea_overlay(bytea *t1, bytea *t2, int sp, int sl)
ereport(ERROR,
(errcode(ERRCODE_SUBSTRING_ERROR),
errmsg("negative substring length not
allowed")));
- sp_pl_sl = sp + sl;
- if (sp_pl_sl <= sl)
+ if (sl > INT_MAX - sp)
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
+ sp_pl_sl = sp + sl;
+
s1 = bytea_substring(PointerGetDatum(t1), 1, sp - 1, false);
s2 = bytea_substring(PointerGetDatum(t1), sp_pl_sl, -1, true);
result = bytea_catenate(s1, t2);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 0ecf651..a9cf6df 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1972,13 +1972,13 @@ exec_stmt_fori(PLpgSQL_execstate *estate,
PLpgSQL_stmt_fori *stmt)
*/
if (stmt->reverse)
{
- if ((int32) (loop_value - step_value) > loop_value)
+ if (loop_value < INT_MIN + step_value)
break;
loop_value -= step_value;
}
else
{
- if ((int32) (loop_value + step_value) < loop_value)
+ if (loop_value > INT_MAX - step_value)
break;
loop_value += step_value;
}
--
1.7.10.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 0/3] Work around icc miscompilation
I'm sending three smaller patches for review, which try to fix icc
and pathscale (mis)compilation problems described in my last email.
Not sure if we need to fix the overflow check x + 1 <= 0 (x > 0) at
src/backend/executor/execQual.c:3088, which icc optimizes away, too.
Fix x + y < x overflow checks
Fix x + 1 < x overflow checks
Fix overflow checking in repeat()
src/backend/utils/adt/float.c |8
src/backend/utils/adt/oracle_compat.c | 18 +++---
src/backend/utils/adt/varbit.c|7 +--
src/backend/utils/adt/varlena.c | 10 ++
src/pl/plpgsql/src/pl_exec.c |4 ++--
5 files changed, 24 insertions(+), 23 deletions(-)
On 1/20/13 2:58 AM, Xi Wang wrote:
> Intel's icc and PathScale's pathcc compilers optimize away several
> overflow checks, since they consider signed integer overflow as
> undefined behavior. This leads to a vulnerable binary.
>
> Currently we use -fwrapv to disable such (mis)optimizations in gcc,
> but not in other compilers.
>
>
> Examples
>
>
> 1) x + 1 <= 0 (assuming x > 0).
>
> src/backend/executor/execQual.c:3088
>
> Below is the simplified code.
>
> -
> void bar(void);
> void foo(int this_ndims)
> {
> if (this_ndims <= 0)
> return;
> int elem_ndims = this_ndims;
> int ndims = elem_ndims + 1;
> if (ndims <= 0)
> bar();
> }
> -
>
> $ icc -S -o - sadd1.c
> ...
> foo:
> # parameter 1: %edi
> ..B1.1:
> ..___tag_value_foo.1:
> ..B1.2:
> ret
>
> 2) x + 1 < x
>
> src/backend/utils/adt/float.c:2769
> src/backend/utils/adt/float.c:2785
> src/backend/utils/adt/oracle_compat.c:1045 (x + C < x)
>
> Below is the simplified code.
>
> -
> void bar(void);
> void foo(int count)
> {
> int result = count + 1;
> if (result < count)
> bar();
> }
> -
>
> $ icc -S -o - sadd2.c
> ...
> foo:
> # parameter 1: %edi
> ..B1.1:
> ..___tag_value_foo.1:
> ret
> 3) x + y <= x (assuming y > 0)
>
> src/backend/utils/adt/varbit.c:1142
> src/backend/utils/adt/varlena.c:1001
> src/backend/utils/adt/varlena.c:2024
> src/pl/plpgsql/src/pl_exec.c:1975
> src/pl/plpgsql/src/pl_exec.c:1981
>
> Below is the simplified code.
>
> -
> void bar(void);
> void foo(int sp, int sl)
> {
> if (sp <= 0)
> return;
> int sp_pl_sl = sp + sl;
> if (sp_pl_sl <= sl)
> bar();
> }
> -
>
> $ icc -S -o - sadd3.c
> foo:
> # parameter 1: %edi
> # parameter 2: %esi
> ..B1.1:
> ..___tag_value_foo.1:
> ..B1.2:
> ret
>
> Possible fixes
> ==
>
> * Recent versions of icc and pathcc support gcc's workaround option,
> -fno-strict-overflow, to disable some optimizations based on signed
> integer overflow. It's better to add this option to configure.
> They don't support gcc's -fwrapv yet.
>
> * This -fno-strict-overflow option cannot help in all cases: it cannot
> prevent the latest icc from (mis)compiling the 1st case. We could also
> fix the source code by avoiding signed integer overflows, as follows.
>
> x + y <= 0 (assuming x > 0, y > 0)
> --> x > INT_MAX - y
>
> x + y <= x (assuming y > 0)
> --> x > INT_MAX - y
>
> I'd suggest to fix the code rather than trying to work around the
> compilers since the fix seems simple and portable.
>
> See two recent compiler bugs of -fwrapv/-fno-strict-overflow as well.
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55883
> http://software.intel.com/en-us/forums/topic/358200
>
> * I don't have access to IBM's xlc compiler. Not sure how it works for
> the above cases.
>
> - xi
>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [RFC] overflow checks optimized away
Intel's icc and PathScale's pathcc compilers optimize away several
overflow checks, since they consider signed integer overflow as
undefined behavior. This leads to a vulnerable binary.
Currently we use -fwrapv to disable such (mis)optimizations in gcc,
but not in other compilers.
Examples
1) x + 1 <= 0 (assuming x > 0).
src/backend/executor/execQual.c:3088
Below is the simplified code.
-
void bar(void);
void foo(int this_ndims)
{
if (this_ndims <= 0)
return;
int elem_ndims = this_ndims;
int ndims = elem_ndims + 1;
if (ndims <= 0)
bar();
}
-
$ icc -S -o - sadd1.c
...
foo:
# parameter 1: %edi
..B1.1:
..___tag_value_foo.1:
..B1.2:
ret
2) x + 1 < x
src/backend/utils/adt/float.c:2769
src/backend/utils/adt/float.c:2785
src/backend/utils/adt/oracle_compat.c:1045 (x + C < x)
Below is the simplified code.
-
void bar(void);
void foo(int count)
{
int result = count + 1;
if (result < count)
bar();
}
-
$ icc -S -o - sadd2.c
...
foo:
# parameter 1: %edi
..B1.1:
..___tag_value_foo.1:
ret
3) x + y <= x (assuming y > 0)
src/backend/utils/adt/varbit.c:1142
src/backend/utils/adt/varlena.c:1001
src/backend/utils/adt/varlena.c:2024
src/pl/plpgsql/src/pl_exec.c:1975
src/pl/plpgsql/src/pl_exec.c:1981
Below is the simplified code.
-
void bar(void);
void foo(int sp, int sl)
{
if (sp <= 0)
return;
int sp_pl_sl = sp + sl;
if (sp_pl_sl <= sl)
bar();
}
-
$ icc -S -o - sadd3.c
foo:
# parameter 1: %edi
# parameter 2: %esi
..B1.1:
..___tag_value_foo.1:
..B1.2:
ret
Possible fixes
==
* Recent versions of icc and pathcc support gcc's workaround option,
-fno-strict-overflow, to disable some optimizations based on signed
integer overflow. It's better to add this option to configure.
They don't support gcc's -fwrapv yet.
* This -fno-strict-overflow option cannot help in all cases: it cannot
prevent the latest icc from (mis)compiling the 1st case. We could also
fix the source code by avoiding signed integer overflows, as follows.
x + y <= 0 (assuming x > 0, y > 0)
--> x > INT_MAX - y
x + y <= x (assuming y > 0)
--> x > INT_MAX - y
I'd suggest to fix the code rather than trying to work around the
compilers since the fix seems simple and portable.
See two recent compiler bugs of -fwrapv/-fno-strict-overflow as well.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55883
http://software.intel.com/en-us/forums/topic/358200
* I don't have access to IBM's xlc compiler. Not sure how it works for
the above cases.
- xi
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Fix NULL checking in check_TSCurrentConfig()
On 1/20/13 10:35 PM, Tom Lane wrote: Great catch, will commit. (But first I'm looking through commit 2594cf0e to see if I made the same mistake anywhere else :-(.) How did you find that, coverity or some such tool? Thanks for reviewing the patch. It was found using a homemade static undefined behavior checker. - xi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [RFC] overflow checks optimized away
Intel's icc and PathScale's pathcc compilers optimize away several
overflow checks, since they consider signed integer overflow as
undefined behavior. This leads to a vulnerable binary.
Currently we use -fwrapv to disable such (mis)optimizations in gcc,
but not in other compilers.
Examples
1) x + 1 <= 0 (assuming x > 0).
src/backend/executor/execQual.c:3088
Below is the simplified code.
-
void bar(void);
void foo(int this_ndims)
{
if (this_ndims <= 0)
return;
int elem_ndims = this_ndims;
int ndims = elem_ndims + 1;
if (ndims <= 0)
bar();
}
-
$ icc -S -o - sadd1.c
...
foo:
# parameter 1: %edi
..B1.1:
..___tag_value_foo.1:
..B1.2:
ret
2) x + 1 < x
src/backend/utils/adt/float.c:2769
src/backend/utils/adt/float.c:2785
src/backend/utils/adt/oracle_compat.c:1045 (x + C < x)
Below is the simplified code.
-
void bar(void);
void foo(int count)
{
int result = count + 1;
if (result < count)
bar();
}
-
$ icc -S -o - sadd2.c
...
foo:
# parameter 1: %edi
..B1.1:
..___tag_value_foo.1:
ret
3) x + y <= x (assuming y > 0)
src/backend/utils/adt/varbit.c:1142
src/backend/utils/adt/varlena.c:1001
src/backend/utils/adt/varlena.c:2024
src/pl/plpgsql/src/pl_exec.c:1975
src/pl/plpgsql/src/pl_exec.c:1981
Below is the simplified code.
-
void bar(void);
void foo(int sp, int sl)
{
if (sp <= 0)
return;
int sp_pl_sl = sp + sl;
if (sp_pl_sl <= sl)
bar();
}
-
$ icc -S -o - sadd3.c
foo:
# parameter 1: %edi
# parameter 2: %esi
..B1.1:
..___tag_value_foo.1:
..B1.2:
ret
Possible fixes
==
* Recent versions of icc and pathcc support gcc's workaround option,
-fno-strict-overflow, to disable some optimizations based on signed
integer overflow. It's better to add this option to configure.
They don't support gcc's -fwrapv yet.
* This -fno-strict-overflow option cannot help in all cases: it cannot
prevent the latest icc from (mis)compiling the 1st case. We could also
fix the source code by avoiding signed integer overflows, as follows.
x + y <= 0 (assuming x > 0, y > 0)
--> x > INT_MAX - y
x + y <= x (assuming y > 0)
--> x > INT_MAX - y
I'd suggest to fix the code rather than trying to work around the
compilers since the fix seems simple and portable.
See two recent compiler bugs of -fwrapv/-fno-strict-overflow as well.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55883
http://software.intel.com/en-us/forums/topic/358200
* I don't have access to IBM's xlc compiler. Not sure how it works for
the above cases.
- xi
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Fix off-by-one in PQprintTuples()
Don't write past the end of tborder; the size is width + 1.
---
src/interfaces/libpq/fe-print.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/interfaces/libpq/fe-print.c b/src/interfaces/libpq/fe-print.c
index 076e1cc..7ed489a 100644
--- a/src/interfaces/libpq/fe-print.c
+++ b/src/interfaces/libpq/fe-print.c
@@ -706,7 +706,7 @@ PQprintTuples(const PGresult *res,
fprintf(stderr, libpq_gettext("out of
memory\n"));
abort();
}
- for (i = 0; i <= width; i++)
+ for (i = 0; i < width; i++)
tborder[i] = '-';
tborder[i] = '\0';
fprintf(fout, "%s\n", tborder);
--
1.7.10.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Fix NULL checking in check_TSCurrentConfig()
The correct NULL check should use `*newval'; `newval' must be non-null. --- src/backend/utils/cache/ts_cache.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c index e688b1a..65a8ad7 100644 --- a/src/backend/utils/cache/ts_cache.c +++ b/src/backend/utils/cache/ts_cache.c @@ -642,7 +642,7 @@ check_TSCurrentConfig(char **newval, void **extra, GucSource source) free(*newval); *newval = strdup(buf); pfree(buf); - if (!newval) + if (!*newval) return false; } -- 1.7.10.4 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] Fix div/mul crash and more undefined behavior
On 11/19/12 11:04 AM, Tom Lane wrote:
> I thought about this some more and realized that we can handle it
> by realizing that division by -1 is the same as negation, and so
> we can copy the method used in int4um. So the code would look like
>
> if (arg2 == -1)
> {
> result = -arg1;
> if (arg1 != 0 && SAMESIGN(result, arg1))
> ereport(ERROR, ...);
> PG_RETURN_INT32(result);
> }
>
> (with rather more comments than this, of course). This looks faster
> than what's there now, as well as removing the need for use of
> explicit INT_MIN constants.
Hah, I used exactly the same code in my initial patch. :)
See below.
> They'd better not, else they'll break many of our overflow checks.
> This is why we use -fwrapv with gcc, for example. Any other compiler
> with similar optimizations needs to be invoked with a similar switch.
Yes, that's the scary part.
Even with -fwrapv in gcc (I tried 4.6/4.7/4.8), it still optimizes away
my overflow check!
if (arg1 < 0 && -arg1 < 0) { ... }
Fortunately, it doesn't optimize way checks using SAMESIGN() for now.
Who knows what could happen in the next version..
Clang's -fwrapv works as expected.
PathScale and Open64 (and probably icc) don't support -fwrapv at all.
Not sure if they have similar options. They do such optimizations, too.
The reality is that C compilers are not friendly to postcondition
checking; they consider signed integer overflow as undefined behavior,
so they do whatever they want to do. Even workaround options like
-fwrapv are often broken, not to mention that they may not even have
those options.
That's why I am suggesting to avoid postcondition checking for signed
integer overflow.
> Not every C compiler provides stdint.h, unfortunately --- otherwise
> I'd not be so resistant to depending on this.
Sigh.. You are right.
- xi
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] Fix div/mul crash and more undefined behavior
On 11/18/12 6:47 PM, Tom Lane wrote:
> Xi Wang writes:
>> [ patch adding a bunch of explicit INT_MIN/MAX constants ]
>
> I was against this style of coding before, and I still am.
> For one thing, it's just about certain to introduce conflicts
> against system headers.
I totally agree.
I would be happy to rewrite the integer overflow checks without
using these explicit constants, but it seems extremely tricky to
do so. One possible check without using INTn_MIN is:
if (arg1 < 0 && -arg1 < 0 && arg2 == -1) { ... }
Compared to (arg1 == INTn_MIN && arg2 == -1), the above check is
not only more confusing and difficult to understand, but it also
invokes undefined behavior (-INT_MIN overflow), which is dangerous:
many C compilers will optimize away the check. I've tried gcc,
clang, PathScale, and AMD's Open64, all of which perform such
optimizations.
Since INTn_MIN and INTn_MAX are standard macros from the C library,
can we assume that every C compiler should provide them in stdint.h?
At least this is true for gcc, clang, and Visual C++. Then we don't
have to define them and worry about possible conflicts (though I
think using #ifndef...#endif should be able to avoid conflicts).
- xi
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [RFC] Fix div/mul crash and more undefined behavior
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.
Re: [HACKERS] [PATCH] Fix INT_MIN % -1 overflow in int8mod().
On 11/14/12 5:03 PM, Tom Lane wrote: > No need, I'm already patching it. Oops. Sorry. Ignore my patches. - xi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 2/2] Clean up INT_MIN % -1 overflow in int4mod().
Since x % -1 returns 0 for any x, we don't need the check x == INT_MIN. Suggested by Tom Lane. --- src/backend/utils/adt/int.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c index 4be3901..3e423fe 100644 --- a/src/backend/utils/adt/int.c +++ b/src/backend/utils/adt/int.c @@ -1096,7 +1096,7 @@ int4mod(PG_FUNCTION_ARGS) } /* SELECT ((-2147483648)::int4) % (-1); causes a floating point exception */ - if (arg1 == INT_MIN && arg2 == -1) + if (arg2 == -1) PG_RETURN_INT32(0); /* No overflow is possible */ -- 1.7.10.4 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 1/2] Fix INT_MIN % -1 overflow in int8mod().
Return 0 for x % -1 instead of throwing an exception (e.g., when x is INT_MIN). Suggested by Tom Lane. --- src/backend/utils/adt/int8.c |4 1 file changed, 4 insertions(+) diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c index 0e59956..a30ab36 100644 --- a/src/backend/utils/adt/int8.c +++ b/src/backend/utils/adt/int8.c @@ -649,6 +649,10 @@ int8mod(PG_FUNCTION_ARGS) PG_RETURN_NULL(); } + /* SELECT ((-9223372036854775808)::int8) % (-1); causes a floating point exception */ + if (arg2 == -1) + PG_RETURN_INT64(0); + /* No overflow is possible */ PG_RETURN_INT64(arg1 % arg2); -- 1.7.10.4 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Fix INT_MIN % -1 overflow in int8mod().
On 11/14/12 4:46 PM, Tom Lane wrote: > Meh. I didn't care for the explicit dependency on INT_MIN in the > previous patch, and I like introducing INT64_MIN even less. I think > we should be able to reduce the test to just > > if (arg2 == -1) > return 0; > > since zero is the correct result for any value of arg1, not only > INT_MIN. I agree. Will send a v2. Thanks. :) - xi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Fix INT_MIN % -1 overflow in int8mod().
Return 0 for INT_MIN % -1 (64-bit) instead of throwing an exception. This patch complements commit f9ac414c that fixed int4mod(). --- src/backend/utils/adt/int8.c |4 src/include/c.h |7 +++ 2 files changed, 11 insertions(+) diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c index 0e59956..9da651b 100644 --- a/src/backend/utils/adt/int8.c +++ b/src/backend/utils/adt/int8.c @@ -649,6 +649,10 @@ int8mod(PG_FUNCTION_ARGS) PG_RETURN_NULL(); } + /* SELECT ((-9223372036854775808)::int8) % (-1); causes a floating point exception */ + if (arg1 == INT64_MIN && arg2 == -1) + PG_RETURN_INT64(0); + /* No overflow is possible */ PG_RETURN_INT64(arg1 % arg2); diff --git a/src/include/c.h b/src/include/c.h index a6c0e6e..d20ba8c 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -294,6 +294,13 @@ typedef unsigned long long int uint64; #define UINT64CONST(x) ((uint64) x) #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 -- 1.7.10.4 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
