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 648147 - Creating structs without constructors segfaults and asserts
Creating structs without constructors segfaults and asserts
Status: RESOLVED OBSOLETE
Product: pygobject
Classification: Bindings
Component: introspection
3.2.x
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks: 708060
 
 
Reported: 2011-04-18 19:27 UTC by Nirbheek Chauhan
Modified: 2018-01-10 20:08 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Nirbheek Chauhan 2011-04-18 19:27:00 UTC
While using glib-2.28.6 gobject-introspection-0.10.7 (with the last variant-related patch added), and pygobject-2.28.4, instantiating the same proxy object three times causes a segfault.

Steps to reproduce:

$ python2
>>> from gi.repository import Gio
>>> a = Gio.DBusProxy.new_for_bus_sync(Gio.BusType.SESSION, Gio.DBusProxyFlags.DO_NOT_AUTO_START, Gio.DBusInterfaceInfo(), "org.gnome.Rhythmbox", "/org/gnome/Rhythmbox/Player", "org.gnome.Rhythmbox.Player", None)
>>> b = Gio.DBusProxy.new_for_bus_sync(Gio.BusType.SESSION, Gio.DBusProxyFlags.DO_NOT_AUTO_START, Gio.DBusInterfaceInfo(), "org.gnome.Rhythmbox", "/org/gnome/Rhythmbox/Player", "org.gnome.Rhythmbox.Player", None)
>>> c = Gio.DBusProxy.new_for_bus_sync(Gio.BusType.SESSION, Gio.DBusProxyFlags.DO_NOT_AUTO_START, Gio.DBusInterfaceInfo(), "org.gnome.Rhythmbox", "/org/gnome/Rhythmbox/Player", "org.gnome.Rhythmbox.Player", None)
Segmentation fault
$ echo $?
139

This happens with any dbus interface, not just Rhythmbox, and doesn't happen when using gdbus from C. Backtrace:

(gdb) run
Starting program: /usr/bin/python gio_segfault.py
process 1010 is executing new program: /usr/bin/python2.6
[Thread debugging using libthread_db enabled]
[New Thread 0x7ffff39aa700 (LWP 1013)]

Program received signal SIGSEGV, Segmentation fault.
g_slice_alloc (mem_size=40) at gslice.c:833
833	gslice.c: No such file or directory.
	in gslice.c
(gdb) bt
  • #0 g_slice_alloc
    at gslice.c line 833
  • #1 g_variant_alloc
    at gvariant-core.c line 475
  • #2 g_variant_new_from_children
    at gvariant-core.c line 560
  • #3 g_variant_builder_end
    at gvariant.c line 3260
  • #4 g_variant_valist_new
    at gvariant.c line 4096
  • #5 g_variant_new_va
    at gvariant.c line 4248
  • #6 g_variant_new
  • #7 remove_match_rule
    at gdbusconnection.c line 3062
  • #8 unsubscribe_id_internal
    at gdbusconnection.c line 3294
  • #9 g_dbus_connection_signal_unsubscribe
    at gdbusconnection.c line 3329
  • #10 g_dbus_proxy_finalize
    at gdbusproxy.c line 148
  • #11 g_object_unref
    at gobject.c line 2734
  • #12 pygobject_clear
    at pygobject.c line 1138
  • #13 pygobject_dealloc
    at pygobject.c line 1048
  • #14 ??
    from /usr/lib64/libpython2.6.so.1.0
  • #15 ??
    from /usr/lib64/libpython2.6.so.1.0
  • #16 PyDict_SetItem
    from /usr/lib64/libpython2.6.so.1.0
  • #17 _PyModule_Clear
    from /usr/lib64/libpython2.6.so.1.0
  • #18 PyImport_Cleanup
    from /usr/lib64/libpython2.6.so.1.0
  • #19 Py_Finalize
    from /usr/lib64/libpython2.6.so.1.0
  • #20 Py_Main
    from /usr/lib64/libpython2.6.so.1.0
  • #21 __libc_start_main
    at libc-start.c line 226
  • #22 _start

Comment 1 Nirbheek Chauhan 2011-04-18 19:30:41 UTC
Deleting a Gio.DBusProxy object using del() causes an assert. Steps to reproduce:

$ python2
>>> from gi.repository import Gio
>>> a = Gio.DBusProxy.new_for_bus_sync(Gio.BusType.SESSION, Gio.DBusProxyFlags.DO_NOT_AUTO_START, Gio.DBusInterfaceInfo(), "org.gnome.Rhythmbox", "/org/gnome/Rhythmbox/Player", "org.gnome.Rhythmbox.Player", None)
>>> del(a)
>>> **
GLib:ERROR:gvarianttypeinfo.c:186:g_variant_type_info_check: assertion failed: (0 <= index && index < 24)
Aborted

