GNOME Bugzilla – Bug 755421
GDBus ignores NO_REPLY_EXPECTED flag in messages, leading to warnings on system bus
Last modified: 2016-09-07 15:46:43 UTC
GDBus services written in the obvious way do not respect the NO_REPLY_EXPECTED flag in method call messages. The D-Bus Specification specifically says that this is valid. However, on the system bus (with its deny-by-default policy), it will cause warnings, because the bus policy allows expected replies but not unsolicited "replies". If a method call has NO_REPLY_EXPECTED, then the dbus-daemon will not set up the necessary infrastructure to remember that a reply should be allowed (because it would never be able to free it); later, it will forbid the reply with a warning. I think everything that wraps g_dbus_method_invocation_return_value_internal() or calls g_dbus_message_new_method_reply() should check for this flag, and silently skip the actual message sending if NO_REPLY_EXPECTED. As an optimization, it could also skip as much as possible of the message composition. However, I think making g_dbus_message_new_method_reply() check the flag would be "too much magic". Because of the slightly odd RAII-like calling convention where completing an invocation is (transfer full) (Bug #749533), it is possible that the intention was that method implementors should use this pattern (pseudocode): if (call.NO_REPLY_EXPECTED) invocation.unref() else invocation.return_value(()) However, that seems rather unexpected to me - it makes system service implementors write boilerplate in each of their methods, while also making GDBus' encapsulation rather leaky. For comparison, the deprecated dbus-glib fixed this as <https://bugs.freedesktop.org/show_bug.cgi?id=19441>.
Created attachment 314485 [details] [review] gdbus: don't send unexpected replies We're seeing a lot of log spam from accountsservice because of this. I agree that it makes sense to handle this in gdbus, as that also sets NO_REPLY_EXPECTED automatically.
Review of attachment 314485 [details] [review]: ::: gio/gdbusmethodinvocation.c @@ +402,3 @@ + g_variant_unref (parameters); + } + return; Shouldn't this "goto out" to unref the invocation?
Created attachment 314507 [details] [review] gdbus: don't send unexpected replies > Shouldn't this "goto out" to unref the invocation? Ah indeed, thanks. Transfer full on in-args is weird. Same for g_dbus_method_invocation_return_dbus_error().
I suggest this grows a unit test as well, since D-Bus is widely used.
See also bug 741397 for some things that ought to be considered.
My official take on this bug: What we are doing now is correct behaviour according to the spec and, further, is what the user is asking us to do via the API. Changing this behaviour would mean that we are now selectively ignoring the user's instructions to us, and with no support from the spec for the necessity of doing so. It seems that "it is difficult to do the necessary bookkeeping in the daemon to make this truly optional" is a good reason to change the spec to mandate this behaviour, in which case I'm happy to change GLib to refuse the user's request and silently drop the message. Otherwise, I suggest we add a new API for the user to find out is a reply is required. Bug 741397 talks about that. In that case, I expect that dbus-daemon would either be modified to do some fancy state tracking or (more realistically) simply not log unexpected replies as errors. I slightly favour the spec change.
(In reply to Allison Ryan Lortie (desrt) from comment #6) > I slightly favour the spec change. I agree. I've attached a new patch to the dbus bug that introduced this awkward wording: https://bugs.freedesktop.org/show_bug.cgi?id=75749
(In reply to Allison Ryan Lortie (desrt) from comment #6) > It seems that "it is difficult to do the necessary bookkeeping in the daemon > to make this truly optional" is a good reason to change the spec to mandate > this behaviour For the session bus, unrequested replies are pointless but OK; it's the system bus where they become actively a problem. The designed security properties of the system bus make it impossible to do the necessary bookkeeping in the dbus-daemon to not get these warnings, unless we silently drop messages on the floor sometimes. We issue warnings whenever a sender is not allowed to send a message to a recipient, because in general that indicates that either we are dropping a message that should have got through (misconfiguration by the sysadmin), or someone is trying to send a message that should not get through (potential attack). If we suppress these warnings, that means there's a class of messages that we're dropping silently, which seems potentially undesirable. We shouldn't let "replies" through unless they are genuinely a (single) reply to a method call that was sent in the past, because if we did, that would be an unintended side-channel between programs on the bus. For messages with NO_REPLY_EXPECTED, if we remember the method call so that we could allow its optional reply through, but the callee chooses to take advantage of the optimization that it was allowed to make and not send a reply, then the method call will use up one of the caller's maximum pending method calls indefinitely. Eventually, it'll run out. We set a maximum on the pending method calls because if we didn't, we'd use unbounded memory for this bookkeeping. (In reply to Allison Ryan Lortie (desrt) from comment #6) > and, further, is what the user is asking us to do via the API I can see where you're coming from, but I think the proposed change here makes sense if you look at the APIs from a slightly different angle: * A /method invocation/ is high-level (object-oriented) API for a D-Bus method call. It /returns/ either success or an error, exactly once. * A /method call message/ is low-level (message-passing-oriented) API for a class of D-Bus messages. It may be answered with a successful reply, or an error reply, or nothing. This is somewhat analogous to how, if you are doing an asynchronous task, you can allocate a GTask to track the lifetime/state/context of the task, even if the callback you were given is NULL. You can still g_task_return() on a task that has no callback, and the result will be ignored.
The other side of "NO_REPLY_EXPECTED is just an optimization" is that I don't want to encourage high-level code in services to make its own decision about whether to return from the method invocation or not. If they do, it seems somewhat likely that they will implement certain methods to never reply at all, annotating them with org.freedesktop.DBus.Method.NoReply, because "surely nobody will ever call this with a reply expected" - but well-behaved D-Bus services are expected to be prepared to reply to any method call. If services have to "return" each GDBusMethodInvocation exactly once, then the obvious implementation of a method where the reply is meaningless would be to "return" the GDBusMethodInvocation immediately, and then do the actual work afterwards, which does the right thing both with and without NO_REPLY_EXPECTED. In particular, if a naive client (with no support for ignoring replies, or using a sync calling convention) calls a method that is implemented to never reply, the dbus-daemon will continue to wait for a reply, using up a pending-reply slot. At the moment, with Bug #741397, it would be a memory leak (regardless of the value of NO_REPLY_EXPECTED) if a service didn't either return from the method call or (undocumented usage) unref the invocation, which at least means valgrind or refdbg might pick up that error.
(In reply to Simon McVittie from comment #8) > We shouldn't let "replies" through unless they are genuinely a (single) > reply to a method call that was sent in the past, because if we did, that > would be an unintended side-channel between programs on the bus. Sure -- but almost certainly a harmless one. Excepting some very serious security problems with the D-Bus implementation that you're sending this message to, or weird use of message filters, the message will be dropped on the floor anyway, as soon as it is received. > For messages with NO_REPLY_EXPECTED, if we remember the method call so that > we could allow its optional reply through, but the callee chooses to take > advantage of the optimization that it was allowed to make and not send a > reply, then the method call will use up one of the caller's maximum pending > method calls indefinitely. Eventually, it'll run out. We set a maximum on > the pending method calls because if we didn't, we'd use unbounded memory for > this bookkeeping. I agree. This is why I don't propose to change the daemon to do this. > I can see where you're coming from, but I think the proposed change here > makes sense if you look at the APIs from a slightly different angle: > > * A /method invocation/ is high-level (object-oriented) API for a D-Bus > method call. It /returns/ either success or an error, exactly once. > > * A /method call message/ is low-level (message-passing-oriented) API > for a class of D-Bus messages. It may be answered with a successful > reply, or an error reply, or nothing. > > This is somewhat analogous to how, if you are doing an asynchronous task, > you can allocate a GTask to track the lifetime/state/context of the task, > even if the callback you were given is NULL. You can still g_task_return() > on a task that has no callback, and the result will be ignored. This is a fine analogy, but it has always been part of the GIO async convention that a NULL callback is always valid and the caller must deal with that by _not_ calling the NULL callback. This is mandatory and it has always been that way. There is no wording about "if the callback is NULL then the implementation may choose to call it if it wants to see what will happen...". One thing that I find interesting about your argument above is that you are effectively saying that the service -must- use g_dbus_method_invocation_return_*() exactly once, even if it knows that the sender does not expect a reply. I agree that this is very nice from an aesthetic/theoretical standpoint, but there may be reasons that the caller does not want to do this. See below. (In reply to Simon McVittie from comment #9) > The other side of "NO_REPLY_EXPECTED is just an optimization" is that I > don't want to encourage high-level code in services to make its own decision > about whether to return from the method invocation or not. Making a simple GVariant (that will be immediately thrown away) is probably not super-expensive, but there may be cases where it will be. The message may be large. It may contain data that is expensive to calculate or fetch. I've wanted for some time to make it easy for methods not to send a reply. > If they do, it seems somewhat likely that they will implement certain > methods to never reply at all, annotating them with > org.freedesktop.DBus.Method.NoReply, because "surely nobody will ever call > this with a reply expected" - but well-behaved D-Bus services are expected > to be prepared to reply to any method call. If services have to "return" > each GDBusMethodInvocation exactly once, then the obvious implementation of > a method where the reply is meaningless would be to "return" the > GDBusMethodInvocation immediately, and then do the actual work afterwards, > which does the right thing both with and without NO_REPLY_EXPECTED. This viewpoint is currently supported by the documentation: """ Ownership of the GDBusMethodInvocation object passed to the method_call() function is transferred to your handler; you must call one of the methods of GDBusMethodInvocation to return a reply (possibly empty), or an error. """ This does not mention that it is valid to simply unref() the thing, so your above interpretation is correct as per the docs. Actual practice may be another issue... and in any case, I would like to make this possible [to simply drop the ref]. > In particular, if a naive client (with no support for ignoring replies, or > using a sync calling convention) calls a method that is implemented to never > reply, the dbus-daemon will continue to wait for a reply, using up a > pending-reply slot. Sure. I completely agree that a service _must_ send replies if the client has not set the no-reply flag. To not do so would simply be an error. The availability of an API for a service to request if a reply is required is not going to make it any _less_ likely for a service to send replies. > At the moment, with Bug #741397, it would be a memory leak (regardless of > the value of NO_REPLY_EXPECTED) if a service didn't either return from the > method call or (undocumented usage) unref the invocation, which at least > means valgrind or refdbg might pick up that error. Sure. We're not really arguing about that. I don't see any strong argument in any of the above for why honoring the no-reply-expected flag should be optional. There is no additional burden placed on the implementation of dbus-daemon by more strictly specifying the behaviour of the client libraries and the additional burden placed on the client libraries is, in fact, exactly what you are asking for now. In short: the maintainer of a spec is filing a bug against an implementation of that spec and saying "your behaviour is perfectly compliant with the spec, but I consider it to be wrong". If you consider the current (spec-compliant) behaviour to be wrong then why don't you change the spec?
(In reply to Allison Ryan Lortie (desrt) from comment #10) > In short: the maintainer of a spec is filing a bug against an implementation > of that spec and saying "your behaviour is perfectly compliant with the > spec, but I consider it to be wrong". If you consider the current > (spec-compliant) behaviour to be wrong then why don't you change the spec? => https://bugs.freedesktop.org/show_bug.cgi?id=75749
Review of attachment 314507 [details] [review]: Looks good to me. Would it also make sense to clarify somewhere in the documentation that if you're using GDBusMethodInvocation, you must call g_dbus_method_invocation_return_*() regardless of whether the GDBusMessage has G_DBUS_MESSAGE_FLAGS_NO_REPLY_EXPECTED set, unless you like memory leaks?
The spec change was accepted into D-Bus. Can we push this patch now? (In reply to Philip Withnall from comment #12) > Looks good to me. Would it also make sense to clarify somewhere in the > documentation that if you're using GDBusMethodInvocation, you must call > g_dbus_method_invocation_return_*() regardless of whether the GDBusMessage > has G_DBUS_MESSAGE_FLAGS_NO_REPLY_EXPECTED set, unless you like memory leaks? Where do you feel the such a comment is missing? I think it's already documented quite well for in method_call on GDBusInterfaceVTable. (You don't have to call any of the return_* functions if you unref the invocation yourself.)
(In reply to Lars Uebernickel from comment #13) > Where do you feel the such a comment is missing? I think it's already > documented quite well for in method_call on GDBusInterfaceVTable. > > (You don't have to call any of the return_* functions if you unref the > invocation yourself.) Maybe in the section introduction for GDBusMethodInvocation? I doubt it will be found there, but that’s the best I can come up with. Maybe also in the documentation for g_dbus_connect_register_object(), since that’s the API people are most likely to use to (eventually) get a GDBusMethodInvocation (unless they’re using gdbus-codegen, but that handles the refcounting correctly already).
Created attachment 318974 [details] [review] GDBusMethodInvocation: document behaviour change We changed the behaviour of this API to adapt to a change in the D-Bus specification. Document the new behaviour, along with the time of the change.
Thanks!
*** Bug 765995 has been marked as a duplicate of this bug. ***