GNOME Bugzilla – Bug 674452
SEGFAULT in gio contenttype test
Last modified: 2012-07-18 01:48:59 UTC
Downstream bug report: https://bugs.gentoo.org/show_bug.cgi?id=411907 Running the tests for glib 2.32.1 I'm getting this test failure: TEST: contenttype... (pid=13562) /contenttype/guess: FAIL GTester: last random seed: R02Sbad8975595b540f5fe12259dfda1ca07 /bin/sh: line 1: 5098 Terminated G_DEBUG=gc-friendly MALLOC_CHECK_=2 MALLOC_PERTURB_=$((${RANDOM:-256} % 256)) ../../glib/gtester --verbose io-stream memory-input-stream memory-output-stream readwrite g-file g-file-info converter-stream data-input-stream data-output-stream g-icon buffered-input-stream buffered-output-stream sleepy-stream filter-streams volumemonitor simple-async-result srvtarget contexts gsettings gschema-compile async-close-output-stream gdbus-addresses network-address gdbus-message socket pollable tls-certificate tls-interaction cancellable vfs network-monitor fileattributematcher gmenumodel resources actions gdbus-close-pending gdbus-connection gdbus-connection-flush gdbus-connection-loss gdbus-connection-slow gdbus-names gdbus-proxy gdbus-proxy-threads gdbus-proxy-well-known-name gdbus-introspection gdbus-threading gdbus-export gdbus-error gdbus-peer gdbus-exit-on-close gdbus-non-socket gdbus-bz627724 appinfo contenttype mimeapps file live-g-file desktop-app-info unix-fd unix-streams gapplication basic-application gdbus-test-codegen gdbus-serialization This happens because of a segmentation fault: Starting program: /var/tmp/portage/dev-libs/glib-2.32.1/work/glib-2.32.1/gio/tests/.libs/contenttype [Thread debugging using libthread_db enabled] /contenttype/guess: Program received signal SIGSEGV, Segmentation fault. cache_magic_matchlet_compare_to_data (len=18446744073709551615, data=0x401c40, offset=<optimized out>, cache=<optimized out>) at xdgmimecache.c:199 199 if (((unsigned char *)cache->buffer)[data_offset + j] != ((unsigned char *) data)[j + i]) (gdb) bt
+ Trace 230104
This seems similar to https://bugzilla.gnome.org/show_bug.cgi?id=625438 but there the reporter mentions that the C test fails and only the python tests in pygobject fails. For me the C test fails, I haven't looked at pygobject.
I confirm this with glib 2.33.3. It happens on several architectures on the Debian autobuilders as well. I can reproduce the crash on my Panda board when building with -O2 (unfortunately this makes the crash rather un-debuggable), so I'm building with -O0 now. But this line: if (((unsigned char *)cache->buffer)[data_offset + j] != ((unsigned char *) data)[j + i]) is obviously the culprit. i counts from range_start on, and j can grow as big as the cache's data lenght, but the "data" field only seems to be the original "data" argument supplied to g_content_type_guess(). In the beginning there is a check if (i + data_length > len) return FALSE; which should prevent an overflow, but the tests call g_content_type_guess() with a size argument of -1, which comes out as g_content_type_guess (filename=0x401c3e "test.otf", data=0x401c39 "OTTO", data_size=18446744073709551615, on a 64 bit platform. I don't believe that -1 is a valid argument here. It is not documented, and I don't see a particular check for this anywhere that would calculate the size based on the string length of data. So either the tests should supply a valid size, or the supplied data_size should be adjusted to strlen(data) if it is (gsize) -1.
Created attachment 217345 [details] [review] gio/tests/contenttype: Call g_content_type_guess() with valid data len This is one way to fix it, which adheres to the current documentation.
Created attachment 217346 [details] [review] g_content_type_guess(): Permit data_size to be -1 This is another way to fix it, to actually permit data_size to be -1. This might be preferrable in the sense of "be liberal with what you accept".
Note, after fixing this, the test case now fails reliably on all architectures; before it accidentally succeeded if it managed to not crash. I filed this as bug 678941 and gave the details there.
Comment on attachment 217345 [details] [review] gio/tests/contenttype: Call g_content_type_guess() with valid data len Can you also add g_return_val_if_fail (data_size != (gsize)-1, NULL); to g_content_type_guess()? (Or should that be "(gssize)data_size != -1" ? Whatever makes it assert I guess...)
So you favor the "strict" approach, i. e. not allow -1? > g_return_val_if_fail (data_size != (gsize)-1, NULL); Hmm, but NULL is also not a defined return value. So if this function should explicitly check for this case, I'd rather do a g_assert(data_size != (gsize) -1) to abort the program?
(In reply to comment #7) > So you favor the "strict" approach, i. e. not allow -1? yeah, the api takes an unsigned type, so... > Hmm, but NULL is also not a defined return value. So if this function should > explicitly check for this case, I'd rather do a > > g_assert(data_size != (gsize) -1) > > to abort the program? I'm not a fan of asserting... especially in a case that used to sorta kinda work most of the time. Maybe g_return_val_if_fail (..., g_strdup (XDG_MIME_TYPE_UNKNOWN)); or if (data_size == (gsize) -1) { g_warn_if_fail (data_size != (gsize) -1); data_size = 0; }
Comment on attachment 217346 [details] [review] g_content_type_guess(): Permit data_size to be -1 OK, seems we're not going with the "allow -1" case.
Created attachment 217403 [details] [review] gio/tests/contenttype: Call g_content_type_guess() with valid data len This adds the check for -1. Without the test case part I now get (/home/martin/upstream/glib/gio/tests/.libs/lt-contenttype:446): GLib-GIO-CRITICAL **: g_content_type_guess: assertion `data_size != (gsize) -1' failed and with the updated test case that part succeeds.
Created attachment 217405 [details] [review] gio/tests/contenttype: Call g_content_type_guess() with valid data len Meh, attached the wrong file.
danw | pitti: looks good. thanks Pushed, thanks for the review!
this patch breaks windows build, where XDG_MIME_TYPE_UNKNOWN is not defined
CC libgio_2_0_la-ginitable.lo gcontenttype.c: In function 'g_content_type_guess': gcontenttype.c:335:3: error: 'XDG_MIME_TYPE_UNKNOWN' undeclared (first use in this function) gcontenttype.c:335:3: note: each undeclared identifier is reported only once for each function it appears in
Would this be a possible fix? - g_return_val_if_fail (data_size != (gsize) -1, g_strdup (XDG_MIME_TYPE_UNKNOWN)); + g_return_val_if_fail (data_size != (gsize) -1, g_strdup ("*"));
Created attachment 217951 [details] [review] win32: fix build after bug 674452
Review of attachment 217951 [details] [review]: Looks fine to me.
The following fix has been pushed: af3b167 win32: fix build after bug 674452
Created attachment 217977 [details] [review] win32: fix build after bug 674452
Thanks Marc-Andre!
It seems that the original patch did make it into 2.32.4, but the fix did not. So now the 2.32.4 release does not build for win32. Any change this bug can be reopenend until a fix is committed to the 2.32 branch?
PS. See https://build.opensuse.org/package/live_build_log?arch=i586&package=mingw32-glib2&project=home%3Amkbosmans%3Amingw32&repository=openSUSE_12.1 for a typical failing build output.
(In reply to comment #15) > Would this be a possible fix? > > - g_return_val_if_fail (data_size != (gsize) -1, g_strdup > (XDG_MIME_TYPE_UNKNOWN)); > + g_return_val_if_fail (data_size != (gsize) -1, g_strdup ("*")); Is "*" an allowed mime type? Maybe "application/octet-stream" would be a better default as it's a commonly used mime fallback default elsewhere, or perhaps simply return NULL? For what it's worth, xdgmime.h defines XDG_MIME_TYPE_UNKNOWN as "application/octet-stream".
*** Bug 679972 has been marked as a duplicate of this bug. ***
(In reply to comment #26) > Is "*" an allowed mime type? Maybe "application/octet-stream" would be a better > default as it's a commonly used mime fallback default elsewhere, or perhaps > simply return NULL? > > For what it's worth, xdgmime.h defines XDG_MIME_TYPE_UNKNOWN as > "application/octet-stream". * g_content_type_is_unknown: * @type: a content type string * * Checks if the content type is the generic "unknown" type. * On UNIX this is the "application/octet-stream" mimetype, * while on win32 it is "*".
Review of attachment 217977 [details] [review]: Hi, Do people mind if I land this patch in the 2.32 branch? Seems like Windows builds are broken there as a result. With blessings, thank you!