Ubuntu

generates-bad-code regression

Reported by Zooko Wilcox-O'Hearn on 2009-10-26
16
This bug affects 2 people
Affects Status Importance Assigned to Milestone
MinGW
Unknown
Unknown
Tahoe-LAFS
Fix Released
Unknown
binutils
Fix Released
Medium
pycryptopp
Fix Released
Unknown
binutils (Fedora)
In Progress
Unknown
binutils (Ubuntu)
Medium
Matthias Klose
Karmic
Medium
Matthias Klose
libcrypto++ (Ubuntu)
Undecided
Unassigned
Karmic
Undecided
Unassigned

Bug Description

g++-4.4 4.4.1-3ubuntu3 was used to build libcrypto++8. The current g++-4.4 in Karmic is g++-4.4 4.4.1-4ubuntu8. When 4.4.1-4ubuntu8 is used to build libcrypto++8 then the libcrypto++8 self-tests hang during the SHA validation. It appears that there has been a regression between g++ 4.4.1-3ubuntu3 and -4ubuntu8 such that the new one generates bad code.

In addition to the libcrypto++8 self-tests hanging during SHA validation, if you then build python-pycryptopp using the libcrypto++8 lib that was built by g++ 4.4.1-4ubuntu8, then the python-pycryptopp library gets segfaults when it runs its self-tests. Here is the pycryptopp upstream buildbot showing segfaults on karmic, along with a valgrind report:

http://allmydata.org/buildbot-pycryptopp/waterfall
http://allmydata.org/buildbot-pycryptopp/builders/linux-amd64-ubuntu-karmic-yukyuk/builds/11/steps/test/logs/valgrind

Colin Watson (cjwatson) on 2009-10-26
affects: ubuntu → gcc-4.4 (Ubuntu)
tags: added: regression-potential
Zooko Wilcox-O'Hearn (zooko) wrote :

Hold on, the reason that I stated that g++ 4.4.1-3ubuntu3 built good code is as follows:

1. The resulting libcrypto++8 is in Karmic, so it must have passed its self-test which happens automatically when you build it, right?

2. If you link pycryptopp to the resulting libcrypto++8 which is in Karmic, the pycryptopp passes all of its self-tests including under valgrind: here is the builder which does this: http://allmydata.org/buildbot-pycryptopp/builders/linux-amd64-ubuntu-karmic-yukyuk-syslib (in contrast, here is the builder which attempts to build the same version of pycryptopp but compile Crypto++ itself from source instead of using the libcrypto++8 which is included in Karmic: http://allmydata.org/buildbot-pycryptopp/builders/linux-amd64-ubuntu-karmic-yukyuk . That one fails.)

However, I just now finished getting a pbuilder chroot configured with g++-4.4 4.4.1-3ubuntu3 and when I pbuild libcrypto++8 then it hangs in its SHA self-test!

So, how did libcrypto++8 as built by g++ 4.4.1-3ubuntu3 pass its self tests? Or *did* it pass its self-tests before it went into Karmic?

Zooko Wilcox-O'Hearn (zooko) wrote :

Okay it looks like this is an issue in binutils, not in g++. As stated in the previous comment I installed the version of g++-4.4 and all of its many dependencies that had been used back in 2009-09-18 to build libcrypto++8, and the build still fails. Then I changed binutils from the current (2.20-0ubuntu1) to the 2009-09-18 vintage: 2.19.91.20090910-0ubuntu1 . Now the build passes!

So to reiterate the only change I made in my pbuilder root was to install binutils 2.19.91.20090910-0ubuntu1, replacing binutils 2.20-0ubuntu1, and this made the pbuild of libcrypto++8 go from hanging during the SHA validation to passing.

Zooko Wilcox-O'Hearn (zooko) wrote :

Okay I just did this:

Preparing to replace binutils 2.19.91.20090910-0ubuntu1 (using binutils_2.19.91.20091014-0ubuntu1_amd64.deb) ...

