Created attachment 81311 [details] [review]
[PATCH] DBus Specification: unknown key in match rule must be  ignored

Created attachment 81313 [details] [review]
[PATCH v2] DBus Specification: unknown key in match rule must be  ignored

As I said on the mailing list, match rule keys are not header field codes.

This is a feature request: "change the D-Bus Specification so that unknown match rule keys are ignored". It would make all known versions of dbus-daemon instantly non-compliant with that specification.

(In reply to comment #3)
> As I said on the mailing list, match rule keys are not header field codes.

Yes, I known that, just want to say this is just like the message header situation, sorry for my english skill.

> 
> This is a feature request: "change the D-Bus Specification so that unknown
> match rule keys are ignored". It would make all known versions of
> dbus-daemon instantly non-compliant with that specification.

I'm so happy to see that you agree with me.

>On 24/06/13 08:26, Yang Chengwei wrote:
>> Hi List,
>
>No need to Cc me - oddly enough, the person who has done most of the
>D-Bus releases in the last 2 years does read the mailing list :-)
>
>> I just found that the *new* dbus-monitor can't run with the *old" 
>> dbus-daemon.
>
>"Old" here means 1.4, which is older than the current stable branch. I

Yes, that is 1.4.18 on ubuntu.

>don't support older-than-current-stable D-Bus versions, except for
>non-intrusive security fixes.
>
>> Error: Unknown key "eavesdrop" in match rule
>> 
>> This is cause by "eavesdrop" is newly added to dbus to fix an
>> issue: https://bugs.freedesktop.org/show_bug.cgi?id=37890
>
>The right way to fix this would be to do the AddMatch with eavesdrop,
>and if it fails, try again without it - like Bustle does[1]. I'd
>accept a patch that did this, if you think compatibility with D-Bus
>1.4 is really that important.
>
>[1] http://cgit.freedesktop.org/bustle/tree/c-sources/pcap-monitor.c
>

Sure, will do so for dbus-monitor.

>> Again, if an implementation sees a header field code that it does
>> not expect, it must ignore that field, as it will be part of a new
>> (but compatible) version of this specification.
>
>This is specifically talking about header fields: if it was meant to
>be about other constructs, it would say so.
>
>> So I think it's applicable to the match rule, say: ignore unknown
>> keys in match rule.
>
>If you think the D-Bus Specification should have similar wording about
>match rules too, that's a reasonable request to consider.

Yes, that's what I did mean.

>
>In general, adding keys to a match rule makes it narrower, so in
>general, adding an unrecognised key would result in the client
>receiving more messages than it had intended to. In principle, clients
>should consider match rules to be advisory, not mandatory, so in
>principle, it should be prepared to ignore those extra messages. In
>practice, there have been clients that are buggy if they receive
>unwanted messages.
>
>So, it's a question of which of these is more valuable:
>
>* making it fractionally easier to add more match rule keys (clients
>  wouldn't have to "guard" their AddMatch calls, if they can depend on
>  a D-Bus version new enough that it ignores unknown keys)
>
>* minimizing how much we break the expectations of clients that wrongly
>  expect their match rules to be exclusive

Yes, as you said, it's *wrongly*.

>
>If we make this change to the specification, it won't change the
>behaviour of currently-deployed copies of D-Bus, so clients using new
>match rules will *still* have to "guard" AddMatch calls until they can
>depend on a new enough D-Bus version that it has the "ignore unknown
>match rules" behaviour. I think that's the sort of behavioural change
>that should go through a development branch (1.7), meaning it won't be
>widespread until 1.8.

That's fine for me.

As you said in comment 3, we'll adopt this change, although this change will make all distributed dbus-daemon not compliant with the new specification.

>
>    S

(In reply to comment #5)
> As you said in comment 3, we'll adopt this change

I didn't say that, I said this was a feature request. There are two possible resolutions for a feature request: "yes, add it" and "no, we don't want that".

I'm not going to make this change until/unless people have had a chance to respond on the mailing list. I don't have any particular objection to it, but the other maintainers might.

Several patches for dbus-monitor uploaded at #bug66107, includes the one keep backwards compatibility with old dbus which hasn't eavesdropping as a match rule key.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org'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.freedesktop.org/dbus/dbus/issues/84.