[RFC] overflow checks optimized away

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

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 <limits.h>
+
#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

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

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 <limits.h>
+
#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

On 24.01.2013 11:33, Xi Wang wrote:

I'm sending three smaller patches for review, which try to fix icc
and pathscale (mis)compilation problems described in my last email.

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?

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

On Thu, Jan 24, 2013 at 6:36 AM, Xi Wang <xi.wang@gmail.com> wrote:

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.

I should mention gcc 4.7 does the same, and it emits a warning.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Xi Wang <xi.wang@gmail.com> writes:

On 1/24/13 5:02 AM, Heikki Linnakangas wrote:

If we fix these, can we stop using -frapv on gcc? Is there any way to
get gcc to warn about these?

I believe we can get rid of -fwrapv once we fix all such checks.

TBH, I find that statement to be nonsense, and these patches to be
completely the wrong way to go about it.

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.

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.

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

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

On Thu, Jan 24, 2013 at 4:36 AM, Xi Wang <xi.wang@gmail.com> wrote:

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.

As you post these patches, please add them to:

https://commitfest.postgresql.org/action/commitfest_view/open

This will ensure that they (eventually) get looked at.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jan 24, 2013 at 11:40:41AM -0500, Xi Wang wrote:

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.

I could not have said it better.

All other things being similar, I'd rather have PostgreSQL be written in C,
not C-with-signed-wrapraround. The latter has been a second class citizen for
over 20 years, and that situation won't be improving. However, compiler
options selecting it are common and decently-maintained. Changes along these
lines would become much more interesting if PostgreSQL has a live bug on a
modern compiler despite the use of available options comparable to -fwrapv.

When, if ever, to stop using -fwrapv (and use -ftrapv under --enable-cassert)
is another question. GCC 4.7 reports 999 warnings at -fstrict-overflow
-Wstrict-overflow=5. That doesn't mean 999 bugs to fix before we could remove
-fwrapv, but it does mean 999 places where the compiler will start generating
different code. That has a high chance of breaking something not covered by
the regression tests, so I'd hope to see a notable upside, perhaps a benchmark
improvement. It would also be instructive to know how much code actually
needs to change. Fixing 20 code sites in exchange for standard-conformance
and an X% performance improvement is a different proposition from fixing 200
code sites for the same benefit.

If we do start off in this direction at any scale, I suggest defining macros
like INT_MULT_OVERFLOWS(a, b) to wrap the checks. Then these changes would at
least make the code clearer, not less so.

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jan 24, 2013 at 3:48 PM, Tom Lane <tgl@sss.pgh.pa.us> 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.

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. And
the x = a+b won't execute unless it's well defined too. (Actually
we'll probably depend on the non-local exit behaviour of PG_THROW but
any compiler has to be able to deal with that anyways).

The point that we have no way to be sure we've gotten rid of any such
case is a good one. Logically as long as we're afraid of such things
we should continue to use -fwrapv and if we're using -fwrapv there's
no urgency to fix the code. But if we do get rid of all the known ones
then at least we have the option if we decide we've inspected the code
enough and had enough compilers check it to feel confident.

--
greg

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Greg Stark <stark@mit.edu> 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

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 <tgl@sss.pgh.pa.us> wrote:

Greg Stark <stark@mit.edu> 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

On Sat, Feb 23, 2013 at 5:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Really? Can't "INT_MAX - a" overflow?

I guess I shouldn't have tried summarizing the code and just pasted
some real code in for fear of getting it wrong. I was thinking of
unsigned arithmetic when I wrote that.

The point being that the compiler isn't going to make optimization
based on code that comes later if that code wouldn't have run in the
same flow of execution.

--
greg

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Xi Wang escribi�:

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.

This thread died without reaching a conclusion. Noah Misch, Robert Haas
and Greg Stark each gave a +1 to the patches, but Tom Lane gave them a
-inf; so they weren't applied. However, I think everyone walked away
with the feeling that Tom is wrong on this.

Meanwhile Xi Wang and team published a paper:
http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf

Postgres is mentioned a number of times in this paper -- mainly to talk
about the bugs we leave unfixed.

It might prove useful to have usable these guys' STACK checker output
available continuously, so that if we happen to introduce more bugs in
the future, it alerts us about that.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jul 15, 2013 at 5:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Xi Wang escribió:

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.

This thread died without reaching a conclusion. Noah Misch, Robert Haas
and Greg Stark each gave a +1 to the patches, but Tom Lane gave them a
-inf; so they weren't applied. However, I think everyone walked away
with the feeling that Tom is wrong on this.

Meanwhile Xi Wang and team published a paper:
http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf

Postgres is mentioned a number of times in this paper -- mainly to talk
about the bugs we leave unfixed.

It might prove useful to have usable these guys' STACK checker output
available continuously, so that if we happen to introduce more bugs in
the future, it alerts us about that.

Yeah, I think we ought to apply those patches. I don't suppose you
have links handy?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas escribi�:

On Mon, Jul 15, 2013 at 5:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Xi Wang escribi�:

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.

This thread died without reaching a conclusion. Noah Misch, Robert Haas
and Greg Stark each gave a +1 to the patches, but Tom Lane gave them a
-inf; so they weren't applied. However, I think everyone walked away
with the feeling that Tom is wrong on this.

Meanwhile Xi Wang and team published a paper:
http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf

Postgres is mentioned a number of times in this paper -- mainly to talk
about the bugs we leave unfixed.

It might prove useful to have usable these guys' STACK checker output
available continuously, so that if we happen to introduce more bugs in
the future, it alerts us about that.

Yeah, I think we ought to apply those patches. I don't suppose you
have links handy?

Sure, see this thread in the archives: first one is at
/messages/by-id/510100AA.4050105@gmail.com and you
can see the others in the dropdown (though since the subjects are not
shown, only date and author, it's a bit hard to follow. The "flat" URL
is useful.)

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jul 15, 2013 at 06:19:27PM -0400, Alvaro Herrera wrote:

Robert Haas escribi�:

On Mon, Jul 15, 2013 at 5:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Xi Wang escribi�:

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.

This thread died without reaching a conclusion. Noah Misch, Robert Haas
and Greg Stark each gave a +1 to the patches, but Tom Lane gave them a
-inf; so they weren't applied. However, I think everyone walked away
with the feeling that Tom is wrong on this.

Meanwhile Xi Wang and team published a paper:
http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf

Postgres is mentioned a number of times in this paper -- mainly to talk
about the bugs we leave unfixed.

It might prove useful to have usable these guys' STACK checker output
available continuously, so that if we happen to introduce more bugs in
the future, it alerts us about that.

Yeah, I think we ought to apply those patches. I don't suppose you
have links handy?

Sure, see this thread in the archives: first one is at
/messages/by-id/510100AA.4050105@gmail.com and you
can see the others in the dropdown (though since the subjects are not
shown, only date and author, it's a bit hard to follow. The "flat" URL
is useful.)

Should these patches be applied?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers