|Summary:||sprintf(buf, "%sfoo", buf) has different results with -O2 -D_FORTIFY_SOURCE=2 (__sprintf_chk bug?)|
|Product:||glibc||Reporter:||Kees Cook <kees>|
|Component:||libc||Assignee:||Ulrich Drepper <drepper.fsp>|
|Severity:||normal||CC:||adconrad, fweimer, glibc-bugs, lidaobing, nsz, siddhesh, yann|
work-around pre-trunc behavior
Description Kees Cook 2008-12-07 17:42:25 UTC
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.
Comment 2 Andreas Schwab 2008-12-07 17:49:37 UTC
sprintf(buf, "%sfoo", buf) is UNDEFINED.
Comment 3 Kees Cook 2008-12-07 18:33:34 UTC
Thanks for the clarification. However, I think it is still a bug that the limitation is not mentioned in the manpage.
Comment 4 Andreas Schwab 2008-12-07 19:05:38 UTC
Then contact whoever wrote it.
Comment 5 Jakub Jelinek 2008-12-07 22:56:34 UTC
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."
Comment 6 Petr Baudis 2008-12-07 23:38:40 UTC
I have submitted a patch for linux-manpages: http://thread.gmane.org/gmane.linux.man/639
Comment 7 Michael Kerrisk 2008-12-19 16:57:40 UTC
(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
Comment 8 Kees Cook 2008-12-24 17:40:22 UTC
Created attachment 3625 [details] 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?
Comment 9 Jackie Rosen 2014-02-16 17:44:19 UTC Comment hidden (spam)
*** 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.
Comment 10 Kees Cook 2014-06-13 19:49:35 UTC
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.
Comment 11 Andreas Schwab 2014-06-13 20:25:19 UTC
The point of _FORTIFY_SOURCE is to expose undefined behaviour.
Comment 12 Kees Cook 2014-06-13 20:36:28 UTC
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.
Comment 13 Siddhesh Poyarekar 2014-06-13 20:48:29 UTC
It might be a good idea to take this discussion to the libc-alpha mailing list.
Comment 14 Kees Cook 2019-02-21 17:01:54 UTC
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/
Comment 15 Siddhesh Poyarekar 2019-02-21 18:10:45 UTC
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.
Comment 16 Szabolcs Nagy 2019-02-25 14:51:44 UTC
(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.
Comment 17 Florian Weimer 2019-02-27 17:16:25 UTC
(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.