Re: [HACKERS] [PATCH] avoid buffer underflow in errfinish()

2013-03-27 Thread Xi Wang
On Wed, Mar 27, 2013 at 9:03 AM, Heikki Linnakangas hlinnakan...@vmware.com 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

[HACKERS] [PATCH] avoid buffer underflow in errfinish()

2013-03-23 Thread Xi Wang
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

Re: [HACKERS] [PATCH] avoid buffer underflow in errfinish()

2013-03-23 Thread Xi Wang
the dereference `autovac-pgprocno' after the check? Thanks. On Sat, Mar 23, 2013 at 6:38 PM, Xi Wang xi.w...@gmail.com 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

Re: [HACKERS] [PATCH 0/3] Work around icc miscompilation

2013-02-23 Thread Xi Wang
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 t...@sss.pgh.pa.us wrote: Greg Stark st...@mit.edu writes:

[HACKERS] [PATCH 0/3] Work around icc miscompilation

2013-01-24 Thread Xi Wang
++ 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

[HACKERS] [PATCH 1/3] Fix x + y x overflow checks

2013-01-24 Thread Xi Wang
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 ++

[HACKERS] [PATCH 2/3] Fix x + 1 x overflow checks

2013-01-24 Thread Xi Wang
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

[HACKERS] [PATCH 3/3] Fix overflow checking in repeat()

2013-01-24 Thread Xi Wang
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

Re: [HACKERS] [PATCH 0/3] Work around icc miscompilation

2013-01-24 Thread Xi Wang
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

Re: [HACKERS] [PATCH 0/3] Work around icc miscompilation

2013-01-24 Thread Xi Wang
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

[HACKERS] [RFC] overflow checks optimized away

2013-01-22 Thread Xi Wang
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

Re: [HACKERS] [PATCH] Fix NULL checking in check_TSCurrentConfig()

2013-01-21 Thread Xi Wang
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

[HACKERS] [PATCH] Fix NULL checking in check_TSCurrentConfig()

2013-01-19 Thread Xi Wang
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 ---

[HACKERS] [PATCH] Fix off-by-one in PQprintTuples()

2013-01-19 Thread Xi Wang
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 ---

[HACKERS] [RFC] overflow checks optimized away

2013-01-19 Thread Xi Wang
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

Re: [HACKERS] [RFC] Fix div/mul crash and more undefined behavior

2012-11-19 Thread Xi Wang
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 =

[HACKERS] [RFC] Fix div/mul crash and more undefined behavior

2012-11-18 Thread Xi Wang
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

Re: [HACKERS] [RFC] Fix div/mul crash and more undefined behavior

2012-11-18 Thread Xi Wang
On 11/18/12 6:47 PM, Tom Lane wrote: Xi Wang xi.w...@gmail.com 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

[HACKERS] [PATCH] Fix INT_MIN % -1 overflow in int8mod().

2012-11-14 Thread Xi Wang
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

Re: [HACKERS] [PATCH] Fix INT_MIN % -1 overflow in int8mod().

2012-11-14 Thread Xi Wang
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

[HACKERS] [PATCH 1/2] Fix INT_MIN % -1 overflow in int8mod().

2012-11-14 Thread Xi Wang
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 ---

[HACKERS] [PATCH 2/2] Clean up INT_MIN % -1 overflow in int4mod().

2012-11-14 Thread Xi Wang
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 ---

Re: [HACKERS] [PATCH] Fix INT_MIN % -1 overflow in int8mod().

2012-11-14 Thread Xi Wang
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