Created attachment 30040 [details] [review] Implement GetConnectionMatchRules Applications subscribing to D-Bus messages with a too broad match rule are a common cause of useless wake-up. In order to find those guilty applications, it is useful to get the list of match rules registered to dbus-daemon. The attached patch is a first implementation of a D-Bus method GetConnectionMatchRules() on the bus driver /org/freedesktop/DBus. GetConnectionMatchRules(s: name) -> s: rules The patch applies on top of Will's optimize-matching branch (see bug #23117). I plan to replace the return type "s" by an array of match rules "as". But it is already useful to find problems in applications connecting to dbus-daemon.
Created attachment 30041 [details] matchrulelist.py Python script to retrieve all D-Bus match rules on the session or system bus. Useful to find match rules without a message type, or without an interface: those match rules defeat the optimizations in #23117.
Haven't looked at the implementation yet, but I don't really like non-standard debugging interfaces as part of org.freedesktop.DBus interface. My intuition here is that what would be nicer is something like a "DumpState" call which would dump all sorts of things into say one big XML blob, like: * match rules * number of pending/processed messages * all connections - process id - binary And maybe create a new interface for this? org.freedesktop.DBus.Debugging? Not sure.
I agree that we should avoid confusion with standard interfaces. I am a bit afraid of the security implication, on the system bus for example. org.freedesktop.DBus.Debugging would be ok for me. But why DumpState with an XML blob? I would prefer to have a D-Bus-like interface and avoid XML. It would be easier to implement, both dbus-daemon side and debugging tool side. I also like the idea to have process id and binary command line. If there could be a way to integrate this nicely with Bustle, it would be great :-)
(In reply to comment #3) > I agree that we should avoid confusion with standard interfaces. I am a bit > afraid of the security implication, on the system bus for example. Yes; we might key this off the <allow eavesdrop/> permission? But that doesn't really work that well. > org.freedesktop.DBus.Debugging would be ok for me. But why DumpState with an > XML blob? I would prefer to have a D-Bus-like interface and avoid XML. It would > be easier to implement, both dbus-daemon side and debugging tool side. It could be an a{sv} too I guess, I don't really have a strong opinion.
Created attachment 39253 [details] [review] Implement message type in match_rule_to_string This is only the part that implements the missing bits in match_rule_to_string, I reckon this is not problematic on its own right.
Created attachment 39254 [details] [review] Add GetConnectionMatchRules in the bus driver This is the GetConnectionMatchRules on top of that, with leaks fixed (even if there's no agreement I figure it's better to have a proper patch here).
Review of attachment 39253 [details] [review]: I think this is good to have, so I'm inclined to apply it despite the points below, and create a follow-up patch that fixes them. ::: bus/signals.c @@ +138,3 @@ + if (rule->message_type == DBUS_MESSAGE_TYPE_INVALID) + { + if (!_dbus_string_append_printf (&str, "type='INVALID'")) This doesn't need to be a printf: we're appending a literal. @@ +141,3 @@ + goto nomem; + } + else if (rule->message_type == DBUS_MESSAGE_TYPE_METHOD_CALL) I'd prefer to have a switch (rule->message_type) with a default case.
(In reply to comment #7) > Review of attachment 39253 [details] [review]: > > I think this is good to have, so I'm inclined to apply it despite the points > below, and create a follow-up patch that fixes them. Well, my most preferred option, as I mentioned in the mailing list a while back, would be an entirely separate /var/run/dbus/system_bus_management_socket that the system bus listens to, and could be used for all sorts of stuff that really doesn't belong as public API on org.freedesktop.DBus, such as the existing UpdateActivationEnvironment. I don't think that would be an unreasonable amount of work, and would allow us to fix other issues (for example, I pretty strongly think eavesdropping should work this way. The bus shouldn't block normal message delivery on eavesdroppers, and it would also allow us to always enable eavesdropping and secure it with a separate policy.) Besides all that, this patch seems like a big one-off really. I mean, I expect how it would be used is that people do an e.g. Fedora 14 install, patch dbus to dump the match rules, find application offenders, and fix them. After that the patch isn't needed again for a while. If we just stick it on org.freedesktop.DBus like we did with UpdateActivationEnvironment, it's forever, and even worse it would normally never be used.
(In reply to comment #8) > Besides all that, this patch seems like a big one-off really. I mean, I expect > how it would be used is that people do an e.g. Fedora 14 install, patch dbus to > dump the match rules, find application offenders, and fix them. After that the > patch isn't needed again for a while. In Maemo (which uses D-Bus pretty heavily, on an embedded device) this has generally been done iteratively, so perhaps we wouldn't have this enabled for a final release, but it'd be desirable for most of the development cycle. Also, this and similar patches seem less likely to bit-rot if this functionality is merged, disabled in ./configure for distros/manual installation, and enabled for developers. Perhaps the default could be tied to maintainer mode, like _dbus_verbose is? My medium-term plan is to make the bus daemon able to implement multiple interfaces (Bug #33757), add --enable-debugging or something, and have an o.fd.DBus.Debugging interface which is conditionally-compiled, and contains: * this * various numeric stats in pairs of current value and peak usage: - number of match rules - number of connections - in general, anything there's a limit for in session.conf On the same or a different interface and ./configure option, we could have similar stats per-connection. For the system bus, we could have these only available via the hypothetical management socket, but the session bus still needs the same functionality (detecting misuse of D-Bus on the session bus is just as useful).
(In reply to comment #9) > > My medium-term plan is to make the bus daemon able to implement multiple > interfaces (Bug #33757), add --enable-debugging or something, and have an > o.fd.DBus.Debugging interface which is conditionally-compiled, and contains: That sounds good to me. > For the system bus, we could have these only available via the hypothetical > management socket, but the session bus still needs the same functionality > (detecting misuse of D-Bus on the session bus is just as useful). Well, there's no reason the session bus couldn't also allocate a different management socket too; the cost is pretty minimal. Not blocking the session on an eavesdropper has as much value as it does for the system. But I agree it's effectively orthonogal, and the first step is defining and implementing the new debugging/profiling interfaces.
I've been reminded that this bug is still here and bit-rotting. I'm working on GDBus thread-safety at the moment, but this is next on my list (unless you want to take it, Cosimo?) (In reply to comment #9) > My medium-term plan is to make the bus daemon able to implement multiple > interfaces (Bug #33757), add --enable-debugging or something, and have an > o.fd.DBus.Debugging interface which is conditionally-compiled This is now o.fd.DBus.Stats, which is #ifdef'd optional functionality. I plan to rebase this branch onto Stats (so GetConnectionMatchRules is a method on Stats - it's not really a statistic, but the general idea is the same). GetConnectionMatchRules is information disclosure, so we should restrict it to root-only in the system bus' default policy. I'm inclined to make the entire Stats interface root-only, just to be sure (I don't think anything in it is sensitive, but still).
(In reply to comment #11) > I've been reminded that this bug is still here and bit-rotting. I'm working on > GDBus thread-safety at the moment, but this is next on my list (unless you want > to take it, Cosimo?) sure, although I'll be able to take it in two days, feel free to work on it if you need/want to earlier.
(In reply to comment #11) > This is now o.fd.DBus.Stats, which is #ifdef'd optional functionality. I plan It's o.fd.DBus.Debug.Stats which probably opens some room for a simple o.fd.DBus.Debug with subsantially the same access & compile-time policies. I'm working on it under Debug.Stats though, it's really easy to add the other Interface. Leaving it outside Stats seems cleaner and more intuitive for who might look for it, I don't know if it's worth to add an interface just for it, from a comment it seems that there is a significant(is it?) lookup cost which increases over the number of interfaces. It's probably enough to put Debug as last, before Debug.Stats, though.
triaging: low/enhancement
Comment on attachment 39253 [details] [review] Implement message type in match_rule_to_string Mark this patch as obsolete: See Bug #34040 Comment #1, and commit 85e9a (Jan 2011). match_rule_to_string() now uses dbus_message_type_to_string().
Created attachment 102017 [details] [review] [PATCH] Implement GetConnectionMatchRules on the Stats interface Updated the GetConnectionMatchRules with the following changes: - implemented on the Stats interface. Needs dbus compiled with --enable-stats - instead of a big string, new signature: - input: "" - output: "a{sas}" Example how it could look like: $ dbus-send --print-reply --dest=org.freedesktop.DBus /org/freedesktop/DBus \ org.freedesktop.DBus.Debug.Stats.GetConnectionMatchRules method return sender=org.freedesktop.DBus -> dest=:1.13 reply_serial=2 array [ dict entry( string ":1.4" array [ ] ) dict entry( string ":1.9" array [ string "type='signal',interface='org.freedesktop.DBus',member='NameOwnerChanged'" ] ) dict entry( string ":1.11" array [ string "eavesdrop='true'" ] ) ]
(In reply to comment #16) > Created attachment 102017 [details] [review] [review] > [PATCH] Implement GetConnectionMatchRules on the Stats interface In order to enable / disable the Stats interface without recompiling dbus, I filed Bug #80759.
Created attachment 102858 [details] Script using GetConnectionMatchRules I used this script to find match rules with the patterns I was interested in, such as member='NameOwnerChanged' without arg0 criteria or match rules without 'sender' criteria.
I would like the match rules and other stats to be easily available through d-feet: https://bugzilla.gnome.org/show_bug.cgi?id=735167
Comment on attachment 102017 [details] [review] [PATCH] Implement GetConnectionMatchRules on the Stats interface Review of attachment 102017 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +1789,4 @@ > static const MessageHandler stats_message_handlers[] = { > { "GetStats", "", "a{sv}", bus_stats_handle_get_stats }, > { "GetConnectionStats", "s", "a{sv}", bus_stats_handle_get_connection_stats }, > + { "GetConnectionMatchRules", "", "a{sas}", bus_stats_handle_get_connection_match_rules }, GetConnectionFoo usually means "take a bus name as an argument, and return foo for that bus name", whereas I think this is returning info about all connections simultaneously. So I'd prefer GetMatchRules or GetAllMatchRules or something. ::: bus/signals.c @@ +125,5 @@ > +#ifdef DBUS_ENABLE_STATS > +#define MATCH_RULE_TO_STRING > +#endif > + > +#ifdef MATCH_RULE_TO_STRING I'd prefer #if defined(DBUS_ENABLE_VERBOSE_MODE) || defined(DBUS_ENABLE_STATS), tbh @@ +130,2 @@ > /* Note this function does not do escaping, so it's only > * good for debug spew at the moment Not ideal that it doesn't escape things correctly; consumers of this API will have to take that into account. Unless the comment is a lie, or you fix it. @@ +1175,5 @@ > + BusMatchRule *rule = link->data; > + > + if (rule->matches_go_to == conn_filter) > + { > + char *s = match_rule_to_string (rule); if (s == NULL) return FALSE; @@ +1196,5 @@ > + BusMatchRule *rule = link->data; > + > + if (rule->matches_go_to == conn_filter) > + { > + char *s = match_rule_to_string (rule); if (s == NULL) return FALSE; ::: bus/stats.c @@ +41,4 @@ > DBusMessage *message, > DBusError *error) > { > + BusContext *context; This patch-band has already been applied, Bug #81043 @@ +50,5 @@ > > _DBUS_ASSERT_ERROR_IS_CLEAR (error); > > + context = bus_transaction_get_context (transaction); > + connections = bus_context_get_connections (context); This patch-band has already been applied, Bug #81043 @@ +300,5 @@ > + goto oom; > + } > + > + if (!dbus_message_iter_close_container (&entry_iter, &arr_iter)) > + goto oom; Still need to abandon the arr_iter and the hash_iter here @@ +302,5 @@ > + > + if (!dbus_message_iter_close_container (&entry_iter, &arr_iter)) > + goto oom; > + if (!dbus_message_iter_close_container (&hash_iter, &entry_iter)) > + goto oom; Still need to abandon the hash_iter here
Created attachment 106796 [details] [review] [PATCH 1/5] Implement GetAllMatchRules on the Stats interface v2: - fixes from smcv's review - fix memory leaks
Created attachment 106797 [details] [review] [PATCH 2/5] Stats: GetAllMatchRules: add tests
Created attachment 106798 [details] [review] [PATCH 3/5] match_rule_to_string: returns NULL if no memory instead of looping
Created attachment 106799 [details] [review] [PATCH 4/5] match_rule_to_string: fix escaping
Created attachment 106800 [details] [review] [PATCH 5/5] match_rule_to_string: add test
Comment on attachment 106796 [details] [review] [PATCH 1/5] Implement GetAllMatchRules on the Stats interface Review of attachment 106796 [details] [review]: ----------------------------------------------------------------- ::: bus/signals.c @@ +1157,5 @@ > + > + _dbus_hash_iter_init (matchmaker->rules_by_type[i].rules_by_iface, &iter); > + while (_dbus_hash_iter_next (&iter)) > + { > + list = *((DBusList **)_dbus_hash_iter_get_value (&iter)); The data types for "list" and "list link" are conventionally DBusList ** and DBusList * respectively, so I'd prefer: DBusList **list; list = _dbus_hash_iter_get_value (&iter)); do_things_with (list); rather than using &list. (Using &list like that would be functionally wrong if you altered the list, but as it happens, you don't.) @@ +1159,5 @@ > + while (_dbus_hash_iter_next (&iter)) > + { > + list = *((DBusList **)_dbus_hash_iter_get_value (&iter)); > + link = _dbus_list_get_first_link (&list); > + while (link != NULL) I'd personally use for (link = _dbus_list_get_first_link (list); link != NULL; link = _dbus_list_get_next_link (list)) and eliminate the "next" variable, but that's your call. (applies twice in this function) @@ +1182,5 @@ > + > + link = next; > + } > + } > + list = matchmaker->rules_by_type[i].rules_without_iface; With list being a DBusList **, you could use &matchmaker->rules_by_type[i].rules_without_iface here.
Comment on attachment 106797 [details] [review] [PATCH 2/5] Stats: GetAllMatchRules: add tests Review of attachment 106797 [details] [review]: ----------------------------------------------------------------- OK ::: bus/dispatch.c @@ +1609,5 @@ > + { > + if (dbus_message_is_error (message, > + DBUS_ERROR_NO_MEMORY)) > + { > + ; /* good, this is a valid response */ No need for the semicolon here, and it introduces an empty statement which some compilers warn about. I would prefer without it. @@ +1622,5 @@ > + else > + { > + if (dbus_message_get_type (message) == DBUS_MESSAGE_TYPE_METHOD_RETURN) > + { > + ; /* good, expected */ ditto
Comment on attachment 106798 [details] [review] [PATCH 3/5] match_rule_to_string: returns NULL if no memory instead of looping Review of attachment 106798 [details] [review]: ----------------------------------------------------------------- OK
Comment on attachment 106797 [details] [review] [PATCH 2/5] Stats: GetAllMatchRules: add tests Review of attachment 106797 [details] [review]: ----------------------------------------------------------------- ::: bus/dispatch.c @@ +1609,5 @@ > + { > + if (dbus_message_is_error (message, > + DBUS_ERROR_NO_MEMORY)) > + { > + ; /* good, this is a valid response */ This pattern was copied from others functions: see check_hello_message() and a dozen of other similar functions.
Comment on attachment 106799 [details] [review] [PATCH 4/5] match_rule_to_string: fix escaping Review of attachment 106799 [details] [review]: ----------------------------------------------------------------- ::: bus/signals.c @@ +137,5 @@ > + if (!_dbus_string_append_printf (str, "%.*s", (int) (next - p), p)) > + return FALSE; > + /* Horrible escape sequence: single quote cannot be escaped inside > + * a single quoted string. So we close the single quote, escape the > + * single quote, and reopen a single quote. This is absolutely horrible, and the fact that you're writing C makes it look even worse... but it appears to be correct. I looked up the quoting rules in the D-Bus Specification, with the predictable result that I found they are undocumented. I'm preparing a patch. @@ +176,1 @@ > dbus_message_type_to_string (rule->message_type))) This change is unnecessary, because dbus_message_type_to_string() cannot output a string containing a comma, apostrophe or backslash... but your version is more obviously correct, which is the best sort of correct. So I like it better anyway.
Comment on attachment 106800 [details] [review] [PATCH 5/5] match_rule_to_string: add test Review of attachment 106800 [details] [review]: ----------------------------------------------------------------- OK
(In reply to comment #29) > This pattern was copied from others functions: see check_hello_message() and > a dozen of other similar functions. OK, either change it or don't, up to you. I would also be happy to review a patch that replaced all our empty statements with empty {...} blocks (where not already present) containing an appropriate comment such as /* do nothing */ (where not already present).
Created attachment 106801 [details] [review] Describe quoting for match rules I wish I could say "I can't believe this was never documented", but it wouldn't be true.
Created attachment 106802 [details] [review] [PATCH 1/5] Implement GetAllMatchRules on the Stats interface v3: fixes suggested by smcv in Comment #26 - Use "DBusList **" types - Use "for" loops
Comment on attachment 106802 [details] [review] [PATCH 1/5] Implement GetAllMatchRules on the Stats interface Review of attachment 106802 [details] [review]: ----------------------------------------------------------------- Looks good. I'll apply these.
Comment on attachment 106801 [details] [review] Describe quoting for match rules Review of attachment 106801 [details] [review]: ----------------------------------------------------------------- The patch looks good. To be honest, I wonder whether it would be better to change the implementation (and spec) to be able to escape a quote inside single quotes. It would *probably* not break existing applications. I'm relieved that D-Bus well-known names, interface names, path names and member names are not allowed to contain a quote. Otherwise, it would break g_dbus_connection_signal_subscribe() because it does not escape quotes when building the match rule string. NameOwnerChanged rules with arg0 are fine too because arg0 is a well-known/unique name without a quote. But it worth checking if g_dbus_connection_signal_subscribe() is used with a non-NameOwnerChanged rule with arg0 by some projects, especially if arg0 comes from user input. http://codesearch.debian.net/search?q=g_dbus_connection_signal_subscribe
(In reply to comment #36) > To be honest, I wonder whether it would be better to change the > implementation (and spec) to be able to escape a quote inside single quotes. > It would *probably* not break existing applications. I would be inclined to just fix any existing implementors that are wrong (e.g. GDBus). Breaking API seems like a riskier fix, and documenting existing behaviour means there is a reference that people can use.
Created attachment 106846 [details] [review] Use ISO C strchr() instead of BSD index() --- Sorry, I didn't spot this while reviewing Attachment #106799 [details]. I'm not 100% sure how portable index() or <strings.h> are, and there's a straightforward replacement with a better name in ISO C, so we should use that. OK to apply?
Comment on attachment 106846 [details] [review] Use ISO C strchr() instead of BSD index() Review of attachment 106846 [details] [review]: ----------------------------------------------------------------- It looks good to me.
Fixed in git for 1.9.0, thanks
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.