GNOME Bugzilla – Bug 772054
Patch for glib/gspawn-win32-helper.c: unexpected behavior re CommandLineToArgvW()
Last modified: 2016-10-16 09:01:25 UTC
Created attachment 336355 [details] [review] Patch protect_wargv() in glib/gspawn-win32-helper.c This bug/patch applies to glib > 2.44, <= 2.50. Tried building the latest GIMP (2.9.5) with MSYS2 on XP SP3 (32-bit), and some cursors wouldn't compile. Example: me@machine MINGW32 ~/gimp/cursors $ glib-compile-resources --sourcedir=. --generate-source --target=gimp-color-picker-cursors.c gimp-color-picker-cursors.gresource.xml (glib-compile-resources.exe:3900): GLib-GIO-CRITICAL **: g_subprocess_wait: assertion 'G_IS_SUBPROCESS (subprocess)' failed (glib-compile-resources.exe:3900): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed gimp-color-picker-cursors.gresource.xml: Failed to read from child pipe (EOF). gspawn-win32-helper-console.exe dies with an invalid memory access, causing glib-compile-resources.exe to fail. I downgraded gspawn-win32-helper-console.exe from mingw-w64-i686-glib2-2.48.2-1 to 2.44.1-4 (where __wgetmainargs() is still used) and the GIMP build completed just fine. It turns out that CommandLineToArgvW() (at least on XP) doesn't end the argv pointer array with a NULL, but goes straight into argv[0] string data, as per MSDN. So gspawn-win32-helper.c:protect_wargv() counts argc too high and overflows. I've submitted what I think is the simplest/ most consistent fix (passing to protect_wargv() the argc returned by CommandLineToArgvW()), but only tested it in a debugger (no compile from source). Not sure if changing the parameter count on protect_wargv() is the Right Thing, I'll leave that to the experts. I wonder, just how many other places is CommandLineToArgvW() used incorrectly as a NULL terminated array?
Passing argc to protect_wargv() is the Right Thing (TM). This is the only place where argv array returned by CommandLineToArgv() is mistakenly treated as NULL-terminated. Patch is OK conceptually, i'll let nacho do the nitpicking. The patch also compiles.
Review of attachment 336355 [details] [review]: See the minor nitpick. Clearly we need a proper commit message as well. ::: gspawn-win32-helper.c @@ +72,2 @@ static gint +protect_wargv (gint argc, argc should be aligned
Created attachment 336374 [details] [review] W32: pass argc returned by CommandLineToArgvW() to to protect_wargv() Added a commit message. Aligned argc. Added an author.
Review of attachment 336374 [details] [review]: Looks good.
Attachment 336374 [details] pushed as 7485abe - W32: pass argc returned by CommandLineToArgvW() to to protect_wargv()
Dang, you folks work fast. Anyway, my concern about argument count was with public functions (but protect_wargv() is static local); also protect_wargv() doesn't match protect_argv() anymore, oh well. (Argv is _supposed_ to be NULL terminated, thank you Microsoft.) Thanks on behalf of all us XP holdouts, and sorry I was too late for a 2.50.0 commit.
Hi, msys2 now has update of glib to version 1.51.1 However looks this patch did cause regression. On msys2/mingw64 glib-compile-resources now fails for me (got same behavior on windows 7/windows 10): glib-compile-resources gresource.xml --sourcedir=.. --target=gresources.c --generate-source gresource.xml: Failed to read from child pipe (EOF). make: *** [Makefile.mingw:89: gresources.c] Error 1 When i've removed preprocess="xml-stripblanks" attributes from gresource.xml command above did success, so looks xmllint part failed. I locally reverted patch from this issue and rebuild glib for mingw64. With patch reverted glib-compile-resources finished successfully.
This patch should be 100% uncontroversial. If something breaks with this patch, there's a good chance that there was a bug out there, but this patch exposed it. I would advise to put a lot of MessageBoxA() calls in all the programs involved, attach gdb everywhere, and see what's going on.
Created attachment 337558 [details] [review] Patch for regression: pass CORRECT argc to protect_wargv() AAAARRRGGGc! Sorry, much dumb. Now everybody's broke. Maybe I was tired that day? Anyway, I've since learned git; cloned the glib source and compiled, and lo and behold it didn't work! Just took a little debugging to see that argc was 13 instead of 3 in line 92. Works (for me) with this patch. They had a reason to count argc after all... Juloml, can you confirm before they commit? BTW, CommandLineToArgvW() on Vista _does_ return a NULL terminated pointer array - more useful, but technically contrary to Microsoft's documentation (see https://msdn.microsoft.com/en-us/library/windows/desktop/bb776391(v=vs.85).aspx ). They probably fixed it after XP's end of service and never updated the docs.
I've tried fixup patch and regression gone away. Source generated successfully again via glib-compile-resources. Thanks.
Created attachment 337608 [details] [review] W32: pass correct (with offset) argc to protect_wargv
Comment on attachment 337608 [details] [review] W32: pass correct (with offset) argc to protect_wargv Attachment 337608 [details] pushed as 01bfa16 - W32: pass correct (with offset) argc to protect_wargv
I guess this needs to be cherry-picked on glib-2-50