After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 674452 - SEGFAULT in gio contenttype test
SEGFAULT in gio contenttype test
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
2.32.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
: 679972 (view as bug list)
Depends on:
Blocks: 678941
 
 
Reported: 2012-04-20 08:21 UTC by Hans de Graaff
Modified: 2012-07-18 01:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gio/tests/contenttype: Call g_content_type_guess() with valid data len (1.36 KB, patch)
2012-06-27 07:27 UTC, Martin Pitt
reviewed Details | Review
g_content_type_guess(): Permit data_size to be -1 (1.63 KB, patch)
2012-06-27 07:31 UTC, Martin Pitt
rejected Details | Review
gio/tests/contenttype: Call g_content_type_guess() with valid data len (1.36 KB, patch)
2012-06-27 14:04 UTC, Martin Pitt
none Details | Review
gio/tests/contenttype: Call g_content_type_guess() with valid data len (2.46 KB, patch)
2012-06-27 14:06 UTC, Martin Pitt
committed Details | Review
win32: fix build after bug 674452 (843 bytes, patch)
2012-07-03 18:04 UTC, Marc-Andre Lureau
accepted-commit_now Details | Review
win32: fix build after bug 674452 (843 bytes, patch)
2012-07-04 01:01 UTC, Matthias Clasen
committed Details | Review

Description Hans de Graaff 2012-04-20 08:21:27 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
  • #0 cache_magic_matchlet_compare_to_data
    at xdgmimecache.c line 199
  • #1 cache_magic_matchlet_compare
    at xdgmimecache.c line 225
  • #2 cache_magic_compare_to_data
    at xdgmimecache.c line 257
  • #3 cache_magic_lookup_data
    at xdgmimecache.c line 293
  • #4 cache_get_mime_type_for_data
    at xdgmimecache.c line 710
  • #5 __gio_xdg_cache_get_mime_type_for_data
    at xdgmimecache.c line 740
  • #6 g_content_type_guess
    at gcontenttype.c line 939
  • #7 test_guess
    at contenttype.c line 43
  • #8 ??
    from /usr/lib64/libglib-2.0.so.0
  • #9 ??
    from /usr/lib64/libglib-2.0.so.0
  • #10 g_test_run_suite
    from /usr/lib64/libglib-2.0.so.0
  • #11 main
    at contenttype.c line 200

Comment 1 Hans de Graaff 2012-04-20 08:22:51 UTC
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.
Comment 2 Martin Pitt 2012-06-27 06:22:31 UTC
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.
Comment 3 Martin Pitt 2012-06-27 07:27:45 UTC
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.
Comment 4 Martin Pitt 2012-06-27 07:31:35 UTC
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".
Comment 5 Martin Pitt 2012-06-27 08:21:06 UTC
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 6 Dan Winship 2012-06-27 13:11:17 UTC
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...)
Comment 7 Martin Pitt 2012-06-27 13:37:38 UTC
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?
Comment 8 Dan Winship 2012-06-27 13:42:18 UTC
(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 9 Martin Pitt 2012-06-27 13:54:00 UTC
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.
Comment 10 Martin Pitt 2012-06-27 14:04:59 UTC
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.
Comment 11 Martin Pitt 2012-06-27 14:06:04 UTC
Created attachment 217405 [details] [review]
gio/tests/contenttype: Call g_content_type_guess() with valid data len

Meh, attached the wrong file.
Comment 12 Martin Pitt 2012-06-27 14:10:51 UTC
danw | pitti: looks good. thanks

Pushed, thanks for the review!
Comment 13 Marc-Andre Lureau 2012-07-03 17:59:49 UTC
this patch breaks windows build, where XDG_MIME_TYPE_UNKNOWN is not defined
Comment 14 Marc-Andre Lureau 2012-07-03 18:00:14 UTC
  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
Comment 15 Marc-Andre Lureau 2012-07-03 18:02:12 UTC
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 ("*"));
Comment 16 Marc-Andre Lureau 2012-07-03 18:04:46 UTC
Created attachment 217951 [details] [review]
win32: fix build after bug 674452
Comment 17 Matthias Clasen 2012-07-04 00:58:34 UTC
Review of attachment 217951 [details] [review]:

Looks fine to me.
Comment 18 Matthias Clasen 2012-07-04 00:59:26 UTC
Review of attachment 217951 [details] [review]:

Looks fine to me.
Comment 19 Matthias Clasen 2012-07-04 00:59:27 UTC
Review of attachment 217951 [details] [review]:

Looks fine to me.
Comment 20 Matthias Clasen 2012-07-04 00:59:27 UTC
Review of attachment 217951 [details] [review]:

Looks fine to me.
Comment 21 Matthias Clasen 2012-07-04 01:01:24 UTC
The following fix has been pushed:
af3b167 win32: fix build after bug 674452
Comment 22 Matthias Clasen 2012-07-04 01:01:29 UTC
Created attachment 217977 [details] [review]
win32: fix build after bug 674452
Comment 23 Martin Pitt 2012-07-04 05:23:36 UTC
Thanks Marc-Andre!
Comment 24 Maarten Bosmans 2012-07-15 12:49:16 UTC
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?
Comment 26 Kalev Lember 2012-07-15 18:55:14 UTC
(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".
Comment 27 Marc-Andre Lureau 2012-07-15 20:01:58 UTC
*** Bug 679972 has been marked as a duplicate of this bug. ***
Comment 28 Marc-Andre Lureau 2012-07-15 20:04:57 UTC
(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 "*".
Comment 29 Fan, Chun-wei 2012-07-18 01:48:59 UTC
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!