Sometimes, for the same steps, instead of the above assert, I get:

>>> **
GLib:ERROR:gvarianttypeinfo.c:187:g_variant_type_info_check: assertion failed: (g_variant_type_info_basic_chars[index][0] != ' ')
Aborted
Comment 2 Nirbheek Chauhan 2011-04-18 19:56:03 UTC
Just to clarify, this has happened since at least pygobject 2.28.0, and is not specific to the latest changes.
Comment 3 johnp 2011-04-19 13:51:29 UTC
Looks like we are most likely double freeing it.  The second time contains junk values that trigger the assert.  For now you can use this.  It doesn't seem to trigger the segfault:

bus = Gio.bus_get_sync(Gio.BusType.SESSION, None)
a = Gio.DBusProxy.new_sync(bus, Gio.DBusProxyFlags.DO_NOT_AUTO_START, Gio.DBusInterfaceInfo(), "org.gnome.Rhythmbox", "/org/gnome/Rhythmbox/Player", "org.gnome.Rhythmbox.Player", None)

This might have to do with the fact the cancelable is not marked as allow-none for Gio.DBusProxy.new_for_bus_sync
Comment 4 Martin Pitt 2011-04-19 14:27:43 UTC
The assertion you got in comment 1 looks a lot like bug 639952 (which has its root cause in bug 647796). However, I confirm that both crash variants here remain valid with the g_variant_new_variant() annotation fixed.
Comment 5 Martin Pitt 2011-04-19 14:50:02 UTC
I think it might be related to the DBusInterfaceInfo. This works:

$ python -c 'from gi.repository import Gio; b=Gio.bus_get_sync(Gio.BusType.SESSION, None); a = Gio.DBusProxy.new_sync(b, Gio.DBusProxyFlags.DO_NOT_AUTO_START, None, "org.gnome.Rhythmbox", "/org/gnome/Rhythmbox/Player", "org.gnome.Rhythmbox.Player", None); print a; del(a)'
<DBusProxy object at 0x2477aa0 (GDBusProxy at 0x2521060)>

But if I pass a real Gio.DBusInterfaceInfo(), it crashes (sometimes, anyway):

$ python -c 'from gi.repository import Gio; b=Gio.bus_get_sync(Gio.BusType.SESSION, None); a = Gio.DBusProxy.new_sync(b, Gio.DBusProxyFlags.DO_NOT_AUTO_START, Gio.DBusInterfaceInfo(), "org.gnome.Rhythmbox", "/org/gnome/Rhythmbox/Player", "org.gnome.Rhythmbox.Player", None); print a; del(a)'
<DBusProxy object at 0x2421aa0 (GDBusProxy at 0x2498860)>
**
GLib:ERROR:/build/buildd/glib2.0-2.28.5/./glib/gvarianttypeinfo.c:187:g_variant_type_info_check: assertion failed: (g_variant_type_info_basic_chars[index][0] != ' ')
Abgebrochen (Speicherabzug geschrieben)

Testing this with new_for_bus_sync() is a bit trickier, as this is missing an allow-none for info. But as they really both crash in the same way, I suppose testing with new_sync() is sufficient.
Comment 6 Martin Pitt 2011-04-19 14:52:22 UTC
Note to self. When instantiating the interface info before, it crashes harder reliably:

python -c 'from gi.repository import Gio; b=Gio.bus_get_sync(Gio.BusType.SESSION, None); i = Gio.DBusInterfaceInfo(); a = Gio.DBusProxy.new_sync(b, Gio.DBusProxyFlags.DO_NOT_AUTO_START, i, "org.gnome.Rhythmbox", "/org/gnome/Rhythmbox/Player", "org.gnome.Rhythmbox.Player", None); print a; del a'
<DBusProxy object at 0x20a7aa0 (GDBusProxy at 0x211e860)>
*** glibc detected *** python: free(): invalid pointer: 0x0000000002117a10 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x78a8f)[0x7fd3ce18ca8f]
/lib/x86_64-linux-gnu/libc.so.6(cfree+0x73)[0x7fd3ce1908e3]
/usr/lib/x86_64-linux-gnu/libgio-2.0.so.0(+0xb280b)[0x7fd3cbff580b]
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0(g_object_unref+0x174)[0x7fd3ccf8bbe4]
/usr/lib/python2.7/dist-packages/gobject/_gobject.so(+0x12ffd)[0x7fd3cb4a8ffd]
python[0x48a2bd]
[...]
Comment 7 Martin Pitt 2011-04-19 16:10:05 UTC
This is apparently related to the fact that Gio.DBusInterfaceInfo is not a class, just a struct, which might get improperly initialized. In particular, the default ref_count argument is 0, which apparently causes this double-free.

