| Summary: | should allow duplicate object registrations | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Colin Walters <walters> |
| Component: | GLib | Assignee: | Will Thompson <will> |
| Status: | RESOLVED FIXED | QA Contact: | John (J5) Palmieri <johnp> |
| Severity: | normal | ||
| Priority: | medium | CC: | dcbw, mbiebl, zeuthen |
| Version: | unspecified | Keywords: | patch |
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
Support duplicate object registrations
this time without g_printerr Support duplicate object registrations Only re-set registration list if it's non-empty Copy object registration list when unregistering. |
||
Created attachment 28791 [details] [review] Support duplicate object registrations Created attachment 28792 [details] [review] this time without g_printerr Hmm, not quite... quick testing shows that: [dcbw@localhost devel]$ dbus-send --session --print-reply --dest=org.freedesktop.PolicyKit.AuthenticationAgent /org/freedesktop/PolicyKit/Manager org.freedesktop.PolicyKit.AuthenticationAgent.ObtainAuthorization string:org.freedesktop.network-manager-settings.system.modify uint32:0 uint32:123 Error org.freedesktop.DBus.Error.UnknownMethod: Method "ObtainAuthorization" with signature "suu" on interface "org.freedesktop.PolicyKit.AuthenticationAgent" doesn't exist [dcbw@localhost devel]$ dbus-send --session --print-reply --dest=org.freedesktop.PolicyKit.AuthenticationAgent / org.freedesktop.PolicyKit.AuthenticationAgent.ObtainAuthorization string:org.freedesktop.network-manager-settings.system.modify uint32:0 uint32:123 method return sender=:1.251 -> dest=:1.252 reply_serial=2 boolean true whereas I'd expect both of these commands (which differ only by object path) to return the exact same result. Introspect also doesn't work for the first registration: [dcbw@localhost dbus]$ dbus-send --session --print-reply --dest=org.freedesktop.PolicyKit.AuthenticationAgent /org/freedesktop/PolicyKit/Manager org.freedesktop.DBus.Introspectable.Introspect method return sender=:1.268 -> dest=:1.270 reply_serial=2 string "<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN" "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd"> <node> </node> " while using "/" as the path does. Trying to track this down. Ignore all that; works fine. I had the object path wrong. Shouldn't object_registration_free() only re-set_data on the object if the list is non-NULL? (Obviously this is trivia.) I think that when _unregister_g_object() calls _unregister_object_path(), the latter causes object_registration_free() to be called, which deletes the link from 'registrations' that 'iter' is currently pointing at. Then 'iter = iter->next' runs, and is dereferencing freed memory. I think advancing 'iter' before calling dbus_connection_unregister_object_path() would work, but wouldn't be obviously correct; maybe copy the list, iterate and free the copy, and then assert that g_object_get_data (object, "dbus_glib_object_registrations"); returns NULL? (It took me a couple of minutes to figure out how the ObjectRegistrations were being freed, so it could do with a comment either way.) (The patch adds some trailing whitespace and tabs even as it diligently removes other trailing whitespace and tabs. :)) (Stealing the bug from Simon.) Is there progress on this bug, is the patch the correct fix or does it need further review? Well, I'm pretty sure it uses freed memory; I'll knock up a couple more patches fixing my review comments. Created attachment 29445 [details] [review] Support duplicate object registrations Before commit e869fda4, we semi-supported registering the same object multiple times. We'd accept messages for both paths, however when signals were emitted, they'd both use the first object path. That commit simply disallowed multiple registrations, which broke backwards compatibility with some projects like PolicyKit which had the same object registered with different paths. With this commit, explicitly allow and support multiple registrations. The primary change is that signals are now emitted once for each registration path of an object, using the correct path. Created attachment 29446 [details] [review] Only re-set registration list if it's non-empty Created attachment 29447 [details] [review] Copy object registration list when unregistering. Since the list of registrations on the object is modified when each path is removed, iterating it directly is wrong: after the first pass of the loop, 'iter' would point to a link which has been freed. Comment on attachment 28792 [details] [review] this time without g_printerr I've attached an updated version of this patch which does not add trailing whitespace. The attached patches (also at <http://git.collabora.co.uk/?p=user/wjt/dbus-glib-wjt.git;a=shortlog;h=refs/heads/duplicate-registrations>) fix the problems I found while reviewing. I think they're good to go; could someone (Colin?) review my fixes and commit them? Reviewing... Looks good to me with Will's changes (i.e. commit 90e2199a), so I pushed it. Fixed in git for 0.84. Where did you push these changes? I don't see them on http://cgit.freedesktop.org/dbus/dbus-glib/ Well spotted. *Really* fixed in git now :-) |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
For backwards compat, PolicyKit does: dbus_g_connection_register_g_object (manager->priv->session_bus_connection, "/org/gnome/PolicyKit/Manager", G_OBJECT (manager)); dbus_g_connection_register_g_object (manager->priv->session_bus_connection, "/", G_OBJECT (manager)); This broke with 0.82, commit 6de1441865da281. We should probably allow duplicate registrations for dispatch purposes. When mapping back from object->path, simply have first one wins behavior.