GNOME Bugzilla – Bug 720380
Segfault when using GDBusMenuModel on a peer-to-peer connection
Last modified: 2017-11-17 11:30:09 UTC
I'm trying to use GDBusMenuModel on a peer-to-peer D-Bus connection (with NULL bus name). It works during the object is alive, but it segfaults when being unref'ed. I'm attaching a test case which produces the following backtrace with the current code. Program terminated with signal 11, Segmentation fault. 164 ../sysdeps/x86_64/multiarch/strcmp-sse42.S: No such file or directory. (gdb) bt b=0x7f8ff4001860) at gdbusmenumodel.c:200 key=0x7f8ff4001860, hash_return=0x7fffb2cacde8) at ghash.c:386 key=0x7f8ff4001860, notify=1) at ghash.c:1283 key=0x7f8ff4001860) at ghash.c:1316 at gdbusmenumodel.c:256 at gdbusmenumodel.c:388 at gdbusmenumodel.c:792 Note that we can't use non-NULL bus name here, since it hits an assertion in g_dbus_connection_signal_subscribe(): g_return_val_if_fail (sender == NULL || (g_dbus_is_name (sender) && (connection->flags & G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION)), 0);
Created attachment 264126 [details] [review] tests/gmenumodel: Add a test case using peer-to-peer D-Bus connection
Created attachment 264127 [details] [review] GDBusMenuModel: Allow NULL bus name for peer-to-peer connection
Oops, lines beginning with '#' have been omitted when pasting. The complete backtrace is: Program terminated with signal 11, Segmentation fault.
+ Trace 232917
Created attachment 264129 [details] [review] GDBusMenuModel: Allow NULL bus name for peer-to-peer connection
Review of attachment 264129 [details] [review]: Thanks for the patch (and the testcase to go with). I feel like we should probably have some assertion check on entry to g_dbus_menu_model_get() that if the bus name is NULL then the connection is indeed peer-to-peer. We then need improved docs, with an allow-none annotation. You can find out if a connection is peer-to-peer by way of g_dbus_connection_get_unique_name().
Created attachment 264257 [details] [review] tests/gmenumodel: Add test cases using peer-to-peer D-Bus connection -- Run roundtrip and subscriptions tests on the peer-to-peer connection.
Created attachment 264258 [details] [review] GDBusMenuModel: Allow NULL bus name for peer-to-peer connection -- Added (allow-none) annotation and the assertion.
Created attachment 264259 [details] [review] GDBusActionGroup: Allow NULL bus name for peer-to-peer connection
Created attachment 264260 [details] [review] GMenuExporter: Allow NULL bus name for peer-to-peer connection -- While adding more tests, I noticed that the exporter side needs similar treatment (sorry, it was not the case in my personal use-case, which was not fully peer-to-peer).
Ryan, should this finally land ?
Review of attachment 264257 [details] [review]: Nice.
Review of attachment 264258 [details] [review]: I’ll fix this annotation before pushing. ::: gio/gdbusmenumodel.c @@ +852,3 @@ * g_dbus_menu_model_get: * @connection: a #GDBusConnection + * @bus_name: (allow-none): the bus name which exports the menu model (allow-none) is now (nullable).
Review of attachment 264259 [details] [review]: I’ll fix this annotation before pushing. ::: gio/gdbusactiongroup.c @@ +470,3 @@ * g_dbus_action_group_get: * @connection: A #GDBusConnection + * @bus_name: (allow-none): the bus name which exports the action (allow-none) has since been replaced by (nullable).
Review of attachment 264260 [details] [review]: Looks good to me.
Thanks for the patches (especially the unit tests); all pushed (with s/allow-none/nullable/). Sorry for the delay in reviewing them. Attachment 264257 [details] pushed as c71098d - tests/gmenumodel: Add test cases using peer-to-peer D-Bus connection Attachment 264258 [details] pushed as e8a09b3 - GDBusMenuModel: Allow NULL bus name for peer-to-peer connection Attachment 264259 [details] pushed as d37af2b - GDBusActionGroup: Allow NULL bus name for peer-to-peer connection Attachment 264260 [details] pushed as f29065c - GMenuExporter: Allow NULL bus name for peer-to-peer connection