It does work if I initialize it to 1:

>>> from gi.repository import Gio
>>> i = Gio.DBusInterfaceInfo()
>>> i.ref_count = 1
>>> a = Gio.DBusProxy.new_for_bus_sync(Gio.BusType.SESSION, Gio.DBusProxyFlags.DO_NOT_AUTO_START, i, "org.gnome.Rhythmbox", "/org/gnome/Rhythmbox/Player", "org.gnome.Rhythmbox.Player", None)
>>> del a

Out of interest, was this just a stripped down example, or do you actually just use a temporary Gio.DBusInterfaceInfo() struct in your production code? It doesn't seem to make much sense without any real data anyway? Or is that just to work around the missing allow-none annotation for the info parameter in new_for_bus_sync()?
Comment 8 Martin Pitt 2011-04-19 16:19:47 UTC
Note, glib and g-i git master already have the correct allow-none annotation, so you can call new_for_bus_sync() with a None value for info.
Comment 9 johnp 2011-04-19 17:17:24 UTC
I see why this is crashing.  There are no constructors for GDBusIntefaceInfo so we shouldn't construct it.  i = Gio.DBusInterfaceInfo() is not proper but GI has no way of telling us this (or at least we don't check to see if there are any constructors).

One fix is to disallow constructing any class or struct which does not have a constructor. That however breaks for things like tree iters.  The other option is to create an override for constructing a Gio.DBusInterfaceInfo which errors out.
Comment 10 Nirbheek Chauhan 2011-04-19 17:24:50 UTC
(In reply to comment #7)
> Out of interest, was this just a stripped down example, or do you actually just
> use a temporary Gio.DBusInterfaceInfo() struct in your production code? It
> doesn't seem to make much sense without any real data anyway? Or is that just
> to work around the missing allow-none annotation for the info parameter in
> new_for_bus_sync()?

It was entirely to work around the allow-none annotation not being there :). My code hasn't grown to the point where any real data is needed to be passed.

I guess I should backport the annotation fixes to the current stable glib/g-i releases and verify.

(In reply to comment #9)
> I see why this is crashing.  There are no constructors for GDBusIntefaceInfo so
> we shouldn't construct it.  i = Gio.DBusInterfaceInfo() is not proper but GI
> has no way of telling us this (or at least we don't check to see if there are
> any constructors).
> 
> One fix is to disallow constructing any class or struct which does not have a
> constructor. That however breaks for things like tree iters.  The other option
> is to create an override for constructing a Gio.DBusInterfaceInfo which errors
> out.

If Gio.DBusInterfaceInfo() shouldn't be called directly, maybe it shouldn't be exposed in the API at all? It only causes confusion when looking at dir(Gio) output.

Thanks a *lot* for the analysis and help!
Comment 11 johnp 2011-04-20 20:30:42 UTC
(In reply to comment #10)

> (In reply to comment #9)
> > I see why this is crashing.  There are no constructors for GDBusIntefaceInfo so
> > we shouldn't construct it.  i = Gio.DBusInterfaceInfo() is not proper but GI
> > has no way of telling us this (or at least we don't check to see if there are
> > any constructors).
> > 
> > One fix is to disallow constructing any class or struct which does not have a
> > constructor. That however breaks for things like tree iters.  The other option
> > is to create an override for constructing a Gio.DBusInterfaceInfo which errors
> > out.
> 
> If Gio.DBusInterfaceInfo() shouldn't be called directly, maybe it shouldn't be
> exposed in the API at all? It only causes confusion when looking at dir(Gio)
> output.
> 
> Thanks a *lot* for the analysis and help!

We can't skip it because it still can be returned from a function and you can still manipulate it before sending it into the method.  The biggest issue is that it is a struct which is refcounted.  If they wanted refcounting they should have created an object which could also validate itself and not crash if not set up correctly.  API like this might make sense in C as a quick and dirty hack but for scripting languages it is just broken.  If you did the same thing in C (malloc the struct) you would crash also.  Since C allows you to do anything C programmers are more careful when it comes to this.

I need to check if we really need the ability to create structs without constructors.  One of the things we can do is disallow it but create a special interface that indicates the person knows what they are doing - e.g. info = Gio.DBusInterfaceInfo.__malloc__() or info = gi.Malloc(Gio.DBusInterfaceInfo)
Comment 12 Martin Pitt 2012-04-22 07:59:52 UTC
Confirmed with 3.2 using the test case in comment 6.
Comment 13 GNOME Infrastructure Team 2018-01-10 20:08:33 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/15.