Bug 156840 - (gcc4 _FORTIFY_SOURCE=2) perl-DBD-pg Placeholders no longer functioning
Summary: (gcc4 _FORTIFY_SOURCE=2) perl-DBD-pg Placeholders no longer functioning
Alias: None
Product: Fedora
Classification: Fedora
Component: perl-DBD-Pg
Version: rawhide
Hardware: i386
OS: Linux
Target Milestone: ---
Assignee: Warren Togami
QA Contact:
Depends On:
Blocks: FC5Blocker FC4Update
TreeView+ depends on / blocked
Reported: 2005-05-04 16:47 UTC by Need Real Name
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2005-11-03 16:12:38 UTC
Type: ---

Attachments (Terms of Use)
Eliminates the problem exposed by _FORTIFY_SOURCE=2 (838 bytes, patch)
2005-05-24 18:24 UTC, Jose Pedro Oliveira
no flags Details | Diff
perl-DBD-Pg specfile patch (1.73 KB, patch)
2005-05-24 18:33 UTC, Jose Pedro Oliveira
no flags Details | Diff
Specfile patch: update to version 1.43 (1.20 KB, patch)
2005-06-28 17:41 UTC, Jose Pedro Oliveira
no flags Details | Diff

Description Need Real Name 2005-05-04 16:47:49 UTC
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


print $cat;
print start_multipart_form (POST,'con_upd.pl');
$row=$dbh->prepare("SELECT type_code FROM lk_sort_of_contact WHERE description = ?");

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:

Steps to Reproduce:
See above

Expected Results:  The script should have run as before

Additional info:

Comment 1 Need Real Name 2005-05-05 11:52:39 UTC
reverting to perl-DBD-Pg-1.31 from FC3 cures problem so definitely appears to a
DBD-Pg problem

Comment 2 Need Real Name 2005-05-16 12:55:30 UTC
perl-DBD-1.41 from test3 has the same problem

Comment 3 Ville Skyttä 2005-05-17 16:49:24 UTC
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

Comment 4 Jose Pedro Oliveira 2005-05-17 17:30:24 UTC

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:

Comment 5 Ville Skyttä 2005-05-17 18:46:17 UTC
Yep, the version built with gcc32 looks good:

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
        15/17 skipped: Array support not implemented
        all skipped: Test::Pod 0.95 required for testing POD
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)

Comment 6 Jose Pedro Oliveira 2005-05-17 22:56:19 UTC