And am now pbuilding again. Now we'll see if the regression happened between 2.19.91.20090910-0ubuntu1 and 2.19.91.20091014-0ubuntu1 or between 2.19.91.20091014-0ubuntu1 and 2.20-0ubuntu1.

Zooko Wilcox-O'Hearn (zooko) wrote :

Okay, the result is that 2.19.91.20091014-0ubuntu1 also misbuilds libcrypto++8.

Next I will try the point halfway between the last known good version (2.19.91.20090910-0ubuntu1) and the earliest known bad version (2.19.91.20091014-0ubuntu1). That would be... 2.19.91.20091003-0ubuntu1 . And while it is building I am going to take a nap.

Zooko Wilcox-O'Hearn (zooko) wrote :

Oh by the way this is all on amd64.

Zooko Wilcox-O'Hearn (zooko) wrote :

Okay, 2.19.91.20091003-0ubuntu1 passed. So the regression must be between 2.19.91.20091003-0ubuntu1 and 2.19.91.20091014-0ubuntu1 . Next I will try 2.19.91.20091005-0ubuntu2 .

Zooko Wilcox-O'Hearn (zooko) wrote :

okay 2.19.91.20091005-0ubuntu2 built it correctly. We must be getting close to the regressing version. Next I'll try 2.19.91.20091006-0ubuntu1 .

Zooko Wilcox-O'Hearn (zooko) wrote :

Preparing to replace binutils 2.19.91.20091005-0ubuntu2 (using binutils_2.19.91.20091006-0ubuntu1_amd64.deb) ...

Okay it passed. This implies that the upgrade from 2.19.91.20091006-0ubuntu1 to 2.19.91.20091014-0ubuntu1 introduced this regression. I'll just double-check that 2.19.91.20091014-0ubuntu1 still builds libcrypto++8 wrong...

Zooko Wilcox-O'Hearn (zooko) wrote :

Preparing to replace binutils 2.19.91.20091006-0ubuntu1 (using binutils_2.19.91.20091014-0ubuntu1_amd64.deb) ...

Yep. 2.19.91.20091014-0ubuntu1 builds a libcrypto++8 that hangs during the SHA validation in its self-tests. What's the next step? How can we get a diff from 2.19.91.20091006-0ubuntu1 to 2.19.91.20091014-0ubuntu1?

Kees Cook (kees) on 2009-10-26
affects: gcc-4.4 (Ubuntu) → binutils (Ubuntu)
Zooko Wilcox-O'Hearn (zooko) wrote :

note that libcrypto++ does include a fair bit of asm code for amd64.

Zooko Wilcox-O'Hearn (zooko) wrote :
Download full text (18.9 KiB)

okay, i'm not at all familiar with binutils, but just studying which files were changed by this patch and excluding build, packaging, translation, and non-amd64 arches, i am left with these three patches:

--- binutils-2.19.91.20091006/gas/read.c 2009-09-15 13:27:21.000000000 +0100
+++ binutils-2.19.91.20091014/gas/read.c 2009-10-07 09:35:59.000000000 +0100
@@ -3911,7 +3911,7 @@
   if (*input_line_pointer == ',')
     {
       ++input_line_pointer;
- expression_and_evaluate (&exp);
+ expression (&exp);
     }
   switch (exp.X_op)
     {
--- binutils-2.19.91.20091006/gas/symbols.c 2009-09-27 10:57:37.000000000 +0100
+++ binutils-2.19.91.20091014/gas/symbols.c 2009-10-07 09:36:44.000000000 +0100
@@ -1514,10 +1514,7 @@
      }
  }

- /* Never change a defined symbol. */
- if (symbolP->bsym->section == undefined_section
- || symbolP->bsym->section == expr_section)
- *symbolPP = symbolP;
+ *symbolPP = symbolP;
       *valueP = expr.X_add_number;
       *segP = symbolP->bsym->section;
       *fragPP = symbolP->sy_frag;
