Bug 75749

Summary: spec: be clearer that non-METHOD_CALL messages never expect replies, even if !NO_REPLY
Product: dbus Reporter: Arhurs Galapovs <galap2>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: trivial    
Priority: low CC: alban.crequy, lars, msniko14, thiago, walters
Version: 1.5   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Proposed change
Expand documentation of NO_REPLY_EXPECTED
Disallow unexpected replies
Disallow unexpected replies

Description Arhurs Galapovs 2014-03-04 11:19:57 UTC
Created attachment 95087 [details]
Proposed change

dbus-message.c: dbus_message_new(int message_type) has message type provided as function parameter, but it doesn't call dbus_message_set_no_reply() for signal, message return and error dbus messages.
Comment 1 Simon McVittie 2014-03-04 11:42:10 UTC
This appears to have been the case since before 1.0, so I would be inclined to say that the NO_REPLY flag is only meaningful on message types that expect a reply (of which the only one currently defined is METHOD), and that D-Bus reimplementations like GDBus are expected to know not to reply to anything else.

Which implementation replies to non-method-calls? If such an implementation exists, it should be fixed. In particular, it must not reply to (success or failure) replies, since that could cause an infinite loop when talking to another instance of the same implementation.

The spec says:

 When an application handles a method call message, it is required to return a reply
 ...
 If a METHOD_CALL message has the flag NO_REPLY_EXPECTED, then as an optimization the application receiving the method call may choose to omit the reply message

...

 Unlike method calls, signal emissions have no replies

...

but does not explicitly say that you shouldn't reply to a (successful or error) reply.
Comment 2 Simon McVittie 2014-03-04 11:43:07 UTC
(In reply to comment #1)
> The spec [...] does not explicitly say that you shouldn't reply to a
> (successful or error) reply.

I'd consider a patch that made it explicitly say that.
Comment 3 Arhurs Galapovs 2014-03-04 12:14:08 UTC
(In reply to comment #1)
> Which implementation replies to non-method-calls? If such an implementation
> exists, it should be fixed. In particular, it must not reply to (success or
> failure) replies, since that could cause an infinite loop when talking to
> another instance of the same implementation.

Originally that problem was seen in QtDBus wrapper implementation ( QT 4.8.1 ). Message header is checked on NO_REPLY flag instead of message type. My assumption was that header should contain correct flag value no matter what. From all you wrote I can say that this assumption is wrong and problem is in QDBus.
Comment 4 Simon McVittie 2014-04-28 16:42:20 UTC
(In reply to comment #3)
> From all you wrote I can say that this assumption is wrong and problem is in
> QDBus.

Yes, it sounds that way.

I would be happy to review a spec patch that made it clearer that the message type is "more important" than the NO_REPLY flag, and that only METHOD_CALL messages can ever expect a reply, regardless of that flag.
Comment 5 Simon McVittie 2014-09-23 16:45:33 UTC
Created attachment 106749 [details] [review]
Expand documentation of NO_REPLY_EXPECTED

The message type is more important than whether NO_REPLY_EXPECTED is
set, when deciding whether a reply is expected. This documents
existing practice in at least libdbus, GDBus and dbus-daemon.

---

(In reply to comment #4)
> I would be happy to review a spec patch that made it clearer

Since it appears that isn't going to happen, I wrote one myself. Reviews would be appreciated.
Comment 6 Thiago Macieira 2014-09-24 16:42:30 UTC
Comment on attachment 106749 [details] [review]
Expand documentation of NO_REPLY_EXPECTED

Review of attachment 106749 [details] [review]:
-----------------------------------------------------------------

Looks good.

We should probably set the NO_REPLY_EXPECTED flag in the reference implementation too.
Comment 7 Simon McVittie 2014-09-24 16:51:37 UTC
(In reply to comment #6)
> We should probably set the NO_REPLY_EXPECTED flag in the reference
> implementation too.

Do you mean we should set it in non-method-call messages, even though we've just documented that that's meaningless?

Or do you mean we should set it in (some subset of) method-call messages, other than those where the API user has explicitly done so?
Comment 8 Simon McVittie 2014-09-24 17:08:58 UTC
(In reply to comment #6)
> Looks good.

Fixed in git for 1.9.0 / spec 0.24, thanks
Comment 9 Thiago Macieira 2014-09-24 18:43:01 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > We should probably set the NO_REPLY_EXPECTED flag in the reference
> > implementation too.
> 
> Do you mean we should set it in non-method-call messages, even though we've
> just documented that that's meaningless?
> 
> Or do you mean we should set it in (some subset of) method-call messages,
> other than those where the API user has explicitly done so?

I meant that we should set the flag for signals, method replies and method calls, just in case.
Comment 10 Simon McVittie 2014-10-01 17:12:54 UTC
(In reply to comment #9)
> I meant that we should set the flag for signals, method replies and method
> calls, just in case.

dbus_message_new_method_return() does.

dbus_message_new_signal() does.

dbus_message_new_error() does.

dbus_message_new() doesn't. I assume you think it should?

There is no API to change the type once the message has been created.
Comment 11 Lars Uebernickel 2015-11-02 15:13:09 UTC
Created attachment 119348 [details] [review]
Disallow unexpected replies

The patch was pushed a while ago as 5392058.

However, I think the current wording is awkward and effectively means that replies may never be sent anyway to avoid log messages about denied messages. I propose clearing this up by changing the spec to disallow sending replies entirely.
Comment 12 Simon McVittie 2015-11-02 15:25:26 UTC
(In reply to Lars Uebernickel from comment #11)
> However, I think the current wording is awkward

Yes. Hysterical raisins :-(

> and effectively means that
> replies may never be sent anyway to avoid log messages about denied
> messages

Only on the system bus, or custom buses resembling it (hence the wording in the spec about "non-trivial security policy"). On the session bus, or custom buses with a similarly trivial security policy, unrequested replies are fine.
Comment 13 Lars Uebernickel 2015-11-02 15:35:56 UTC
(In reply to Simon McVittie from comment #12)
> Only on the system bus, or custom buses resembling it (hence the wording in
> the spec about "non-trivial security policy"). On the session bus, or custom
> buses with a similarly trivial security policy, unrequested replies are fine.

I know, and that is my point: because unrequested replies are sometimes not fine, let's just simplify this and don't ever allow them. Libraries need to behave like this anyway, as they might be running on a restricted bus.
Comment 14 Lars Uebernickel 2015-11-06 11:10:04 UTC
Created attachment 119435 [details] [review]
Disallow unexpected replies

Amend the patch to use "should" instead of "must" and update another reference of NO_REPLY_EXPECTED.
Comment 15 Philip Withnall 2015-11-06 12:29:23 UTC
Comment on attachment 119435 [details] [review]
Disallow unexpected replies

Review of attachment 119435 [details] [review]:
-----------------------------------------------------------------

Looks good to me, for what that's worth.
Comment 16 Simon McVittie 2015-11-06 17:00:40 UTC
Comment on attachment 119435 [details] [review]
Disallow unexpected replies

Review of attachment 119435 [details] [review]:
-----------------------------------------------------------------

Yes, I like this version.
Comment 17 Simon McVittie 2015-11-06 17:37:43 UTC
Fixed in git for spec 0.27 (which will be released with dbus 1.11.0).

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.