Обсуждение: clang -fsanitize=undefined error in ecpg

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?

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 inputselect '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
turnedinto 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