Bug 19441 - reply sent even for no_reply messages
reply sent even for no_reply messages
Status: RESOLVED FIXED
Product: dbus
Classification: Unclassified
Component: GLib
unspecified
Other All
: medium normal
Assigned To: Rob Taylor
John (J5) Palmieri
:
: 7211 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-07 07:02 UTC by Colin Walters
Modified: 2010-10-19 06:10 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
don't send unrequested replies (4.27 KB, patch)
2009-01-07 07:03 UTC, Colin Walters
Details | Splinter Review
updated for comments (5.71 KB, patch)
2009-01-15 11:55 UTC, Colin Walters
Details | Splinter Review
also handle error case (6.17 KB, patch)
2009-01-16 09:46 UTC, Colin Walters
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Walters 2009-01-07 07:02:18 UTC
The glib bindings are sending a reply message even if a message has the no_reply flag set.  This is normally rather harmless (if slightly inefficient), but with the security policy fixes for the system bus, they will show up in the logs as denied unrequested replies.

Better to not send them at all.
Comment 1 Colin Walters 2009-01-07 07:03:34 UTC
Created attachment 21749 [details] [review]
don't send unrequested replies
Comment 2 Simon McVittie 2009-01-15 09:09:55 UTC
I don't think I'm the right person to review this, but since mbiebl asked me to look at it...

The variable name "use_thread" is completely untrue. "A" in the method data stands for asynchronous - "use_thread" would be better named as "reply_later" or the current "call_only". dbus-glib never invents extra threads.

Methods with "A" in the method data use a different calling convention, where the method implementation takes an extra parameter (a pointer to a DBusGMethodInvocation) and returns nothing. Instead of the return value from the method implementation being used to send the reply, the reply is only sent when a reply function (dbus_g_method_return() or dbus_g_method_return_error(), iirc) is called on the DBusGMethodInvocation (this can be done before or after returning from the method implementation).

As a result, for a complete fix, the call_only code path needs to save whether the message was no-reply (or perhaps just the entire message) into the DBusGMethodInvocation, and use that information to avoid emitting a message from dbus_g_method_return() or dbus_g_method_return_error().

telepathy-glib exclusively uses the "async" calling convention, since some of our methods need to wait for some network traffic (returning to the main loop to process more messages while waiting for it) and *then* reply, and for the rest we might as well be consistent - (the "async" calling convention is no harder to use than the default one).
Comment 3 Colin Walters 2009-01-15 11:54:49 UTC
Thanks for the review; you're absolutely right.  The thread naming was me being confused with the patch in http://bugs.freedesktop.org/show_bug.cgi?id=18972 .

I've fixed that up, added a comment, and also fixed the async case by carrying through the send_reply boolean into DBusGMethodInvocation.
Comment 4 Colin Walters 2009-01-15 11:55:11 UTC
Created attachment 22015 [details] [review]
updated for comments
Comment 5 Simon McVittie 2009-01-16 04:26:13 UTC
I don't think you've covered dbus_g_method_return_error(), which is the other way to emit an async reply message for a DBusGMethodInvocation.
Comment 6 Colin Walters 2009-01-16 09:46:51 UTC
Created attachment 22034 [details] [review]
also handle error case

Thanks, fixed.
Comment 7 Colin Walters 2009-01-27 15:44:26 UTC
commit d92a44109e3fdc766e34b53f7ec5329e98e13909
Author: Colin Walters <walters@verbum.org>
Date:   Tue Jan 27 17:00:37 2009 -0500

    Bug 19441: Don't send replies for messages explicitly not requesting one
    
    In sending a reply when a message has the dbus_message_set_no_reply
    flag set, we can cause spurious denials logged on the system bus, aside
    from being inefficient.
Comment 8 Colin Walters 2009-02-25 08:33:28 UTC
*** Bug 7211 has been marked as a duplicate of this bug. ***
Comment 9 David Zeuthen (not reading bugmail) 2010-08-20 11:27:42 UTC
(I just saw this bug referenced on IRC so late to the party etc. etc.)

Anyway, here's my view: If the system bus is causing "spurious denials" then it is the system bus that is broken. The specification *specifically* allows for such replies to be sent. I quote

 This message does not expect method return replies or error
 replies; the reply can be omitted as an optimization. However,
 it is compliant with this specification to return the reply
 despite this flag and the only harm from doing so is extra
 network traffic.

An example of where this extra traffic is useful is message tracing and other tools where a 3rd party wants to listen in.

Frankly, I'd rather we'd fix the bus to behave properly than trying to make our bindings overly "clever" and "optimized".
Comment 10 Colin Walters 2010-10-18 10:46:31 UTC
David, the main issue I see is that it's problematic for debugging if the bus just silently drops messages.

I'd be fine with special casing no logging for an unrequested reply denial though.
Comment 11 David Zeuthen (not reading bugmail) 2010-10-18 11:53:10 UTC
(In reply to comment #10)
> David, the main issue I see is that it's problematic for debugging if the bus
> just silently drops messages.

In what situation would the bus silently drop the message? I mean, if someone really replies to a NO_REPLY_REQUESTED message.. then the bus should honor that and deliver the reply. The bus must not second guess.
Comment 12 Simon McVittie 2010-10-19 02:58:40 UTC
(In reply to comment #11)
> In what situation would the bus silently drop the message? I mean, if someone
> really replies to a NO_REPLY_REQUESTED message.. then the bus should honor that
> and deliver the reply. The bus must not second guess.

Conceptually, NO_REPLY_REQUESTED is an optimization, which can have any or all of the following effects. Ideally, it has *all* of these effects:

- The client process doesn't allocate memory to track the reply (e.g. DBusPendingCall, or some similar entry in a serial number => closure hash table)
- The bus daemon doesn't allocate memory to track the reply and allow it through
- The bus daemon doesn't set a timeout and, when it expires, synthesize a "timed out" message back to the sender
- The server process doesn't reply

If the bus daemon wants to allow replies, it needs to remember which serial number from which sender is waiting for a reply, and it needs to associate timeouts with these so they don't pile up forever if the reply never arrives. Requiring it to do this for NO_REPLY_REQUESTED messages would defeat (a relatively large) part of the optimization value of the flag.
Comment 13 Colin Walters 2010-10-19 06:10:23 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > David, the main issue I see is that it's problematic for debugging if the bus
> > just silently drops messages.
> 
> In what situation would the bus silently drop the message?

This comes out of the generic security policy checking code.  The system bus configuration has:

    <allow send_requested_reply="true" send_type="method_return"/>
    <allow send_requested_reply="true" send_type="error"/>

Now, we could brute force this and simply not log denials for method_return or error type messages.  Clearly the main problems people run into are method calls, because those are what are denied by default.