clang -fsanitize=undefined error in ecpg

Started by Peter Eisentrautalmost 11 years ago2 messages

peter_e@gmx.net

almost 11 years ago

With clang -fsanitize=undefined (clang-3.4), I get the following test failure in ecpg
(it's the only one in the entire tree):

--- a/src/interfaces/ecpg/test/expected/pgtypeslib-dt_test2.stderr
+++ b/src/interfaces/ecpg/test/results/pgtypeslib-dt_test2.stderr
@@ -1,2 +1,4 @@
 [NO_PID]: ECPGdebug: set to 1
 [NO_PID]: sqlca: code: 0, state: 00000
+dt_common.c:2209:13: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
+dt_common.c:1424:12: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'

This happens while parsing these strings:

"19990108foobar"
"19990108 foobar",
"1999-01-08 foobar"
"........................Xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"

(I'm not sure why it reports two warnings for four cases. Maybe it collapses some warnings.)

This patch fixes it:

diff --git a/src/interfaces/ecpg/pgtypeslib/dt.h b/src/interfaces/ecpg/pgtypeslib/dt.h
index 145e2b7..2ccd0be 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt.h
+++ b/src/interfaces/ecpg/pgtypeslib/dt.h
@@ -193,7 +193,7 @@ typedef double fsec_t;
  * Bit mask definitions for time parsing.
  */
 /* Copy&pasted these values from src/include/utils/datetime.h */
-#define DTK_M(t)               (0x01 << (t))
+#define DTK_M(t)               ((t) == UNKNOWN_FIELD ? 0 : 0x01 << (t))
 #define DTK_ALL_SECS_M    (DTK_M(SECOND) | DTK_M(MILLISECOND) | DTK_M(MICROSECOND))
 #define DTK_DATE_M             (DTK_M(YEAR) | DTK_M(MONTH) | DTK_M(DAY))
 #define DTK_TIME_M             (DTK_M(HOUR) | DTK_M(MINUTE) | DTK_M(SECOND))

Strangely, I cannot reproduce this failure with the backend datetime code that
this was supposedly copied from.

Comments?

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

Tom Lane

tgl@sss.pgh.pa.us

almost 11 years ago

In reply to: Peter Eisentraut (#1)

Re: clang -fsanitize=undefined error in ecpg

Peter Eisentraut <peter_e@gmx.net> writes:

With clang -fsanitize=undefined (clang-3.4), I get the following test failure in ecpg
(it's the only one in the entire tree):

Hm. I don't know why you can't reproduce that in the backend, because
when stepping through DecodeDateTime() on the input
select '19990108foobar'::timestamptz;
I definitely see it shifting 1 left 31 places:

Breakpoint 2, DecodeDateTime (field=<value optimized out>,
ftype=<value optimized out>, nf=2, dtype=0x7ffff6fec168,
tm=0x7ffff6fec170, fsec=0x7ffff6fec1b4, tzp=0x7ffff6fec16c)
at datetime.c:1193
1193 type = DecodeTimezoneAbbrev(i, field[i], &val, &valtz);
(gdb) n
1194 if (type == UNKNOWN_FIELD)
(gdb) p type
$1 = 31
(gdb) s
1195 type = DecodeSpecial(i, field[i], &val);
(gdb) s
DecodeSpecial (field=1, lowtoken=0x7ffff6febf89 "foobar", val=0x7ffff6febf28)
at datetime.c:3018
3018 {
(gdb) fin
Run till exit from #0 DecodeSpecial (field=1,
lowtoken=0x7ffff6febf89 "foobar", val=0x7ffff6febf28) at datetime.c:3031
DecodeDateTime (field=<value optimized out>, ftype=<value optimized out>,
nf=2, dtype=0x7ffff6fec168, tm=0x7ffff6fec170, fsec=0x7ffff6fec1b4,
tzp=0x7ffff6fec16c) at datetime.c:1196
1196 if (type == IGNORE_DTF)
Value returned is $2 = 31
(gdb) s
1199 tmask = DTK_M(type);
(gdb) p type
$3 = 31
(gdb) s
1200 switch (type)
(gdb) p tmask
$4 = -2147483648

This patch fixes it:

-#define DTK_M(t)               (0x01 << (t))
+#define DTK_M(t)               ((t) == UNKNOWN_FIELD ? 0 : 0x01 << (t))

Don't like that even a little bit. The intent of the code is perfectly
clear, cf this comment in datetime.h:

* Field types for time decoding.
*
* Can't have more of these than there are bits in an unsigned int
* since these are turned into bit masks during parsing and decoding.

So I think the correct fix is

-#define DTK_M(t)               (0x01 << (t))
+#define DTK_M(t)               (0x01U << (t))

It looks to me like it doesn't actually matter at the moment, because
anyplace where we apply DTK_M to a value that could be UNKNOWN_FIELD,
we'll immediately after that either return an error or replace the
tmask value with something else. So the lack of portability of this
construction hasn't mattered. But we should fix it in a way that won't
create time bombs for future code changes, and producing a zero mask
from a valid field type code would be a time bomb.

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