--- binutils-2.19.91.20091006/ld/scripttempl/elf.sc 2009-09-29 10:51:03.000000000 +0100
+++ binutils-2.19.91.20091014/ld/scripttempl/elf.sc 2009-10-09 06:43:07.000000000 +0100
@@ -110,42 +110,42 @@
   DATA_SEGMENT_RELRO_END=". = DATA_SEGMENT_RELRO_END (${SEPARATE_GOTPLT-0}, .);"
 fi
 if test -z "${INITIAL_READONLY_SECTIONS}${CREATE_SHLIB}"; then
- INITIAL_READONLY_SECTIONS=".interp : { *(.interp) }"
+ INITIAL_READONLY_SECTIONS=".interp ${RELOCATING-0} : { *(.interp) }"
 fi
 if test -z "$PLT"; then
- IPLT=".iplt : { *(.iplt) }"
- PLT=".plt : { *(.plt)${IREL_IN_PLT+ *(.iplt)} }
+ IPLT=".iplt ${RELOCATING-0} : { *(.iplt) }"
+ PLT=".plt ${RELOCATING-0} : { *(.plt)${IREL_IN_PLT+ *(.iplt)} }
   ${IREL_IN_PLT-$IPLT}"
 fi
 test -n "${DATA_PLT-${BSS_PLT-text}}" && TEXT_PLT=yes
 if test -z "$GOT"; then
   if test -z "$SEPARATE_GOTPLT"; then
- GOT=".got : { *(.got.plt) *(.igot.plt) *(.got) *(.igot) }"
+ GOT=".got ${RELOCATING-0} : { *(.got.plt) *(.igot.plt) *(.got) *(.igot) }"
   else
- GOT=".got : { *(.got) *(.igot) }"
- GOTPLT=".got.plt : { *(.got.plt) *(.igot.plt) }"
+ GOT=".got ${RELOCATING-0} : { *(.got) *(.igot) }"
+ GOTPLT=".got.plt ${RELOCATING-0} : { *(.got.plt) *(.igot.plt) }"
   fi
 fi
-REL_IFUNC=".rel.ifunc : { *(.rel.ifunc) }"
-RELA_IFUNC=".rela.ifunc : { *(.rela.ifunc) }"
-REL_IPLT=".rel.iplt :
+REL_IFUNC=".rel.ifunc ${RELOCATING-0} : { *(.rel.ifunc) }"
+RELA_IFUNC=".rela.ifunc ${RELOCATING-0} : { *(.rela.ifunc) }"
+REL_IPLT=".rel.iplt ${RELOCATING-0} :
     {
       ${RELOCATING+${CREATE_SHLIB-PROVIDE_HIDDEN (${USER_LABEL_PREFIX}__rel_iplt_start = .);}}
       *(.rel.iplt)
       ${RELOCATING+${CREATE_SHLIB-PROVIDE_HIDDEN (${USER_LABEL_PREFIX}__rel_iplt_end = .);}}
     }"
