Bug 37890 - accidentally eavesdropping on the session bus is too easy, leading to foot-shooting incidents
accidentally eavesdropping on the session bus is too easy, leading to foot-sh...
Status: RESOLVED FIXED
Product: dbus
Classification: Unclassified
Component: core
unspecified
Other All
: medium normal
Assigned To: Cosimo Alfarano
John (J5) Palmieri
NB#269748
:
Depends on:
Blocks: 39140
  Show dependency treegraph
 
Reported: 2011-06-03 12:42 UTC by David Zeuthen (not reading bugmail)
Modified: 2011-07-21 09:04 UTC (History)
6 users (show)

See Also:


Attachments
Add eavesdrop=true|false to match rules. (7.65 KB, patch)
2011-07-11 03:57 UTC, Cosimo Alfarano
Details | Splinter Review
Enables dbus-monitor to automatically use eavesdrop=true (1.44 KB, patch)
2011-07-11 04:00 UTC, Cosimo Alfarano
Details | Splinter Review
[test] first part of the test (1.78 KB, text/x-python)
2011-07-11 06:44 UTC, Cosimo Alfarano
Details
[test] second part of the test (2.22 KB, text/x-python)
2011-07-11 06:50 UTC, Cosimo Alfarano
Details
Add eavesdrop=true|false to match rules (7.97 KB, patch)
2011-07-11 07:42 UTC, Cosimo Alfarano
Details | Splinter Review
Add eavesdrop=true to dbus-monitor (2.14 KB, patch)
2011-07-11 07:43 UTC, Cosimo Alfarano
Details | Splinter Review
handle no filter at cmdline case (1.35 KB, patch)
2011-07-12 09:58 UTC, Cosimo Alfarano
Details | Splinter Review
Add autometed tests for eavesdrop=true|false (16.93 KB, patch)
2011-07-14 05:50 UTC, Cosimo Alfarano
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Zeuthen (not reading bugmail) 2011-06-03 12:42:29 UTC
It appears that eavesdropping is on by default on the session bus - at least in the upstream defaults and the Fedora build.

I just realized that eavesdropping incurs a non-trivial cost; therefore, we should probably turn it off by default when using the session bus. For extra credit, we should have a way to turn it on/off on the fly - things like dbus-monitor(1) and  http://willthompson.co.uk/bustle/ should then use this new interface.

Btw, the interface in question is the (private) interface between GIO-using applications and out-of-process GVfs volume monitors. In the normal case all N clients listen to signal from the volume monitor process (which owns a unique name, e.g. org.gtk.Private.GPhoto2VolumeMonitor). But in rare cases the volume monitor wants to send information to only one client - for example when requesting information needed to perform an operation (cf. GMountOperation). And this is implemented by D-Bus signals rather than a registration pattern and method calls.

In reality I doubt this is a big problem. But it _does_ incur unanticipated wakeups so there is a real cost to defaulting to eavesdropping on. It's at least something to consider.

Relevant IRC log:

<davidz> walters: any particular reason that /etc/dbus-1/session.conf defaults to <allow eavesdrop="true"/> ?
<walters> well it only allows connections from one uid
<davidz> walters: as far as I can tell, it incurs needless wakeups
 Tomas ran into this with this gvfs gdbus port
<walters> hmm, why?
<davidz> he wants to emit a signal to a unique name, say :1.42
<walters> i prefer #gnome-os or #gnome-hackers for development stuff btw
<davidz> but :1.43 (which happens to have a match rule that matches the signal) also receives it
 exactly because eavesdropping is on
<walters> hmm
<davidz> if eavesdropping is turned off then :1.43 won't receive it
<walters> why does :1.43 have a match rule for it?
<davidz> because of the way the D-Bus interface works
 I mean, this particular D-Bus interface
<walters> is there a bug on this where i can read the surrounding details?
<davidz> no, it was all private mail
<walters> sounds like a good one to file
 by doing so you may get smcv to look at it, and he is very skilled
<davidz> well, it _has_ to work this way unless we add a StartEavesdropping/StopEavesdropping interface to the bus daemon
 I'm not so much contesting the way it works
 I mean, it is what it is
<walters> well, my plan was to add a separate management socket
<davidz> socket/interface, all the same
 my question was just why it's enabled by default
 I know the answer, of course: developer convenience
