Bug 156840 - (gcc4 _FORTIFY_SOURCE=2) perl-DBD-pg Placeholders no longer functioning
(gcc4 _FORTIFY_SOURCE=2) perl-DBD-pg Placeholders no longer functioning
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: perl-DBD-Pg (Show other bugs)
rawhide
i386 Linux
medium Severity high
: ---
: ---
Assigned To: Warren Togami
:
Depends On:
Blocks: FC5Blocker FC4Update
  Show dependency treegraph
 
Reported: 2005-05-04 12:47 EDT by Need Real Name
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-11-03 11:12:38 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


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

  None (edit)
Description Need Real Name 2005-05-04 12:47:49 EDT
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:
Comment 1 Need Real Name 2005-05-05 07:52:39 EDT
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 08:55:30 EDT
perl-DBD-1.41 from test3 has the same problem
Comment 3 Ville Skyttä 2005-05-17 12:49:24 EDT
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 13:30:24 EDT
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

Comment 5 Ville Skyttä 2005-05-17 14:46:17 EDT
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)
Comment 6 Jose Pedro Oliveira 2005-05-17 18:56:19 EDT
Thanks!

Forwarding this info to the DBD-Pg maintainers (CPAN #12204).
Comment 7 Warren Togami 2005-05-17 19:19:14 EDT
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 14:12:10 EDT
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.
Comment 9 Jose Pedro Oliveira 2005-05-19 10:21:23 EDT
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 12:20:54 EDT
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 16:51:50 EDT
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 18:30:36 EDT
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 
Comment 13 Ville Skyttä 2005-05-22 07:51:04 EDT
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 08:09:06 EDT
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 15:09:25 EDT
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 15:38:51 EDT
sed s/-Wp,-D_FORTIFY_SOURCE=2/-Wp,-D_FORTIFY_SOURCE=1/

The test suite also runs without problems with _FORTIFY_SOURCE=1.
Comment 17 Ville Skyttä 2005-05-23 16:03:27 EDT
(Removing personal Cc, I receive this traffic through the list...)
Comment 18 Warren Togami 2005-05-23 16:05:41 EDT
<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 16:09:56 EDT
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-23 20:50:21 EDT
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?
Comment 21 Warren Togami 2005-05-23 21:15:58 EDT
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 03:42:29 EDT
Until know we have found one case, the example warren quoted me on above.
Comment 23 Jakub Jelinek 2005-05-24 06:05:07 EDT
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';
Comment 24 Jakub Jelinek 2005-05-24 07:42:29 EDT
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.
Comment 25 Jose Pedro Oliveira 2005-05-24 14:24:52 EDT
Created attachment 114790 [details]
Eliminates the problem exposed by _FORTIFY_SOURCE=2
Comment 26 Jose Pedro Oliveira 2005-05-24 14:33:36 EDT
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 14:49:46 EDT
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).
Comment 28 Jose Pedro Oliveira 2005-05-24 17:41:35 EDT
Jakub,

Thanks for the explanation and the new patch.
Comment 29 Jose Pedro Oliveira 2005-05-24 17:57:14 EDT
The sprintf statement in line 1677 should also be patched.
Comment 30 Warren Togami 2005-05-24 18:00:51 EDT
I'm not building anything until Ville (or somebody else from fpdl) reviews and
approves it.
Comment 31 Jose Pedro Oliveira 2005-05-30 22:04:03 EDT
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

Comment 32 Warren Togami 2005-05-30 22:06:38 EDT
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-30 22:15:33 EDT
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-30 22:18:34 EDT
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 13:41:46 EDT
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.

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