-RELA_IPLT=".rela.iplt :
+RELA_IPLT=".rela.iplt ${RELOCATING-0} :
     {
       ${RELOCATING+${CREATE_SHLIB-PROVIDE_HIDDEN (${USER_LABEL_PREFIX}__rela_iplt_start = .);}}
       *(.rela.iplt)
       ${RELOCATING+${CREATE_SHLIB-PROVIDE_HIDDEN (${USER_LABEL_PREFIX}__rela_iplt_end = .);}...

Zooko Wilcox-O'Hearn (zooko) wrote :

I posted to the Crypto++ mailing list asking for help and warning them not to upgrade to Karmic:
http://groups.google.com/group/cryptopp-users/browse_thread/thread/36ceee8e8f500fd3

Zooko Wilcox-O'Hearn (zooko) wrote :

Wei Dai, the author of Crypto++, has kindly volunteered to investigate: http://groups.google.com/group/cryptopp-users/msg/e49e6e8a0adf4630

He asks:

"How can we find out who submitted the the binutil patch and contact him or her?"

Zooko Wilcox-O'Hearn (zooko) wrote :

Please also see my response where I say that I don't know where the patches come from and where I speculate about blacklisting this particular version of GNU assembler:

http://groups.google.com/group/cryptopp-users/msg/fef83f2a64c797cc

On 27.10.2009 16:44, Zooko O'Whielacronx wrote:
> Wei Dai, the author of Crypto++, has kindly volunteered to investigate:
> http://groups.google.com/group/cryptopp-users/msg/e49e6e8a0adf4630

did you identify the patch causing the failure?

> He asks:
>
> "How can we find out who submitted the the binutil patch and contact him
> or her?"

<email address hidden>, but it would be good to identify the relevant checkin
first. See the ChangeLog files in the various subdirectories.

The patch to fix PR gas/10704 causes sha.cpp assembled in a way that causes the
sha-256 test not to terminate. Undoing the patch runs the test fine. Replacing
the sha.o with one assembled with gas having this patch reverted lets the test pass.

Created attachment 4329
diff of disassembled files

Created attachment 4330
sha .s and .o files

Changed in binutils (Ubuntu):
importance: Undecided → Medium
status: New → Confirmed

Simpler testcase:

 .text
 .intel_syntax noprefix
 .space 5480
 .byte 0,0,0
0:
 .space 1620
 .byte 0
1:
 .space 2468
 nop
 jne 1b
 .space 52
 nop
 jl 0b

Without the .intel_syntax this assembles OK.

Subject: Bug 10856

CVSROOT: /cvs/src
Module name: src
Changes by: <email address hidden> 2009-10-28 08:21:45

Modified files:
 gas : ChangeLog expr.c
 gas/testsuite : ChangeLog
 gas/testsuite/gas/i386: intelpic.d

Log message:
 PR gas/10856
 * expr.c (resolve_expression): Only add "left" value to O_symbol
 expression when the symbol is undefined and different from the
 original symbol. Simplify negative logic.

 * gas/i386/intelpic.d: Correct.

Patches:
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/ChangeLog.diff?cvsroot=src&r1=1.3999&r2=1.4000
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/expr.c.diff?cvsroot=src&r1=1.78&r2=1.79
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1573&r2=1.1574
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/i386/intelpic.d.diff?cvsroot=src&r1=1.3&r2=1.4

Subject: Bug 10856

CVSROOT: /cvs/src
Module name: src
Branch: binutils-2_20-branch
Changes by: <email address hidden> 2009-10-28 08:23:49

Modified files:
 gas : ChangeLog expr.c

Log message:
 PR gas/10856
 * expr.c (resolve_expression): Only add "left" value to O_symbol
 expression when the symbol is undefined and different from the
 original symbol. Simplify negative logic.

Patches:
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/ChangeLog.diff?cvsroot=src&only_with_tag=binutils-2_20-branch&r1=1.3938.2.27&r2=1.3938.2.28
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/expr.c.diff?cvsroot=src&only_with_tag=binutils-2_20-branch&r1=1.77.2.1&r2=1.77.2.2

checked that the sha test passes, when sha.o is built with this patch.

Matthias Klose (doko) on 2009-10-28
Changed in binutils (Ubuntu Karmic):
status: Confirmed → In Progress
Changed in libcrypto++ (Ubuntu Karmic):
status: New → Invalid
Changed in binutils (Ubuntu Karmic):
milestone: none → karmic-updates
Martin Pitt (pitti) wrote :

I reviewed the package in -proposed. Will accept right after karmic release.

What is the test case here? libcrypto++ failing the tests (and build) in karmic final, and succeeding with the proposed update?

Changed in binutils (Ubuntu Karmic):
assignee: nobody → Matthias Klose (doko)
status: In Progress → Fix Committed
Martin Pitt (pitti) wrote :

Accepted binutils into karmic-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

tags: added: verification-needed
Changed in binutils:
status: Unknown → Fix Released
Matthias Klose (doko) wrote :

the updated package builds libcrypto++, same fix for lucid is in 2.20-1ubuntu1, now in lucid.

Martin Pitt (pitti) on 2009-10-30
tags: added: verification-done
removed: verification-needed
Changed in binutils (Ubuntu):
status: Fix Committed → Fix Released
milestone: karmic-updates → none
Changed in tahoe-lafs:
status: Unknown → New
Martin Pitt (pitti) wrote :

I built several packages with this binutils version (like udev, devicekit-disks, empathy, etc.), and they all work fine.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package binutils - 2.20-0ubuntu2

---------------
binutils (2.20-0ubuntu2) karmic-proposed; urgency=low

  * Fix PR gas/10856, wrong code with assembler files in intel syntax.
    Patch taken from the 2.20 branch. LP: #461303.

 -- Matthias Klose <email address hidden> Wed, 28 Oct 2009 09:46:50 +0100

Changed in binutils (Ubuntu Karmic):
status: Fix Committed → Fix Released

*** Bug 10906 has been marked as a duplicate of this bug. ***

Changed in tahoe-lafs:
status: New → Fix Released

Any indication of when this fix is likely to get into a release? The more OS
distributions as 2.20 goes into, the more problems this bug will cause.

This launchpad page tracks the increasing number of OS distributions and open
source projects that are being negatively affected by the bug in 2.20:

https://bugs.launchpad.net/fedora/+source/binutils/+bug/461303

Changed in binutils (Fedora):
status: Unknown → In Progress

Subject: Re: [2.20 regression] gas creates wrong code which results in a test failure in libcrypto++'s sha2 test

On Dec 13, 2009, at 6:55 PM, david-sarah at jacaranda dot org wrote:

>
> ------- Additional Comments From david-sarah at jacaranda dot org 2009-12-13 17:55 -------
> Any indication of when this fix is likely to get into a release? The more OS
> distributions as 2.20 goes into, the more problems this bug will cause.

I agree that this is annoying. What about a 2.20.1 release in January ?

Tristan.

Changed in pycryptopp:
status: Unknown → Fix Released

Hey folks, some distributions such as MinGW have still not patched their version of binutils or reverted
to binutils 2.19:

https://sourceforge.net/tracker/index.php?func=detail&aid=2913876&group_id=2435&atid=102435

This is still causing problems for my users:

http://tahoe-lafs.org/trac/tahoe-lafs/ticket/853

If you folks could go ahead and release a binutil 2.20.1 then that might stimulate distributors like
MinGW to upgrade to it and that would fix the problem for my users. Thanks!

Zooko Wilcox-O'Hearn (zooko) wrote :

Fixed in MingW's upgrade to binutils 2.20.51.20100613

Changed in mingw:
importance: Unknown → Undecided
status: Unknown → New
status: New → Fix Released
importance: Undecided → Unknown
status: Fix Released → Unknown
importance: Unknown → Undecided
status: Unknown → New
importance: Undecided → Unknown
status: New → Unknown
importance: Unknown → Undecided
status: Unknown → New
importance: Undecided → Unknown
status: New → Unknown

binutils 2.20.1, released 2010-03-03, does *not* have the ChangeLog entry from http://sourceware.org/bugzilla/show_bug.cgi?id=10856#c5 but *does* have the patch to expr.c. Weird. But I guess it is fixed in binutils 2.20.1.

Zooko Wilcox-O'Hearn (zooko) wrote :

binutils 2.20.1, released 2010-03-03, does *not* have the ChangeLog entry from http://sourceware.org/bugzilla/show_bug.cgi?id=10856#c5 but *does* have the patch to expr.c. Weird. But I guess it is fixed in binutils 2.20.1.

Changed in binutils:
importance: Unknown → Medium
Zooko Wilcox-O'Hearn (zooko) wrote :

Could a "project maintainer or bug supervisor" please change the Fedora bug status from "In Progress" to "Fix Released" or authorize me to do so?

Zooko Wilcox-O'Hearn (zooko) wrote :

Could a "project maintainer or bug supervisor" please change the MinGW bug status from "In Progress" to "Fix Released" or authorize me to do so?

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.