GNOME Bugzilla – Bug 167569
g_string_append_printf crashes on win32 when used with a NULL argument
Last modified: 2009-11-04 11:14:25 UTC
Build and run the following piece of code ---------- #include <glib.h> #include <stdio.h> int main(int argc, char *argv[]) { GString *string=g_string_new(""); g_string_append_printf(string,"%s",NULL); printf(string->str); return 0; } ---------- I tested this against glib 2.4.8 and glib 2.6.1 (from http://gladewin32.sourceforge.net) on win32 (winxp sp2), using both mingw32-gcc 3.4.2 and MSVC.Net 2003 (7.1). The same program does not crash on linux (2.6.10, fedora core 3), with glib 2.4.8. I understand that passing a NULL pointer as third argument is a bad idea, but maybe some kind of g_return_val_if_fail(arg != NULL, NULL) would be nice in g_string_append_printf.
Surely this is not a bug. Don't expect printf() and its likes to be able to print any random garbage for the %s format. NULL is not a valid pointer to an array of chars. That it doesn't crash on Linux is purely coincidental. One might even say that it is a bug that it doesn't crash on Linux. (I don't see any mention of the "(null)" output for NULL pointers in the printf(3) manpage. I wouldn't rely on this "feature".) If a buggy program happens to work in one way on one system in one implementation-defined way one cannot expect it to work identically on another system. If we "fix" this, what next? Somebody might as well ask "why doesn't g_string_append_printf(string, "%s", 42) work on Linux? It works on FooBarIx 3.0."...
I used to agree 100% with Tor on this one, for a number reasons - Supporting NULL in printf is not required by any standard - We can't fix uses of printf(), only of the g_log(), g_debug(), g_strdup_printf(), etc. - I don't think we we should be using the gnulib printf wrapper *just* to fix this issue, if a printf (like the Solaris printf) is fine for us for every other reason. But I've had a change of heart on the matter ... if we *are* using the gnulib printf wrapper (as we do on Win32), it's just silly to sit there and crash an application for some g_debug() statement when a few line patch would avoid it. I'll attach such a patch. (Probably should be sent upstream to gnulib if we decide to go ahead and apply it.)
Created attachment 81516 [details] [review] Check for NULL %s in vasprintf.c
OK, I trust Owen on this, so I'm all for it. As long as we then also have a configure test to make sure we use the gnulib printf also on platforms where the system printf would be otherwise good enough but doesn't like NULLs.
I understood Owens comment to say "fix gnulib, but don't switch to using gnulib just for this"
Hmm. OK... Although I feel sorry for the people who then are going to wonder why their code crashes on Solaris if it works fine on Linux and Win32.
(In reply to comment #6) > Hmm. OK... Although I feel sorry for the people who then are going to wonder > why their code crashes on Solaris if it works fine on Linux and Win32. Agreed. Why not force the included printf in debugging mode and g_critical on things that are not portable?
mclasen> I understood Owens comment to say "fix gnulib, but don't switch to mclasen> using gnulib just for this" Yes, that's what I meant. tml> Hmm. OK... Although I feel sorry for the people who then are going to tml> wonder why their code crashes on Solaris if it works fine on Linux and tml> Win32. True. Maybe we should just put in the test Either way, there would be pressure on Sun to improve their printf :-) Solaris is probably the only significant example of a printf that is good enough otherwise but doesn't catch null. The test should be simple enough to write, , though leaving core files from a configure run is a just a little bit messy... behdad> Agreed. Why not force the included printf in debugging mode and behdad> g_critical on things that are not portable? The trouble is that the most common case of hitting crashes like this is in rare debug paths, or in places that are g_critical() already. After all, few programs are *typically* printing out "(null)" all the time. So, I don't think a g_critical() is going to catch a lot of the cases out there. And I must admit that I like the ability when inserting temporary debugging prints on Linux to not have to foo ? foo : "(null)"...
(In reply to comment #8) > behdad> Agreed. Why not force the included printf in debugging mode and > behdad> g_critical on things that are not portable? > > The trouble is that the most common case of hitting crashes like this is in > rare > debug paths, or in places that are g_critical() already. Then don't do it for g_warning/error/critical/message/...(). Just for g_print*, g_string_*, ... The real user APIs, not debugging ones. > After all, few > programs > are *typically* printing out "(null)" all the time. Well, nm-applet was doing that before it actually connected to any network and that was crashing on me. > So, I don't think a > g_critical() is going to catch a lot of the cases out there. And I must admit > that I like the ability when inserting temporary debugging prints on Linux > to not have to foo ? foo : "(null)"... You can do that as long as you use printf or g_message/...
Hello guys. On glib 2.19 this issue still exists. This patch will be applied to gnulib? Thanks.
All right then. Patch applied to trunk: 2009-02-27 Tor Lillqvist <tml@novell.com> Bug 167569 - g_string_append_printf crashes on win32 when used with a NULL argument * glib/gnulib/vasnprintf.c (vasnprintf): Add workaround for buggy programs. Patch by Owen.
Thank you very much Tor.
*** Bug 458457 has been marked as a duplicate of this bug. ***