GNOME Bugzilla – Bug 706675
missing implementation for null interface method calls
Last modified: 2018-05-24 15:38:17 UTC
Created attachment 252890 [details] [review] Patch v1 gdbus does not implement method calls without interface being set in message, as http://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-types Attached: A fix for that issue, tested using the dbus-demo https://github.com/drahnr/gdbus-demo
Created attachment 252891 [details] [review] Minor comment removal Related: Remove misleading TODO comments, GDBusMethodInfo caching is already implemented via GDBusInterfaceInfo call g_dbus_interface_info_cache_build .
Created attachment 253134 [details] [review] [v2, 1/3] Remove misleading TODOs, already implemented.
Created attachment 253135 [details] [review] [v2, 2/3] Comply to method call specification, allow omitted interface field.
Created attachment 253136 [details] [review] [v2, 3/3] gdbusconnection: add lazy NULL interface caching
Do you need this for something that you're doing? Despite what the spec says, sending no interface is not really common practice anywhere and is widely believed to have been a mistake...
Yes, I am working with some (closed source) libraries which do exactly that, and it is not in my power to change that. I do not think that it is common either, thus the `G_UNLIKELY` flag, but I think following the spec should be a higher priority than implementing what is believed/common sense here - a spec is a spec, not a experimental playground. qtdbus obides that, so does libdbus-1, and gdbus should too. Anything codewise speaking against inclusion?
Review of attachment 253134 [details] [review]: (I renamed your patches from "patch #1" to their git Subject, to make them easier to talk about.) I have no opinion on this patch; if the TODO is really obsolete (I haven't checked), it seems good to remove it.
Review of attachment 253135 [details] [review]: I would prefer this patch not to be merged in its current form. ::: gio/gdbusconnection.c @@ +5027,3 @@ + /* iterate over all interfaces and check if a member method exists + * on the first occurence, continue If it is necessary for the service-side of GDBus to do this at all, then I think it should make the ambiguous case mentioned in the spec (see my mailing list post) be an error. However, I think needing this functionality is a bug, and I'm tempted to change dbus-specification to not require it to work. If you really, really need to respond to NULL-interface calls to work around a broken module that cannot be changed, you can do so with the lower-level API g_dbus_connection_add_filter(). @@ +5547,3 @@ g_return_if_fail (bus_name == NULL || g_dbus_is_name (bus_name)); g_return_if_fail (object_path != NULL && g_variant_is_object_path (object_path)); + g_return_if_fail (interface_name == NULL || g_dbus_is_interface_name (interface_name)); You can make client-side "no-interface" calls with g_dbus_message_new_method_call() and the g_dbus_connection_send_message*() family, if you really need them. I think it's fine for higher-level APIs like the g_dbus_connection_call*() family to only support the 99% case, especially if that helps to guide their users to a better design.
Review of attachment 253136 [details] [review]: This is a rare case which should only be reached with poorly-designed D-Bus APIs, and I don't think adding any complexity to optimize it is justified.
(In reply to comment #6) > Yes, I am working with some (closed source) libraries which do exactly that, > and it is not in my power to change that. Are these libraries buggy on the client side (they call methods on an object you export from GDBus), or on the service side (they export an object and you call its methods with GDBus), or both? > I think following the spec should be a higher priority than implementing what > is believed/common sense here - a spec is a spec, not a experimental > playground. A spec is a spec, but it isn't sacred. If the spec conflicts with reality, we should change it. In the reference implementation of D-Bus (libdbus/dbus-daemon), no-interface method calls will rarely work as intended on the system bus - and if they did, it would be a security flaw. I think the spec should reflect that.
(In reply to comment #10) > (In reply to comment #6) > > Yes, I am working with some (closed source) libraries which do exactly that, > > and it is not in my power to change that. > > Are these libraries buggy on the client side (they call methods on an object > you export from GDBus), or on the service side (they export an object and you > call its methods with GDBus), or both? > They call methods I export. > > I think following the spec should be a higher priority than implementing what > > is believed/common sense here - a spec is a spec, not a experimental > > playground. > > A spec is a spec, but it isn't sacred. If the spec conflicts with reality, we > should change it. The change should be made to the actual spec, not its implementation. Also in my case that will not change anything. > In the reference implementation of D-Bus (libdbus/dbus-daemon), no-interface > method calls will rarely work as intended on the system bus - and if they did, > it would be a security flaw. I think the spec should reflect that. It should.
(In reply to comment #11) > They call methods I export. My advice would be to use a message filter to catch those ambiguous method calls, and ask the vendor of this closed-source library to use the unambiguous form in future. > The change should be made to the actual spec, not its implementation. https://bugs.freedesktop.org/show_bug.cgi?id=68597
(In reply to comment #12) > (In reply to comment #11) > > They call methods I export. > > My advice would be to use a message filter to catch those ambiguous method > calls, and ask the vendor of this closed-source library to use the unambiguous > form in future. There is infrastructure in glib handling _all_ of this - properly and fast - this will lead to reimplementation for no sane reason. Asking a company with limited manpower to change a property of a production system in a potentially backwards incompatible way (some might have used different interface names exporting their stuff) will (I bet a 100$ on that) not yield anything than a short "no, we are obiding the [current] spec", be it bad practice or not.
(In reply to comment #12) > (In reply to comment #11) > > They call methods I export. > > My advice would be to use a message filter to catch those ambiguous method > calls, and ask the vendor of this closed-source library to use the unambiguous > form in future. Apology, I overlooked the `in future`. Forget about the 2nd paragraph of my last response to comment #12
So, how shall we proceeed? I'd (still) be happy to get this upstream. Is there anything that needs to be done beyond adding error return on member ambiguity? Or is this bug already implicitly closed for you?
-- 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/glib/issues/748.