Intrepid gcc -O2 breaks string appending with sprintf(), due to fortify source patch

Bug #305901 reported by Anders Kaseorg
20
This bug affects 2 people
Affects Status Importance Assigned to Milestone
GLibC
Confirmed
Medium
4g8 (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
abiword (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
asterisk (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
atomicparsley (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
audacious-plugins (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
barnowl (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
billard-gl (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
binutils (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
blender (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
ctn (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
gcc-4.3 (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
glibc (Ubuntu)
Fix Released
High
Kees Cook
Intrepid
Fix Released
High
Kees Cook
Jaunty
Fix Released
High
Kees Cook
hypermail (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
mpeg4ip (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
nagios-plugins (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
owl (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned
xmcd (Ubuntu)
Invalid
Undecided
Unassigned
Intrepid
Invalid
Undecided
Unassigned
Jaunty
Invalid
Undecided
Unassigned

Bug Description

Binary package hint: gcc-4.3

In Hardy and previous releases, one could use statements such as
  sprintf(buf, "%s %s%d", buf, foo, bar);
to append formatted text to a buffer buf. Intrepid’s gcc-4.3, which has fortify source turned on by default when compiling with -O2, breaks this pattern. This introduced mysterious bugs into an application I was compiling (the BarnOwl IM client).

Test case: gcc -O2 sprintf-test.c -o sprintf-test
<http://web.mit.edu/andersk/Public/sprintf-test.c>:
  #include <stdio.h>
  char buf[80] = "not ";
  int main()
  {
      sprintf(buf, "%sfail", buf);
      puts(buf);
      return 0;
  }
This outputs "not fail" in Hardy, and "fail" in Intrepid.

The assembly output shows that the bug has been introduced by replacing the sprintf(buf, "%sfail", buf) call with __sprintf_chk(buf, 1, 80, "%sfail", buf). A workaround is to disable fortify source (gcc -U_FORTIFY_SOURCE).

One might argue that this usage of sprintf() is questionable. I had been under the impression that it is valid, and found many web pages that agree with me, though I was not able to find an authoritative statement either way citing the C specification. I decided to investigate how common this pattern is in real source code.

You can search a source file for instances of it with this regex:
  pcregrep -M 'sprintf\s*\(\s*([^,]*)\s*,\s*"%s[^"]*"\s*,\s*\1\s*,'

To determine how common the pattern is, I wrote a script to track down instances using Google Code Search, and found 2888 matches:
  <http://web.mit.edu/andersk/Public/sprintf-results>
(For the curious: the script uses a variant of the regex above. I had to use a binary search to emulate backreferences, which aren’t supported by Code Search, so the script makes 46188 queries and takes a rather long time to run. The source is available at <http://web.mit.edu/andersk/Public/sprintf-codesearch.py>.)

My conclusion is that, whether or not this pattern is technically allowed by the C specification, it is common enough that the compiler should be fixed, if that is at all possible.

Revision history for this message
Anders Kaseorg (andersk) wrote :

C99 (at least the draft that’s available online) actually defines this code as invalid.

  #include <stdio.h>
  int sprintf(char * restrict s, const char * restrict format, ...);
“The sprintf function is equivalent to fprintf, except that the output is written into an array (specified by the argument s) rather than to a stream. A null character is written at the end of the characters written; it is not counted as part of the returned value. If copying takes place between objects that overlap, the behavior is undefined.”

So I guess the real answer is to fix the affected source. It might be nice to know if any software in Ubuntu is affected.

Changed in gcc-4.3:
status: New → Invalid
Anders Kaseorg (andersk)
description: updated
Revision history for this message
Anders Kaseorg (andersk) wrote :

I’m about 8% of the way through my list, and it looks like there might indeed be a _lot_ of affected Ubuntu packages. I’ll stop filing bugs for now and see what happens with these ones.

Revision history for this message
Kees Cook (kees) wrote :

Given the large number of affected packages, perhaps it is better to fix the compiler option. I'm curious to see what upstream thinks of this.

Revision history for this message
In , Kees Cook (kees) wrote :

Anders Kaseorg noticed that the use of _FORTIFY_SOURCE breaks a specific use of
sprintf (see attached):

$ gcc -O0 -o foo foo.c && ./foo
not fail
$ gcc -O2 -o foo foo.c && ./foo
not fail
$ gcc -O2 -D_FORTIFY_SOURCE=2 -o foo foo.c && ./foo
fail

The original report was filed in Ubuntu, where -D_FORTIFY_SOURCE=2 is enabled by
default: https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/305901

C99 states:
The sprintf function is equivalent to fprintf, except that the output is written
into an array (specified by the argument s) rather than to a stream. A null
character is written at the end of the characters written; it is not counted as
part of the returned value. If copying takes place between objects that overlap,
the behavior is undefined.

The man page does not mention this limitation, and prior to the use of
__sprintf_chk, this style of call worked as expected. As such, a large volume
of source code uses this style of call:
http://web.mit.edu/andersk/Public/sprintf-results

It seems that it would make sense to fix __sprintf_chk, or very loudly mention
the C99-described overlap-is-undefined behavior in sprintf documentation.

Revision history for this message
In , Kees Cook (kees) wrote :

Created attachment 3095
test case

Changed in glibc:
importance: Undecided → High
status: New → Confirmed
Revision history for this message
In , Andreas Schwab (schwab-linux-m68k) wrote :

sprintf(buf, "%sfoo", buf) is UNDEFINED.

Changed in glibc:
status: Unknown → Invalid
Revision history for this message
In , Kees Cook (kees) wrote :

Thanks for the clarification. However, I think it is still a bug that the
limitation is not mentioned in the manpage.

Revision history for this message
In , Andreas Schwab (schwab-linux-m68k) wrote :

Then contact whoever wrote it.

Revision history for this message
Kees Cook (kees) wrote :

Searching all of Ubuntu source in Jaunty:

29 main
0 restricted
182 universe
15 multiverse

Revision history for this message
Matthias Klose (doko) wrote :

> You can search a source file for instances of it with this regex:
> pcregrep -M 'sprintf\s*\(\s*([^,]*)\s*,\s*"%s[^"]*"\s*,\s*\1\s*,'

the regexp doesn't search for snprintf, and doesn't look for functions spanning more than one line.

> I’ll stop filing bugs for now and see what happens with these ones.

the bug reports are ok, but separate reports with a common tag should be filed instead.

Revision history for this message
Anders Kaseorg (andersk) wrote :

>> pcregrep -M 'sprintf\s*\(\s*([^,]*)\s*,\s*"%s[^"]*"\s*,\s*\1\s*,'
>
> the regexp doesn't search for snprintf, and doesn't look for functions spanning more than one line.

It does with pcregrep -M. For example,

$ pcregrep -M 'sprintf\s*\(\s*([^,]*)\s*,\s*"%s[^"]*"\s*,\s*\1\s*,' \
  linux-2.6/arch/x86/kernel/cpu/intel_cacheinfo.c
  ret += sprintf(buf, "%sEntry: %d\n", buf, i);
  ret += sprintf(buf, "%sReads: %s\tNew Entries: %s\n",
   buf,
  ret += sprintf(buf, "%sSubCache: %x\tIndex: %x\n",
   buf, (reg & 0x30000) >> 16, reg & 0xfff);

However, it appears that the multiline results did not show up in Kees’ reports, so the reports should be rerun with pcregrep -M if that is possible.

Revision history for this message
Anders Kaseorg (andersk) wrote :

For snprintf, use

pcregrep -M 'snprintf\s*\(\s*([^,]*)\s*,[^,]*,\s*"%s[^"]*"\s*,\s*\1\s*,' "$@"

Revision history for this message
Kees Cook (kees) wrote :

yeah, my search was glitched. New logs attached only count difference was universe, which went to 187.

Revision history for this message
Kees Cook (kees) wrote :
Revision history for this message
Kees Cook (kees) wrote :
Revision history for this message
In , Jakub Jelinek (jakub-redhat) wrote :

man 3p sprintf certainly documents it:
"If copying takes place between objects that overlap as a result of a call
to sprintf() or snprintf(), the results are undefined."

Revision history for this message
In , Petr Baudis (pasky) wrote :

I have submitted a patch for linux-manpages:
http://thread.gmane.org/gmane.linux.man/639

Revision history for this message
In , Michael Kerrisk (mtk-manpages) wrote :

(In reply to comment #6)
> I have submitted a patch for linux-manpages:
> http://thread.gmane.org/gmane.linux.man/639

I've applied the following patch for man-pages-3.16.

--- a/man3/printf.3
+++ b/man3/printf.3
@@ -133,6 +133,17 @@ string that specifies how subsequent arguments (or
arguments accessed via
 the variable-length argument facilities of
 .BR stdarg (3))
 are converted for output.
+
+C99 and POSIX.1-2001 specify that the results are undefined if a call to
+.BR sprintf (),
+.BR snprintf (),
+.BR vsprintf (),
+or
+.BR vsnprintf ()
+would cause to copying to take place between objects that overlap
+(e.g., if the target string array and one of the supplied input arguments
+refer to the same buffer).
+See NOTES.
 .SS "Return value"
 Upon successful return, these functions return the number of characters
 printed (not including the
@@ -851,6 +862,26 @@ and conversion characters \fBa\fP and \fBA\fP.
 glibc 2.2 adds the conversion character \fBF\fP with C99 semantics,
 and the flag character \fBI\fP.
 .SH NOTES
+Some programs imprudently rely on code such as the following
+
+ sprintf(buf, "%s some further text", buf);
+
+to append text to
+.IR buf .
+However, the standards explicitly note that the results are undefined
+if source and destination buffers overlap when calling
+.BR sprintf (),
+.BR snprintf (),
+.BR vsprintf (),
+and
+.BR vsnprintf ().
+.\" http://sourceware.org/bugzilla/show_bug.cgi?id=7075
+Depending on the version of
+.BR gcc (1)
+used, and the compiler options employed, calls such as the above will
+.B not
+produce the expected results.
+
 The glibc implementation of the functions
 .BR snprintf ()
 and

Revision history for this message
Anders Kaseorg (andersk) wrote :

Kees, some quick questions about your search:
• There are no instances of snprintf in your results. I could believe that there aren’t any, because this use of snprintf has been broken for longer than this use of sprintf, but I just wanted to confirm this.
• Does your search include DBS style tarball-inside-a-tarball packages?

Matthias, shall I go ahead and use massfile to create 231 bugs for this issue? I have attached a proposed massfile template, and tested it by filing bug #310800 against barnowl. I noticed though that massfile didn’t successfully add the sprintf-append tag as I was expecting; I’m not sure why.

Revision history for this message
Anders Kaseorg (andersk) wrote :

Oops, and I would use the right bug URL, of course.

Revision history for this message
Kees Cook (kees) wrote : Re: [Bug 305901] Re: Intrepid gcc -O2 breaks string appending with sprintf(), due to fortify source patch

On Tue, Dec 23, 2008 at 06:14:32AM -0000, Anders Kaseorg wrote:
> • There are no instances of snprintf in your results.

I haven't yet re-run the search with snprintf.

> • Does your search include DBS style tarball-inside-a-tarball packages?

It does not yet, but I've put together a script that will attempt to apply
all patches before doing the search. I was going to merge this when adding
the snprintf regex.

> Matthias, shall I go ahead and use massfile to create 231 bugs for this
> issue?

It probably makes more sense to approach Debian with the mass-filing. I'd
be happy to help drive this.

http://www.debian.org/doc/developers-reference/beyond-pkging.html#submit-many-bugs

Revision history for this message
Matthias Klose (doko) wrote : Re: [Bug 305901] Re: Intrepid gcc -O2 breaks string appending with sprintf(), due to fortify source patch

Kees Cook schrieb:
> On Tue, Dec 23, 2008 at 06:14:32AM -0000, Anders Kaseorg wrote:
>> Matthias, shall I go ahead and use massfile to create 231 bugs for this
>> issue?
>
> It probably makes more sense to approach Debian with the mass-filing. I'd
> be happy to help drive this.

seems to be the right thing. please use a non RC severity and a separate user
tag to identify these reports.

Revision history for this message
Kees Cook (kees) wrote :

http://people.ubuntu.com/~kees/sprintf-glibc/

     29 main
     15 multiverse
    208 universe
    251 total

I removed a few copies of the kernel, which all show the same report, as well as gnokii, which had a note in the Changelog about how they'd fixed it already.

Revision history for this message
Kees Cook (kees) wrote :

(er, 252 total -- I added "linux" back in at the last moment) I'm also testing a patch to glibc to avoid the change in behavior when using _FORTIFY_SOURCE.

Revision history for this message
In , Kees Cook (kees) wrote :

Created attachment 3625
work-around pre-trunc behavior

This patch restores the prior sprintf behavior. Looking through
_IO_str_init_static_internal seems to indicate that nothing actually requires
"s" to lead with a NULL. Is there anything wrong with this work-around, which
could be used until the number of affected upstream sources is not quite so
large?

Revision history for this message
Kees Cook (kees) wrote :

Marking the source packages as Invalid, since they will be handled upstream. The glibc patch restores the original behavior, so it will get SRU'd into Intrepid and fixed in Jaunty.

Changed in 4g8:
status: New → Invalid
Changed in abiword:
status: New → Invalid
Changed in asterisk:
status: New → Invalid
Changed in atomicparsley:
status: New → Invalid
Changed in audacious-plugins:
status: New → Invalid
Changed in barnowl:
status: New → Invalid
Changed in billard-gl:
status: New → Invalid
Changed in binutils:
status: New → Invalid
Changed in blender:
status: New → Invalid
Changed in ctn:
status: New → Invalid
Changed in gcc-4.3:
status: New → Invalid
Changed in glibc:
status: New → Invalid
Changed in hypermail:
status: New → Invalid
Changed in mpeg4ip:
status: New → Invalid
Changed in nagios-plugins:
status: New → Invalid
Changed in owl:
status: New → Invalid
Changed in xmcd:
status: New → Invalid
Changed in 4g8:
status: New → Invalid
Changed in abiword:
status: New → Invalid
Changed in asterisk:
status: New → Invalid
Changed in atomicparsley:
status: New → Invalid
Changed in audacious-plugins:
status: New → Invalid
Changed in barnowl:
status: New → Invalid
Changed in billard-gl:
status: New → Invalid
Changed in binutils:
status: New → Invalid
Changed in blender:
status: New → Invalid
Changed in ctn:
status: New → Invalid
Changed in glibc:
status: Confirmed → Invalid
Changed in hypermail:
status: New → Invalid
Changed in mpeg4ip:
status: New → Invalid
Changed in nagios-plugins:
status: New → Invalid
Changed in owl:
status: New → Invalid
Changed in xmcd:
status: New → Invalid
Changed in glibc:
assignee: nobody → kees
importance: Undecided → High
status: Invalid → Confirmed
assignee: nobody → kees
status: Invalid → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package glibc - 2.9-0ubuntu6

---------------
glibc (2.9-0ubuntu6) jaunty; urgency=low

  [ Matthias Klose ]
  * Merge with Debian, glibc-2.9 branch, r3200.

  [ Kees Cook ]
  * Add debian/patches/ubuntu/no-sprintf-pre-truncate.diff: do not
    pre-clear target buffers on sprintf to retain backward compatibility
    (LP: #305901).

 -- Kees Cook <email address hidden> Thu, 01 Jan 2009 13:28:59 -0800

Changed in glibc:
status: Fix Committed → Fix Released
Revision history for this message
Kees Cook (kees) wrote :

For intrepid-proposed:

glibc (2.8~20080505-0ubuntu8) intrepid-proposed; urgency=low

  * Add debian/patches/ubuntu/no-sprintf-pre-truncate.diff: do not
    pre-clear target buffers on sprintf to retain backward compatibility
    (LP: #305901).

Changed in glibc:
status: Confirmed → In Progress
Revision history for this message
Martin Pitt (pitti) wrote :

Accepted glibc into intrepid-proposed, please test and give feedback here. Please see https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Changed in glibc:
status: In Progress → Fix Committed
Revision history for this message
Mathieu Marquer (slasher-fun) wrote :

Not sure whether this is related (please tell me if it's not), but that is the only significant update I've done since yesterday (with xine...) :

With glibc 2.8~20080505-0ubuntu8 :
* The system takes a loooong time to scan the different WiFi networks available
* A "sudo iwlist wlan0 scan" returns "print_scanning_info: Allocation failed"

Please let me know if you need additional information.

Revision history for this message
Kees Cook (kees) wrote :

Mathieu: does reverting to an earlier glibc solve the problem for you?

Revision history for this message
Mathieu Marquer (slasher-fun) wrote :

Actually :
* On the time I've seen this problem, it was still there after three reboots. But it has now disappeared...
* If I try to revert to an earlier version of glibc, synaptic wants as well to remove 56 packets including some important ones... So I prefer not to try.

So for the moment, this problem has disappeared.

Revision history for this message
Martin Pitt (pitti) wrote :

Anyone who has this glibc version installed and can tell us whether the original problems/crashes are now fixed, as well as if the system generally still works as before?

Revision history for this message
Kees Cook (kees) wrote :

My intrepid machines with this glibc show the expected behavior and show no signs of regression.

Revision history for this message
Anders Kaseorg (andersk) wrote :

I can confirm that the intrepid-proposed libc6 fixes both my test program and the Intrepid barnowl package.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package glibc - 2.8~20080505-0ubuntu8

---------------
glibc (2.8~20080505-0ubuntu8) intrepid-proposed; urgency=low

  * Add debian/patches/ubuntu/no-sprintf-pre-truncate.diff: do not
    pre-clear target buffers on sprintf to retain backward compatibility
    (LP: #305901).

 -- Kees Cook <email address hidden> Wed, 07 Jan 2009 20:15:15 -0800

Changed in glibc:
status: Fix Committed → Fix Released
Changed in glibc:
importance: Unknown → Medium
Revision history for this message
In , Jackie-rosen (jackie-rosen) wrote :

*** Bug 260998 has been marked as a duplicate of this bug. ***
Seen from the domain http://volichat.com
Page where seen: http://volichat.com/adult-chat-rooms
Marked for reference. Resolved as fixed @bugzilla.

Revision history for this message
In , Kees Cook (kees) wrote :

I'd still like to have this patch applied -- while we can claim the behavior is "undefined", it is not, in fact, undefined. It behaves one way without -D_FORTIFY_SOURCE=2, and differently with it. And that difference doesn't need to exist. Ubuntu carried this patch for quite a while.

Revision history for this message
In , Andreas Schwab (schwab-linux-m68k) wrote :

The point of _FORTIFY_SOURCE is to expose undefined behaviour.

Revision history for this message
In , Kees Cook (kees) wrote :

It's not defined in POSIX, but it has worked a certain way in glibc for decades. There's no _reason_ to break it for _FORTIFY_SOURCE. Pre-truncating just silently breaks programs and does weird stuff. If you want to expose it with _FORITFY_SOURCE then have vsprintf notice that the target and first format argument are the same variable, and refuse to build.

Either pretruncation should be eliminated, or the undefined behavior should be explicitly detected and dealt with. Just having programs lose data while running with no indication of the cause seems like a terrible user experience.

Revision history for this message
In , Siddhesh-n (siddhesh-n) wrote :

It might be a good idea to take this discussion to the libc-alpha mailing list.

Revision history for this message
In , Kees Cook (kees) wrote :

So I'd like to bring this back up and reiterate the issue: there is no benefit to the early truncation, and it actively breaks lots of existing software (which is why Debian and Ubuntu have had this fix for 10 years now).

What is the _benefit_ of early truncation that justifies breaking so many existing cases?

Can glibc please take this patch? http://paste.ubuntu.com/p/CbrxmSfKD4/

Revision history for this message
In , Siddhesh-n (siddhesh-n) wrote :

There was a pretty lengthy discussion on this late last year:

https://sourceware.org/ml/libc-alpha/2018-12/msg00838.html

where the behaviour breakage was introduced in the non-fortified path and then reverted. It might be a good idea to resume that discussion for the fortified case as well.

Changed in glibc:
status: Invalid → Confirmed
Revision history for this message
In , Nsz-j (nsz-j) wrote :

(In reply to Kees Cook from comment #14)
> So I'd like to bring this back up and reiterate the issue: there is no
> benefit to the early truncation, and it actively breaks lots of existing
> software (which is why Debian and Ubuntu have had this fix for 10 years now).
>
> What is the _benefit_ of early truncation that justifies breaking so many
> existing cases?

ideally sprintf, snprintf and sprintf_chk would be able to share code and have identical behaviour (currently there is a lot of duplicated logic in glibc with a potential for inconsistent behaviour).

that said, i think _FORTIFY_SOURCE should detect undefined behaviour if possible since it's a bug that breaks portability.

note that it does not matter what guarantees a library documents: there are plenty of precedents for compiler optimizations to break code based on ub in library calls, a compiler can remove all code paths leading to a sprintf(s, "%s", s), trying to make such code work in glibc is just hiding a ticking time bomb.

Revision history for this message
In , Florian Weimer (fweimer) wrote :

(In reply to Szabolcs Nagy from comment #16)
> (In reply to Kees Cook from comment #14)
> > So I'd like to bring this back up and reiterate the issue: there is no
> > benefit to the early truncation, and it actively breaks lots of existing
> > software (which is why Debian and Ubuntu have had this fix for 10 years now).
> >
> > What is the _benefit_ of early truncation that justifies breaking so many
> > existing cases?

I wonder if the early truncation was introduced to avoid cases where aliasing can be used to avoid fortify length checks. But then again, truncation might not effectively prevent that after all. And we do not seem to use strlen followed by strcpy in vfprintf.

I haven't looked at this in detail, though.

> ideally sprintf, snprintf and sprintf_chk would be able to share code and
> have identical behaviour (currently there is a lot of duplicated logic in
> glibc with a potential for inconsistent behaviour).

Not sure what you mean by this. The core vfprintf engine is shared, of course.

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

Other bug subscribers

Remote bug watches

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