GNOME Bugzilla – Bug 648147
Creating structs without constructors segfaults and asserts
Last modified: 2018-01-10 20:08:33 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
+ Trace 226777
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
Just to clarify, this has happened since at least pygobject 2.28.0, and is not specific to the latest changes.
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
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.
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.
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] [...]
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()?
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.
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.
(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!
(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)
Confirmed with 3.2 using the test case in comment 6.
-- 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.