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.
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.
(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.
(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.
(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.
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 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.
(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?
(In reply to comment #6) > Looks good. Fixed in git for 1.9.0 / spec 0.24, thanks
(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.
(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.
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.
(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.
(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.
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 on attachment 119435 [details] [review] Disallow unexpected replies Review of attachment 119435 [details] [review]: ----------------------------------------------------------------- Looks good to me, for what that's worth.
Comment on attachment 119435 [details] [review] Disallow unexpected replies Review of attachment 119435 [details] [review]: ----------------------------------------------------------------- Yes, I like this version.
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.