<walters> which if it existed is where UpdateActivationEnvironment would have obviously gone
<davidz> separate socket sounds annoying.. but that's another discussion
 anyway, I don't think people realized that this convenience actually has a real price
 of course most interfaces do not suffer from this
 the GVfs one does, however
 (it's for the out-of-process volume monitor stuff)
 anyway, my recommendation would be to turn eavesdropping off by default
 and allowing turned it on/off on the fly
Comment 1 Thiago Macieira 2011-06-03 12:46:46 UTC
> <davidz> he wants to emit a signal to a unique name, say :1.42

Signals don't have destinations. The fact that the reference implementation allows this is a bug. It should deliver the signal to all clients with matching rules.
Comment 2 Colin Walters 2011-06-03 12:49:03 UTC
Can we get the actual match rules and example messages in here?  Is it like:

match type='signal',interface='org.gtk.Private.GPhoto2VolumeMonitor'

and then gvfs sends out a signal with that interface and destination of 1.42, but dbus says "hey, this other client has a match rule and eavesdropping is globally enabled, so i'll also send it to 1.43"?

I think that makes sense, and yeah, we should probably add an opt-in mechanism for eavesdropping.

However - clients can also work around this by adding destination=$self_unique_name in the match rules (yes, this bloats them and probably slows down the bus processing, but).
Comment 3 David Zeuthen (not reading bugmail) 2011-06-03 12:59:42 UTC
(In reply to comment #1)
> > <davidz> he wants to emit a signal to a unique name, say :1.42
> 
> Signals don't have destinations. 

Of course they do. Any D-Bus message can have the DESTINATION header field set. Furthermore, the spec even says "Signals normally do not specify a destination; they are sent to all applications with message matching rules that match the message."

> The fact that the reference implementation
> allows this is a bug. 

No. FWIW, there are valid use cases for this. For example, delivering a signal to another peer without having that peer subscribing to it via a match rule [1]. In fact, you _need_ this functionality to easily do certain things in a race-free way.

[1] : from the spec "The message bus must send messages (of any type) with the DESTINATION field set to the specified recipient, regardless of whether the recipient has set up a match rule matching the message."

> It should deliver the signal to all clients with matching
> rules.

No. The spec clearly says

 When the message bus receives a method call, if the DESTINATION
 field is absent, the call is taken to be a standard one-to-one
 message and interpreted by the message bus itself. For example,
 sending an org.freedesktop.DBus.Peer.Ping message with no
 DESTINATION will cause the message bus itself to reply to the
 ping immediately; the message bus will not make this message
 visible to other applications.

Either way, I'm not disputing how it works (it seems to be working correctly), I'm only complaining that we ship with eavesdropping turned off by default.
Comment 4 David Zeuthen (not reading bugmail) 2011-06-03 13:05:10 UTC
(In reply to comment #2)
> Can we get the actual match rules and example messages in here?  Is it like:
> 
> match type='signal',interface='org.gtk.Private.GPhoto2VolumeMonitor'

Essentially, yes - it's 

 type='signal',
 interface='org.gtk.Private.RemoteVolumeMonitor',
 sender='org.gtk.Private.GPhoto2VolumeMonitor',

> and then gvfs sends out a signal with that interface and destination of 1.42,
> but dbus says "hey, this other client has a match rule and eavesdropping is
> globally enabled, so i'll also send it to 1.43"?

Yes, that is what appears to be happening.

> I think that makes sense, and yeah, we should probably add an opt-in mechanism
> for eavesdropping.

I'm not saying it's *wrong* what the bus is doing. I'm just asking for eavesdropping to be turned off by default. OK, this is inconvenient so that's why I'm asking for a way to turn it on/off at runtime. So dbus-monitor(1) and Bustle can do that.

> However - clients can also work around this by adding
> destination=$self_unique_name in the match rules (yes, this bloats them and
> probably slows down the bus processing, but).

They can, yeah.
Comment 5 David Zeuthen (not reading bugmail) 2011-06-03 13:09:23 UTC
(In reply to comment #3)
> No. The spec clearly says
> 
>  When the message bus receives a method call, if the DESTINATION
>  field is absent, the call is taken to be a standard one-to-one
>  message and interpreted by the message bus itself. For example,
>  sending an org.freedesktop.DBus.Peer.Ping message with no
>  DESTINATION will cause the message bus itself to reply to the
>  ping immediately; the message bus will not make this message
>  visible to other applications.

I was quoting the wrong part of the spec here, sorry.
Comment 6 David Zeuthen (not reading bugmail) 2011-06-03 14:08:12 UTC
Btw, note that dbus-send(1) does not work (at least not the one from dbus-1.4.6-4.fc15.x86_64) - I used the program in [1] to reproduce

If you run

 ./emitsignal session ":1.3"

then you only see something with 'dbus-monitor --session' if eavesdropping is enabled.

[1] :

#include <gio/gio.h>

int
main (int argc, char *argv[])
{
  GDBusConnection *c;
  g_type_init ();
  c = g_bus_get_sync (g_strcmp0 (argv[1], "system") == 0 ? G_BUS_TYPE_SYSTEM : G_BUS_TYPE_SESSION, NULL, NULL);
  g_dbus_connection_emit_signal (c, argv[2],
                                 "/org/example/object",
                                 "org.example.iface",
                                 "Signal",
                                 g_variant_new("(s)", "foo"),
                                 NULL);
  g_dbus_connection_flush_sync (c, NULL, NULL);
  g_object_unref (c);
  return 0;
}
Comment 7 David Zeuthen (not reading bugmail) 2011-06-03 14:15:18 UTC
OK, researched this some more and

 1. This is not mentioned by the spec at all.

 2. It is mentioned in the dbus-daemon(1) man page - here the
    concept of "eavesdropping" is introduced as (basically)
    policy on how to deal with messages where the DESTINATION
    header is set

 3. The man page contradicts the code by saying "it [eavesdropping]
    does not apply to signals". We should fix the man page.

Again, not proposing any wild changes here. Just trying to explain.
Comment 8 Thiago Macieira 2011-06-03 14:22:54 UTC
I'd say then the behaviour is undefined. Nothing in the spec says that signals should have destinations. It says that they usually don't -- which would imply that they sometimes do.

But as a design principle, they shouldn't have destinations. Signals are broadcasts and should be received by all.

If we want signals to have destinations, let's specify that. Add to the spec, add a dbus_message_new_signal_with_destination that sets the destination when creating and the bindings should add a similar function.

Otherwise, I maintain that the current eavesdropping behaviour is the correct one: the destination set by the sender is ignored.
Comment 9 David Zeuthen (not reading bugmail) 2011-06-03 14:33:12 UTC
(In reply to comment #8)
> I'd say then the behaviour is undefined. Nothing in the spec says that signals
> should have destinations. It says that they usually don't -- which would imply
> that they sometimes do.
> 
> But as a design principle, they shouldn't have destinations. Signals are
> broadcasts and should be received by all.

As I said in comment 3 there are valid reasons for wanting to set the DESTINATION field. I'm not sure why you are ignoring that, it's not really helping the discussion.

> If we want signals to have destinations, let's specify that. Add to the spec,

No, need, the spec already allows signals with the DESTINATION header field set.

> add a dbus_message_new_signal_with_destination that sets the destination when
> creating and the bindings should add a similar function.

Uhm, no need for this, you can already use dbus_message_set_destination() on a DBusMessage created with dbus_message_new() or dbus_message_new_signal().

(Btw, if you didn't know, as a general libdbus design principle, things like dbus_message_new_signal() are only convenience constructors.)

> Otherwise, I maintain that the current eavesdropping behaviour is the correct
> one: the destination set by the sender is ignored.

I think you are confused: "destination set by the sender is ignored" is _not_ the current behavior. It is only the current behavior when eavesdropping is enabled. Which is how the session bus works. Notably, this is _not_ what happens on the system bus.
Comment 10 Thiago Macieira 2011-06-03 15:02:44 UTC
(In reply to comment #9)
> As I said in comment 3 there are valid reasons for wanting to set the
> DESTINATION field. I'm not sure why you are ignoring that, it's not really
> helping the discussion.

I'm not ignoring that. I'm saying that the spec doesn't require that and my reading is that it shouldn't have.

You mentioned twice now that there are reasons for having it. I haven't ever encountered one and, before today, I didn't know anyone even wanted it.

> > If we want signals to have destinations, let's specify that. Add to the spec,
> 
> No, need, the spec already allows signals with the DESTINATION header field
> set.

It doesn't say what we should do. You're seeing a lot of contradictions in the man page, the implementations, etc. because it doesn't specify completely. Don't treat this as a bug: treat this as the need to specify undefined/underdefined behaviour.

> > Otherwise, I maintain that the current eavesdropping behaviour is the correct
> > one: the destination set by the sender is ignored.
> 
> I think you are confused: "destination set by the sender is ignored" is _not_
> the current behavior. It is only the current behavior when eavesdropping is
> enabled. Which is how the session bus works. Notably, this is _not_ what
> happens on the system bus.

Then let's be 100% clear: write it in the spec. 

Another thing to be noted is that applications should not rely on the behaviour of eavesdropping being enabled or disabled. Most importantly, if we say that directed signals are permitted (rather than being undefined behaviour), then well-written applications that aren't meant to eavesdrop shouldn't be affected by the flag. Do you have a fix in mind for this?
Comment 11 David Zeuthen (not reading bugmail) 2011-06-03 16:01:02 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > As I said in comment 3 there are valid reasons for wanting to set the
> > DESTINATION field. I'm not sure why you are ignoring that, it's not really
> > helping the discussion.
> 
> I'm not ignoring that. I'm saying that the spec doesn't require that and my
> reading is that it shouldn't have.

The spec says "Signals normally do not specify a destination [...]" which implies that they sometimes do. Additionally, the DESTINATION header field is allowed in a message for a signal. Additionally, the reference implementation specifically allows it and it works according to spec including the eavesdropping feature. I don't know how this can be much clearer.

> You mentioned twice now that there are reasons for having it. I haven't ever
> encountered one and, before today, I didn't know anyone even wanted it.

OK, two examples

1. As a non-snoopable transport mechanism - e.g. just like how it's suitable to include a password in method call parameters on the system bus [a] - no one can snoop it because eavesdropping is just not allowed on the system bus.

[a]: and we indeed have software doing this for unlocking e.g. LUKS encrypted block devices - you don't want uid 501 to snoop the password provided by uid 500

2. Consider a method

  Foo (IN String whatever, OUT ObjectPath some_path);

where the Foo() method creates an object that only the caller of Foo() cares about. Assume that signals on the created object has DESTINATION set to the unique name of whoever invoked Foo(). Since the spec says

  The message bus must send messages (of any type) with
  the DESTINATION field set to the specified recipient,
  regardless of whether the recipient has set up a match rule
  matching the message

then the app does not need to call AddMatch() to catch signals from the @some_path. Bingo, we just avoided a race condition.

Btw, this is not some far-fetched example - think of Foo() as creating a view (the created object) into some data. Or think of the created object as a Job object for tracking progress.

Additionally, since DESTINATION is set for all signals, we can include confidential data (e.g. data that cannot be snooped). Again, very handy on e.g. the system bus.
 
> > > If we want signals to have destinations, let's specify that. Add to the spec,
> > 
> > No, need, the spec already allows signals with the DESTINATION header field
> > set.
> 
> It doesn't say what we should do. You're seeing a lot of contradictions in the
> man page, the implementations, etc. because it doesn't specify completely.
> Don't treat this as a bug: treat this as the need to specify
> undefined/underdefined behaviour.

As I said, I don't think there are no bugs in the implementation! Just a bug in the man page (it should mention that signals are fine), plus some overhead which is what I like to remove.

I'm pretty sure Havoc had the two uses I sketched out above in mind. Of course something like that (confidentiality; avoiding race conditions) needs to be possible.

> Then let's be 100% clear: write it in the spec. 

I think the spec is fine - in my view it 100% allows signal messages to have a destination. Additionally, libdbus-1 allows it. And my own impl in GLib allows it too.

The spec *could* mention "eavesdropping" but then again it's probably also fine that it's just a dbus-daemon(1) implementation detail. I think the spec should say something like "some message bus implementations may prohibit eavesdropping" (of course defining what eavesdropping means) but nothing more than that. Maybe the system message bus section of the spec should mention that apps are guaranteed that non-privileged processes can eavesdrop. The thing is that we don't want the spec to limit too much how our code should work - it's about finding that balance.

> Another thing to be noted is that applications should not rely on the behaviour
> of eavesdropping being enabled or disabled. Most importantly, if we say that
> directed signals are permitted (rather than being undefined behaviour), then
> well-written applications that aren't meant to eavesdrop shouldn't be affected
> by the flag. Do you have a fix in mind for this?

If an app relies on eavesdropping, then, well, fix the app if it doesn't work when eavesdropping is turned off. I don't think any existing app has that problem though.

And I disagree it's currently "undefined behavior" - heck, the spec directly mentions that a signal message may have the DESTINATION field set... the bus allows it.. libdbus-1 allows it... gdbus has explicit support for it via the @destination parameter in g_dbus_connection_emit_signal().

So I still maintain you are not reading the spec correctly if you think it's "undefined behavior".... what do you think spec needs in order to make you read it as I do?
Comment 12 Thiago Macieira 2011-06-04 01:13:54 UTC
(In reply to comment #11)
> 1. As a non-snoopable transport mechanism - e.g. just like how it's suitable to
> include a password in method call parameters on the system bus [a] - no one can
> snoop it because eavesdropping is just not allowed on the system bus.
> 
> [a]: and we indeed have software doing this for unlocking e.g. LUKS encrypted
> block devices - you don't want uid 501 to snoop the password provided by uid
> 500

Ok, that is a valid use-case. Assuming that signals can have destinations, then signals can be used to transport confidential data. But that's a consequence, not a requirement: to be a requirement, you'd have to tell me why the data needs to be transported as a signal.

> 2. Consider a method
> 
>   Foo (IN String whatever, OUT ObjectPath some_path);
> 
> where the Foo() method creates an object that only the caller of Foo() cares
> about. Assume that signals on the created object has DESTINATION set to the
> unique name of whoever invoked Foo(). Since the spec says
> 
>   The message bus must send messages (of any type) with
>   the DESTINATION field set to the specified recipient,
>   regardless of whether the recipient has set up a match rule
>   matching the message
> 
> then the app does not need to call AddMatch() to catch signals from the
> @some_path. Bingo, we just avoided a race condition.

I see. You can't call AddMatch before Foo because you don't have the path yet. And you adding a match based on other properties (say, the interface) would be sub-optimal, since you may get signals that aren't meant for you and you'd have to ignore.

Ok, this is a valid use-case.

> > Then let's be 100% clear: write it in the spec. 
> 
> I think the spec is fine - in my view it 100% allows signal messages to have a
> destination. Additionally, libdbus-1 allows it. And my own impl in GLib allows
> it too.

And I'm telling you that I didn't read it like that. I have 12 years of experience with signals inside Qt and 5½ years of D-Bus and it never crossed my mind that they should be legal until yesterday. The API also suggests that they aren't meant to be used since dbus_message_new_signal doesn't have it as an optional parameter, unlike the g_dbus_connection_emit_signal counterpart of your comment 6.

So take this again as a suggestion: make it 100% clear.

> > Another thing to be noted is that applications should not rely on the behaviour
> > of eavesdropping being enabled or disabled. Most importantly, if we say that
> > directed signals are permitted (rather than being undefined behaviour), then
> > well-written applications that aren't meant to eavesdrop shouldn't be affected
> > by the flag. Do you have a fix in mind for this?
> 
> If an app relies on eavesdropping, then, well, fix the app if it doesn't work
> when eavesdropping is turned off. I don't think any existing app has that
> problem though.

I'm saying that well-written apps may have unintended side-effects due to eavesdropping being enabled. If we say that the default is "off" and that signals with destinations will be received by only one connection, then applications may be written to expect that the signals they receive are intended for it. After all, in the process of development, the developer of said application may only have seen that behaviour.

However, if eavesdropping happens to be on for whatever valid reason on someone else's workstation, applications may suddenly start receiving signals that aren't meant for it. Its match rules aren't meant to eavesdrop, they are meant to receive signal broadcasts, but suddenly non-broadcasts are received too. They may start mis-behaving because of this.

I am asking of you: do you have a suggestion on how to handle this?
Comment 13 Thiago Macieira 2011-06-07 11:39:58 UTC
> I am asking of you: do you have a suggestion on how to handle this?

Here's my suggestion: we make the regular match rules work as if eavesdropping were off, even if it's on in the configuration. In order to turn eavesdropping on in a rule, something special must be added.

For the signals case, we can change the specification to say that the absence of "destination=" means the default rules for destinations: the message is either a broadcast or directed at this connection, To receive messages to other destinations, applications would need to write "destination=*".

Or, another alternative, it's to add an "eavesdrop=yes" option to the match rule. This actually preferred, as it makes clear that eavesdropping was intended and dbus-monitor can simply prepend it to all rules it sends to the bus, without having to parse them. It's probably easier to implement too.
Comment 14 Colin Walters 2011-06-07 11:48:00 UTC
(In reply to comment #13)

> Or, another alternative, it's to add an "eavesdrop=yes" option to the match
> rule. This actually preferred, as it makes clear that eavesdropping was
> intended and dbus-monitor can simply prepend it to all rules it sends to the
> bus, without having to parse them. It's probably easier to implement too.

I like this suggestion.
Comment 15 Simon McVittie 2011-06-28 08:12:09 UTC
(In reply to comment #2)
> However - clients can also work around this by adding
> destination=$self_unique_name in the match rules (yes, this bloats them and
> probably slows down the bus processing, but).

In the short term, I think GVFS should do that. If quantifiable benchmarks prove that it adds significant extra cost for the bus daemon, let us know. (I doubt it does - it's basically a single strcmp.)

(In reply to comment #13)
> Here's my suggestion: we make the regular match rules work as if eavesdropping
> were off, even if it's on in the configuration. In order to turn eavesdropping
> on in a rule, something special must be added.

For master, I do like this idea: it removes a lot of potential for shooting yourself in the foot, without removing the developer convenience of having eavesdropping available on your session bus. However, I don't think we can say that this is a compatible change with a straight face: it breaks existing behaviour (even if only debug tools like Bustle should have been using it).

I think this sort of minor breakage is OK in master (if announced properly in NEWS), but not in stable branches!
Comment 16 Simon McVittie 2011-06-28 08:31:50 UTC
If we offer eavesdropping (in its current form) at all, I think it should continue to be allowed-by-default for the session bus, even if processes need to adjust their match rules to opt in.

Rationale:

* Having things behave differently in "debug mode" is Not Right; it
  leads to Heisenbugs. If developers are going to have eavesdropping
  switched on, at least while they debug something, and if it doesn't
  result in performance or security problems in "production" usage
  (dbus-monitor not running), then we should just leave it on all the time.

* The bus security policy is a security policy, not a performance policy;
  the session bus isn't a security boundary (except on specialized
  platforms like OLPC and Maemo, which get to keep both pieces tbh),
  and as such, it has no security policy.

* Applications can already set up match rules to get optimal performance
  (in terms of avoiding undesired messages - although fewer match rules can
  probably also be an improvement), so the only performance problem is that
  the most obvious way to add a match rule (omitting destination) is secretly
  not the best; but if we add an eavesdrop='yes' switch on match rules,
  we've fixed that.
Comment 17 Simon McVittie 2011-07-01 02:53:50 UTC
Cosimo is working on a patch for 1.5, to implement Thiago's suggestion.
Comment 18 Cosimo Alfarano 2011-07-11 03:57:06 UTC
Created attachment 48962 [details] [review]
Add eavesdrop=true|false to match rules.

Add "eavesdrop=true|false" to match rules, implementing Thiago's suggestion on Comment #13

The patch acts on the rule structure, adding a flag.

One issue I see with this approach is that the added flag is actually a meta information about how the rule should act (ie it rules about the rule), but it's expressed within the same rule.
Comment 19 Cosimo Alfarano 2011-07-11 04:00:15 UTC
Created attachment 48963 [details] [review]
Enables dbus-monitor to automatically use eavesdrop=true

Dbus-monitor wants to eavesdrop (by definition of monitor).

In the exceptional case the user does not want to eavesdrop, he/she can always use eavesdrop=false in the passed filter, which will disable it.
(collateral effect for this feature: a match rule is allowed to have any number of "eavesdrop" keyword occurrences)
Comment 20 Simon McVittie 2011-07-11 04:39:14 UTC
Implementation-wise this looks fine, just some nitpicking:

> +      else if (strcmp (key, "eavesdrop") == 0)
> +        {
> +          /* do not detect "eavesdrop" being used more than once in rule:
> ...
> +          _dbus_assert ((rule->flags & BUS_MATCH_CLIENT_IS_EAVESDROPPING) == FALSE);

The assertion contradicts the comment: please remove it.

(Also, (rule->flags & ANY_FLAG) is flag-valued, not boolean-valued, so
comparing to 0 would have been clearer. Conceptually, it takes values in the
set { 0, BUS_MATCH_CLIENT_IS_EAVESDROPPING }, rather than { FALSE, TRUE }.)

> +                              "Eavesdropper client value '%s' is invalid\n",

A better message might be "eavesdrop='%s' is invalid, should be true or false"
or something?

> +  /* BUS_MATCH_CLIENT_IS_EAVESDROPPING: we already matched flags */
> +

I don't think this comment really adds anything?

> +void bus_match_rule_set_client_is_eavesdropping (BusMatchRule     *rule,
> +                                                 dbus_bool_t is_eavesdropping);

Couldn't this be static?

> @@ -50,6 +52,7 @@
> #define _IONBF 0x04
> #endif
>
>+
> void
> GetSystemTimeAsFileTime (LPFILETIME ftp)
> {

Unnecessary whitespace change

> +        /* append a rule (and a comma) to enable the monitor to eavesdrop */

You're prepending, not appending, so please change the comment.

(It is correct and significant that you prepend, since it lets the rule
itself override eavesdrop back to false - I think that might deserve
mentioning.)

> +     filters = (char **)realloc(filters, numFilters * sizeof(char *));
> +     filters[j] = (char *)malloc(filter_len * sizeof(char *));

Pre-existing bug, but you should do something about malloc/realloc failures.
Something simple like

    if (filters == NULL)
      oom ();

(where oom() just complains to stderr and calls exit (1)) would be enough.

Pre-existing bug, but I'd prefer the lines you've touched to have this
spacing:

    (char **) realloc (filters, numFilters * sizeof (char *));

and be indented with spaces instead of tabs. (General principle: if the line
is going to be changed anyway, you might as well make it look nice.)
Comment 21 Cosimo Alfarano 2011-07-11 05:56:42 UTC
The not quoted suggestion have been fixed.

(In reply to comment #20)
> > +  /* BUS_MATCH_CLIENT_IS_EAVESDROPPING: we already matched flags */
> I don't think this comment really adds anything?

I usually like to leave similar hints for future code readers/bug hunters, so they won't waste time to understand why that specific flag has not been checked explicitly: "Wait, there is a flag not checked..... oh, that's why".

No problem to remove it, though.

> > +void bus_match_rule_set_client_is_eavesdropping (BusMatchRule     *rule,
> > +                                                 dbus_bool_t is_eavesdropping);
> 
> Couldn't this be static?

It can, other BusMatchRule methods are public and I assumed that it's to allow  someone to create a rule using the given setters instead of passing a string.
If it's not the case, it should be static, I agree.

Currently it's used only in bus/signal.c

> Pre-existing bug, but I'd prefer the lines you've touched to have this
> spacing:
> 
>     (char **) realloc (filters, numFilters * sizeof (char *));
> 
> and be indented with spaces instead of tabs. (General principle: if the line
> is going to be changed anyway, you might as well make it look nice.)


done, I'll leave the rest of the previous "if" branches untouched.
Let me know if I'm supposed to fix them as well. Tabs have been used massively in dbus-monitor.c.
Comment 22 Cosimo Alfarano 2011-07-11 06:44:49 UTC
Created attachment 48971 [details]
[test] first part of the test

code mostly copied from example-emitter/receiver.

The emitter emits three signals:
- broadcast
- unicast to the emitter itself (ie not client should be an actual destination for this signal, but the emitter itself)
- unicast to the caller of the method triggering the signal emission

this way, using the receiver we can test that:
- broadcast messages are always received either with eavesdrop=true or eavesdrop=false
- unicast signals are received by the rule owner (the method caller) when eavesdrop=false (default case for non-monitors)
- unicast signals are not received by non rule owners (the other unicast msg sent to <self>)


then using a patched version of dbus-monitor we can see:
- that by default (eavesdrop=true) dbus-monitor will see the three signals
- if eavesdrop=false is added to the rule, only broadcast msg is seen, and the other two will be not received, since dbus-monitor is not the rule owner for any of the rules associated with the emitted signals

for testing purposes, the old (non-patched) version of dbus-monitor used against a patched dbus-daemon will act as eavesdrop=false is passed.
Comment 23 Cosimo Alfarano 2011-07-11 06:50:02 UTC
Created attachment 48972 [details]
[test] second part of the test

against a session of a patched dbus-daemon
$ python ./test_emitter &

on two other another terminals using a patched version of dbus-monitor:

$ dbus-monitor member=HelloSignal
signal sender=org.freedesktop.DBus -> dest=:1.159 serial=2 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.159"
signal sender=:1.158 -> dest=(null destination) serial=4 path=/com/example/TestService/object; interface=com.example.TestService; member=HelloSignal
   string "s"
   string "hello broadcasted"
signal sender=:1.158 -> dest=com.example.TestService serial=5 path=/com/example/TestService/object; interface=com.example.TestService; member=HelloSignal
   string "s"
   string "hello for com.example.TestService (unicast)"
signal sender=:1.158 -> dest=:1.160 serial=6 path=/com/example/TestService/object; interface=com.example.TestService; member=HelloSignal
   string "s"
   string "hello for :1.160 (unicast)"






$ dbus-monitor member=HelloSignal,eavesdrop=false
signal sender=org.freedesktop.DBus -> dest=:1.161 serial=2 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.161"
signal sender=:1.162 -> dest=(null destination) serial=4 path=/com/example/TestService/object; interface=com.example.TestService; member=HelloSignal
   string "s"
   string "hello broadcasted"


The monitor output will be produced when the received is called:

$ python test_receiver.py 
Received a hello signal for 'None' and it says: hello broadcasted
Received signal (by connecting using remote object) for 'None' and it says: hello broadcasted
Received a hello signal for ':1.163' and it says: hello for :1.163 (unicast)
Received signal (by connecting using remote object) for ':1.163' and it says: hello for :1.163 (unicast)
Signal emitted
Comment 24 Simon McVittie 2011-07-11 07:02:12 UTC
(In reply to comment #21)
> The not quoted suggestion have been fixed.
> 
> (In reply to comment #20)
> > > +  /* BUS_MATCH_CLIENT_IS_EAVESDROPPING: we already matched flags */
> > I don't think this comment really adds anything?
> 
> I usually like to leave similar hints for future code readers/bug hunters, so
> they won't waste time to understand why that specific flag has not been checked
> explicitly: "Wait, there is a flag not checked..... oh, that's why".

I'd prefer something like this, then:

    /* we already compared the value of flags, and
     * BUS_MATCH_CLIENT_IS_EAVESDROPPING does not have another struct member */

> > > +void bus_match_rule_set_client_is_eavesdropping (BusMatchRule     *rule,
> > > +                                                 dbus_bool_t is_eavesdropping);
> > 
> > Couldn't this be static?
> 
> It can, other BusMatchRule methods are public

*shrug* leave it, then.

> done, I'll leave the rest of the previous "if" branches untouched.

That's fine, I'm only asking you to fix whitespace in lines you're already changing :-)
Comment 25 Cosimo Alfarano 2011-07-11 07:42:24 UTC
Created attachment 48975 [details] [review]
Add eavesdrop=true|false to match rules

Fixed patch for addeavesdrop=true|false, obsoleting attachment #48962 [details] [review]
Comment 26 Cosimo Alfarano 2011-07-11 07:43:44 UTC
Created attachment 48976 [details] [review]
Add eavesdrop=true to dbus-monitor

fixed patch for adding eavesdrop=false to dbus-monitor, obsoletes attachment #48963 [details] [review]
Comment 27 Simon McVittie 2011-07-11 08:48:42 UTC
Comment on attachment 48975 [details] [review]
Add eavesdrop=true|false to match rules

>+              dbus_set_error (error, DBUS_ERROR_MATCH_RULE_INVALID,
>+                              "'eavesdropper=%s' is invalid, "
>+                              "it should be 'true' or 'false'\n",

Nitpicking: s/eavesdropper/eavesdrop/

(An eavesdropper is a person or thing that eavesdrops.)
Comment 28 Simon McVittie 2011-07-11 09:06:15 UTC
(In reply to comment #25)
> Add eavesdrop=true|false to match rules

Applied, with the typo fix I suggested. 1.5.6.

(In reply to comment #26)
> Add eavesdrop=true to dbus-monitor

Applied. 1.5.6.

Leaving this bug open while Cosimo works on an automated test.
Comment 29 Cosimo Alfarano 2011-07-12 09:58:20 UTC
Created attachment 49015 [details] [review]
handle no filter at cmdline case

I forgot to handle the case on which no filters are passed at command line.

Here's the fix.
Comment 30 Simon McVittie 2011-07-12 10:43:49 UTC
(In reply to comment #29)
> handle no filter at cmdline case

Oops, yes. Also merged.
Comment 31 Cosimo Alfarano 2011-07-14 05:50:38 UTC
Created attachment 49076 [details] [review]
Add autometed tests for eavesdrop=true|false
Comment 32 Simon McVittie 2011-07-14 07:51:21 UTC
Review of attachment 49076 [details] [review]:

Some typos and stuff, but I'm going to merge it anyway, because test > no test :-)

::: test/dbus-daemon-eavesdrop.c
@@ +49,3 @@
+
+/* This rule is equivalend to the one added to a proxy connecting to
+ * SENDER_NAME+SENDER_IFACE, plus restructubg on signal name.

"equivalent", "added by a proxy", "restricting"

@@ +64,3 @@
+/* a connection received a signal to whom? */
+typedef enum {
+  UNKNOWN,

From its usage, NONE_YET would be a more accurate description?

@@ +244,3 @@
+}
+
+/* Send special broadcast signal to indicate that conncetion can "stop"

connection

@@ +246,3 @@
+/* Send special broadcast signal to indicate that conncetion can "stop"
+ * listening and check their results.
+ * DBus does not re-order msgs, so when the three connection will receive this

Please don't abbreviate unnecessarily: "messages"

Grammar: when the three connections have received this... message sent before it

@@ +269,3 @@
+
+/* Ignore NameAcquired, then depending on the signal received:
+ * - updateds f-><conn>_dst based on the destination of the msg

updates, message

@@ +327,3 @@
+    }
+
+  return DBUS_HANDLER_RESULT_HANDLED;

It doesn't actually matter here, but in general signals should be NOT_YET_HANDLED

@@ +502,3 @@
+
+
+  if (f->receiver != NULL)

Unnecessary double blank line just above here
Comment 33 Simon McVittie 2011-07-21 09:04:56 UTC
Fixed in git for 1.5.6; incompatible change, so should not be fixed in 1.4