From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050328 Firefox/1.0.2 Fedora/1.0.2-3 Description of problem: With test2 place holders no longer seem to be functioning in perl cgi scripts using DBD-pg eg $cat='1'; #$cat=param('Category'); print $cat; print start_multipart_form (POST,'con_upd.pl'); $row=$dbh->prepare("SELECT type_code FROM lk_sort_of_contact WHERE description = ?"); $row->bind_param(1,$cat); $row->execute(); results in DBD::Pg::st execute failed: ERROR: syntax error at or near "$1" at character 1 DBD::Pg::st fetchrow_array failed: no statement executing Use of uninitialized value in print at /home/www/cgi-bin/con_by_role_sel2.pl line 22. Version-Release number of selected component (if applicable): perl-DBD-pg-1.40 Postgresql-8.0.1 How reproducible: Always Steps to Reproduce: See above Expected Results: The script should have run as before Additional info:
reverting to perl-DBD-Pg-1.31 from FC3 cures problem so definitely appears to a DBD-Pg problem
perl-DBD-1.41 from test3 has the same problem
This appears to be a problem with compiling DBD-Pg with gcc4. When built with gcc4, all versions of DBD-Pg seem to exhibit this breakage, and on the other hand, when built with compat-gcc-32, 1.41 seems to work just fine. Same story with 1.41_1. To build with compat-gcc-32: make %{?_smp_mflags} \ OPTIMIZE="`echo $RPM_OPT_FLAGS | sed s/mtune/mcpu/`" CC=gcc32
Ville, Could you check if the version compiled with gcc32 passes the test suite (today I don't have access to my FC4test3 system)? A couple of weeks ago I didn't successed in running them in a FC4test system: https://rt.cpan.org/NoAuth/Bug.html?id=12204
Yep, the version built with gcc32 looks good: t/00basic...........ok t/01connect.........ok 8/8# # Program Version # Perl 5.8.6 (linux) # DBD::Pg 1.41 # PostgreSQL (compiled) 80003 # PostgreSQL (target) 80003 # DBI 1.48 # DBI_DSN dbi:Pg:dbname=bugtest t/01connect.........ok t/01constants.......ok t/01setup...........ok t/02attribs.........ok t/03dbmethod........ok t/03smethod.........ok t/04misc............ok t/05arrays..........ok 15/17 skipped: Array support not implemented t/06bytea...........ok t/07copy............ok t/12placeholders....ok t/20savepoints......ok t/99_pod............skipped all skipped: Test::Pod 0.95 required for testing POD t/99cleanup.........ok All tests successful, 1 test and 15 subtests skipped. Files=15, Tests=411, 13 wallclock secs ( 4.05 cusr + 4.16 csys = 8.21 CPU)
Thanks! Forwarding this info to the DBD-Pg maintainers (CPAN #12204).
Let's see if jakub can find the cause of this problem. If we are running out of time, I'll build the gcc32 binary before May 23rd.
Can you please: 1) try gcc4 -O{0,1} as well 2) do a binary search between objects built with gcc4 -O2 and a compiler with which it works to see which exact *.o file matters 3) in the debugger, see which functions from that .o file are called and where the problem could possibly happen and ideally 4) distill a self-contained testcase from it ? The fact that something works with older compiler and does not with a newer one does not imply a compiler bug, it more often actually is an application bug.
DBD-Pg test suite and gcc v4 ---------------------------- 1) gcc -O1: test suite fails 2) gcc -O0: test suite OK Only passes the test suite if compiled with -O0 make %{?_smp_mflags} \ OPTIMIZE="`echo $RPM_OPT_FLAGS | sed s/-O2/-O0/`" Note: tested with DBD-Pg-1.41 and DBD-Pg-1.41_2.
Confirmed, compiling with -O0 makes the test suite pass (which implies also that the placeholder problem is fixed by that).
We are running out of time. I'll ship it with -O0 for now and keep the bug open for a real fix later.
http://people.redhat.com/wtogami/temp/perl-DBD-Pg/ Hmm... this is a bit odd. These changes happened when I did s/O2/OO/. Probably not a problem? [BAD] [perl-DBD-Pg] usr/lib/perl5/vendor_perl/5.8.6/i386-linux-thread-multi/auto/DBD/Pg/Pg.so lost -DFORTIFY_SOURCE on i386 [BAD] [perl-DBD-Pg] usr/lib/perl5/vendor_perl/5.8.6/ia64-linux-thread-multi/auto/DBD/Pg/Pg.so lost -DFORTIFY_SOURCE on ia64 [BAD] [perl-DBD-Pg] usr/lib/perl5/vendor_perl/5.8.6/ppc-linux-thread-multi/auto/DBD/Pg/Pg.so lost -DFORTIFY_SOURCE on ppc [BAD] [perl-DBD-Pg] usr/lib/perl5/vendor_perl/5.8.6/s390-linux-thread-multi/auto/DBD/Pg/Pg.so lost -DFORTIFY_SOURCE on s390 [BAD] [perl-DBD-Pg] usr/lib64/perl5/vendor_perl/5.8.6/ppc64-linux-thread-multi/auto/DBD/Pg/Pg.so lost -DFORTIFY_SOURCE on ppc64 [BAD] [perl-DBD-Pg] usr/lib64/perl5/vendor_perl/5.8.6/s390x-linux-thread-multi/auto/DBD/Pg/Pg.so lost -DFORTIFY_SOURCE on s390x [BAD] [perl-DBD-Pg] usr/lib64/perl5/vendor_perl/5.8.6/x86_64-linux-thread-multi/auto/DBD/Pg/Pg.so lost -DFORTIFY_SOURCE on x86_64 [VERIFY] [perl-DBD-Pg] RPM.requires <tt>libc.so.6(GLIBC_2.3.4)(64bit)</tt> disappearing on ppc64 s390x x86_64 [VERIFY] [perl-DBD-Pg] RPM.requires <tt>libc.so.6(GLIBC_2.3.4)</tt> disappearing on i386 ppc s390 [VERIFY] [perl-DBD-Pg] RPM.requires <tt>libc.so.6.1(GLIBC_2.3.4)(64bit)</tt> disappearing on ia64
1.42 is out, fixes a number of problems but unfortunately not this one; s/O2/O0/ is still needed with it and gcc-4.0.0-8.
We can issue an FC4 update later when a real solution is found. Someone needs to do Jakub's suggestion in Comment #8.
make %{?_smp_mflags} \ OPTIMIZE="`echo $RPM_OPT_FLAGS | sed s/-Wp,-D_FORTIFY_SOURCE=2//`" The test suite also runs without problems if I just remove the FORTIFY_SOURCE define. Does this implies a DPD::Pg bug (some kind of buffer overflow)? Where can I get more information about FORTIFY_SOURCE?
sed s/-Wp,-D_FORTIFY_SOURCE=2/-Wp,-D_FORTIFY_SOURCE=1/ The test suite also runs without problems with _FORTIFY_SOURCE=1.
(Removing personal Cc, I receive this traffic through the list...)
<arjan> warren: =2 and =1 aren't entirely identical <arjan> =2 is changing some subtle behavior that I would consider a bug <arjan> but the standards don't <arjan> example <arjan> struct foo { char buf[20]; int a;}; <arjan> strcpy(foo->buf, "21 character long string...."); <arjan> that is allowed with =1 <arjan> but not with =2 Most likely this is FORTIFY_SOURCE=2 breaking on some bad code.
there are one or two other subtle changes for example printf to %n formats needs to come from read only memory (or gettext)
The gcc4/glibc/_FORTIFY_SOURCE buffer overflow protection appears to documented in this Jakub's email: [PATCH] Object size checking to prevent (some) buffer overflows http://gcc.gnu.org/ml/gcc-patches/2004-09/msg02055.html --------------------------------------------------------------- ... The intended use in glibc is that by default no protection is done, when the above GCC 4.0+ and -D_FORTIFY_SOURCE=1 is used at optimization level 1 and above, security measures that shouldn't change behaviour of conforming programs are taken. With -D_FORTIFY_SOURCE=2 some more checking is added, but some conforming programs might fail. ... --------------------------------------------------------------- The last sentence in the above paragraph worries me. How many false positives were found? Does anyone know examples of some false positives?
AFAIK very few instances were detected where =2 caused trouble, and those cases were considered to be poor coding to begin with.
Until know we have found one case, the example warren quoted me on above.
It seems to be the dbd_st_prepare_statement routine in dbdimp.c that matters. If this routine is built with -D_FORTIFY_SOURCE=2, make test fails, with -D_FORTIFY_SOURCE=1 succeeds. It is irrelevant whether the other .o files are built with fortify 1 or 2, and similarly for other routines in dbdimp.c. But looking at the assembly difference, it is really minimal: --- /tmp/1 2005-05-24 11:29:49.000000000 +0200 +++ /tmp/2 2005-05-24 11:39:23.000000000 +0200 @@ -50,11 +50,13 @@ dbd_st_prepare_statement: # basic block 1 movl -32(%ebp), %edi # imp_dbh, movl 112(%edi), %eax # <variable>.prepare_number, <variable>.prepare_number - movl %eax, 8(%esp) # <variable>.prepare_number, + movl %eax, 16(%esp) # <variable>.prepare_number, leal .LC149@GOTOFF(%ebx), %eax #, tmp109 - movl %eax, 4(%esp) # tmp109, + movl %eax, 12(%esp) # tmp109, + movl $-1, 8(%esp) #, + movl $1, 4(%esp) #, movl %edx, (%esp) # D.18444, - call sprintf@PLT # + call __sprintf_chk@PLT # movl 12(%ebp), %eax # imp_sth, movl 128(%eax), %edx # <variable>.prepare_name, temp.762 cld @@ -282,12 +284,14 @@ dbd_st_prepare_statement: ret .L1348: # basic block 27 - movl %eax, 12(%esp) # D.18478, - movl %edi, 8(%esp) # statement, + movl %eax, 20(%esp) # D.18478, + movl %edi, 16(%esp) # statement, leal .LC152@GOTOFF(%ebx), %eax #, tmp140 - movl %eax, 4(%esp) # tmp140, + movl %eax, 12(%esp) # tmp140, + movl $-1, 8(%esp) #, + movl $1, 4(%esp) #, movl %edi, (%esp) # statement, - call sprintf@PLT # + call __sprintf_chk@PLT # .L1303: # basic block 28 movl 12(%esi), %esi # <variable>.nextseg, currseg.772 and I don't see how that would change things (__sprintf_chk (buf, 1, -1, ...) works like sprintf, except %n from writable memory is refused (but there is no %n in this case and format strings are in read-only memory) and -1 length means no length limit). 20(%esp) is still in the area used for outgoing arguments, %ebp - %esp is 72 bytes and the lowest variable is at -48(%ebp). The code quality of that routine is horrible, look e.g. at this junk line: imp_sth->prepare_name[strlen(imp_sth->prepare_name)]='\0';
Ok, finally see the bug. I was mislead by looking at second sprintf in the routine, but that one is optimized out and there is a third one, which is bogus: if (currseg->placeholder) { sprintf(statement, "%s$%d", statement, currseg->placeholder); } You simply can't do this in C, see ISO C99, 7.19.6.6: If copying takes place between objects that overlap, the behavior is undefined. I certainly don't have time to rewrite this whole junk, so here is just a quick fix for this exact case, though someone please rewrite this, ideally from scratch.
Created attachment 114790 [details] Eliminates the problem exposed by _FORTIFY_SOURCE=2
Created attachment 114791 [details] perl-DBD-Pg specfile patch Could someone else review/comment the dbdimp.c patch?
That's complete overkill. If you look at the (very ugly) loop counting the execsize, there is room in statement buffer for that sprintf, so the patch I apparently forgot to provide is enough if all you care about is to make it work. --- dbdimp.c.jj 2005-04-06 16:40:20.000000000 -0400 +++ dbdimp.c 2005-05-24 07:40:21.000000000 -0400 @@ -1243,7 +1243,7 @@ int dbd_st_prepare_statement (sth, imp_s for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { strcat(statement, currseg->segment); if (currseg->placeholder) { - sprintf(statement, "%s$%d", statement, currseg->placeholder); + sprintf(strchr(statement, '\0'), "$%d", currseg->placeholder); } } Calling pow in a loop is insane though: /* The parameter itself: dollar sign plus digit(s) */ for (x=1; x<7; x++) { if (currseg->placeholder < pow(10,x)) break; } if (x>=7) croak("Too many placeholders!"); execsize += x+1; Guess e.g. #include <limits.h> and #define DBD_STRINGIFY_1(x) #x #define DBD_STRINGIFY(x) DBD_STRINGIFY_1 (x) execsize += strlen (DBD_STRINGIFY (INT_MAX)) + 2; would be far cheaper (well, assuming CHAR_BIT 8 one can write execsize += sizeof (int) * 3 + 2; as well). The strlen will be optimized out by the compiler (unlike the expensive pow calls).
Jakub, Thanks for the explanation and the new patch.
The sprintf statement in line 1677 should also be patched.
I'm not building anything until Ville (or somebody else from fpdl) reviews and approves it.
dbdimp.c has just been patched upstream: http://gborg.postgresql.org/project/dbdpg/cvs/diff.php/dbdpg/dbdimp.c?r1=1.134&r2=1.136&ty=u
That exact patch is desired against the current SRPM or should we wait for the next upstream release?
It appears to be OK but I will test it today. How many days can the update be postponed? Until June 6th (-1) ?
Anytime really. The FC4 version works fine, so we can afford to wait until the upstream release, and verify that it works properly.
Created attachment 116072 [details] Specfile patch: update to version 1.43 This new upstream version builds and passes its test suite using the standard _FORTIFY_SOURCE value.