Forwarding this info to the DBD-Pg maintainers (CPAN #12204).

Comment 7 Warren Togami 2005-05-17 23:19:14 UTC
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.

Comment 8 Jakub Jelinek 2005-05-18 18:12:10 UTC
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

Comment 9 Jose Pedro Oliveira 2005-05-19 14:21:23 UTC
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.

Comment 10 Ville Skyttä 2005-05-19 16:20:54 UTC
Confirmed, compiling with -O0 makes the test suite pass (which implies also that
the placeholder problem is fixed by that).

Comment 11 Warren Togami 2005-05-19 20:51:50 UTC
We are running out of time.  I'll ship it with -O0 for now and keep the bug open
for a real fix later.

Comment 12 Warren Togami 2005-05-19 22:30:36 UTC
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
[BAD] [perl-DBD-Pg]
usr/lib/perl5/vendor_perl/5.8.6/ia64-linux-thread-multi/auto/DBD/Pg/Pg.so lost
[BAD] [perl-DBD-Pg]
usr/lib/perl5/vendor_perl/5.8.6/ppc-linux-thread-multi/auto/DBD/Pg/Pg.so lost
[BAD] [perl-DBD-Pg]
usr/lib/perl5/vendor_perl/5.8.6/s390-linux-thread-multi/auto/DBD/Pg/Pg.so lost
[BAD] [perl-DBD-Pg]
lost -DFORTIFY_SOURCE on ppc64 
[BAD] [perl-DBD-Pg]
lost -DFORTIFY_SOURCE on s390x 
[BAD] [perl-DBD-Pg]
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 

Comment 13 Ville Skyttä 2005-05-22 11:51:04 UTC
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.

Comment 14 Warren Togami 2005-05-22 12:09:06 UTC
We can issue an FC4 update later when a real solution is found.  Someone needs
to do Jakub's suggestion in Comment #8.

Comment 15 Jose Pedro Oliveira 2005-05-23 19:09:25 UTC
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?

Comment 16 Jose Pedro Oliveira 2005-05-23 19:38:51 UTC

The test suite also runs without problems with _FORTIFY_SOURCE=1.

Comment 17 Ville Skyttä 2005-05-23 20:03:27 UTC
(Removing personal Cc, I receive this traffic through the list...)

Comment 18 Warren Togami 2005-05-23 20:05:41 UTC
<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.

Comment 19 Arjan van de Ven 2005-05-23 20:09:56 UTC
there are one or two other subtle changes
for example printf to %n formats needs to come from read only memory (or gettext)

Comment 20 Jose Pedro Oliveira 2005-05-24 00:50:21 UTC
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
  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?

Comment 21 Warren Togami 2005-05-24 01:15:58 UTC
AFAIK very few instances were detected where =2 caused trouble, and those cases
were considered to be poor coding to begin with.

Comment 22 Arjan van de Ven 2005-05-24 07:42:29 UTC
Until know we have found one case, the example warren quoted me on above.

Comment 23 Jakub Jelinek 2005-05-24 10:05:07 UTC
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,
-       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
@@ -282,12 +284,14 @@ dbd_st_prepare_statement:
        # 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       #
        # 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:

Comment 24 Jakub Jelinek 2005-05-24 11:42:29 UTC
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

                if (currseg->placeholder) {
                        sprintf(statement, "%s$%d", statement,

You simply can't do this in C, see ISO C99,
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.

Comment 25 Jose Pedro Oliveira 2005-05-24 18:24:52 UTC
Created attachment 114790 [details]
Eliminates the problem exposed by _FORTIFY_SOURCE=2

Comment 26 Jose Pedro Oliveira 2005-05-24 18:33:36 UTC
Created attachment 114791 [details]
perl-DBD-Pg specfile patch

Could someone else review/comment the dbdimp.c patch?

Comment 27 Jakub Jelinek 2005-05-24 18:49:46 UTC
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,
+                       sprintf(strchr(statement, '\0'), "$%d",

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))
if (x>=7)
        croak("Too many placeholders!");
execsize += x+1;
Guess e.g. #include <limits.h> and
#define DBD_STRINGIFY_1(x) #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).

Comment 28 Jose Pedro Oliveira 2005-05-24 21:41:35 UTC

Thanks for the explanation and the new patch.

Comment 29 Jose Pedro Oliveira 2005-05-24 21:57:14 UTC
The sprintf statement in line 1677 should also be patched.

Comment 30 Warren Togami 2005-05-24 22:00:51 UTC
I'm not building anything until Ville (or somebody else from fpdl) reviews and
approves it.

Comment 31 Jose Pedro Oliveira 2005-05-31 02:04:03 UTC
dbdimp.c has just been patched upstream:


Comment 32 Warren Togami 2005-05-31 02:06:38 UTC
That exact patch is desired against the current SRPM or should we wait for the
next upstream release?

Comment 33 Jose Pedro Oliveira 2005-05-31 02:15:33 UTC
It appears to be OK but I will test it today.

How many days can the update be postponed? Until June 6th (-1) ?

Comment 34 Warren Togami 2005-05-31 02:18:34 UTC
Anytime really.  The FC4 version works fine, so we can afford to wait until the
upstream release, and verify that it works properly.

Comment 35 Jose Pedro Oliveira 2005-06-28 17:41:46 UTC
Created attachment 116072 [details]
Specfile patch: update to version 1.43

This new upstream version builds and passes its test suite using the standard

Note You need to log in before you can comment on or make changes to this bug.