From f3be583b40dadfd78ddefbc9fb3fa182bafde949 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 6 Nov 2015 15:52:51 +0100 Subject: [PATCH] monitor: use the addressed_recipient to select matches This means we respect the destination keyword in arguments to BecomeMonitor. In bus_dispatch(), this means that we need to defer capturing until we have decided whether there is an addressed recipient; so instead of capturing once, we capture at each leaf of the decision tree. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92074 Reviewed-by: Philip Withnall Reviewed-by: Lars Uebernickel --- bus/activation.c | 10 ++- bus/connection.c | 13 ++-- bus/connection.h | 2 + bus/dispatch.c | 56 +++++++++++++--- bus/driver.c | 2 +- bus/signals.c | 15 ++++- test/monitor.c | 197 +++++++++++++++++++++++++++++++++++++++++++++++-------- 7 files changed, 246 insertions(+), 49 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index ada3ded..76f5a88 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -1964,6 +1964,7 @@ bus_activation_activate_service (BusActivation *activation, DBusString service_string; BusService *service; BusRegistry *registry; + DBusConnection *systemd = NULL; /* OK, we have a systemd service configured for this entry, hence let's enqueue an activation request message. This @@ -2012,11 +2013,14 @@ bus_activation_activate_service (BusActivation *activation, _dbus_string_init_const (&service_string, "org.freedesktop.systemd1"); service = bus_registry_lookup (registry, &service_string); + if (service) + systemd = bus_service_get_primary_owners_connection (service); + /* Following the general principle of "log early and often", * we capture that we *want* to send the activation message, even if * systemd is not actually there to receive it yet */ if (!bus_transaction_capture (activation_transaction, - NULL, message)) + NULL, systemd, message)) { dbus_message_unref (message); BUS_SET_OOM (error); @@ -2030,8 +2034,8 @@ bus_activation_activate_service (BusActivation *activation, service_name, entry->systemd_service); /* Wonderful, systemd is connected, let's just send the msg */ - retval = bus_dispatch_matches (activation_transaction, NULL, bus_service_get_primary_owners_connection (service), - message, error); + retval = bus_dispatch_matches (activation_transaction, NULL, + systemd, message, error); } else { diff --git a/bus/connection.c b/bus/connection.c index 95e20a6..67793ba 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -2182,6 +2182,7 @@ bus_transaction_get_context (BusTransaction *transaction) dbus_bool_t bus_transaction_capture (BusTransaction *transaction, DBusConnection *sender, + DBusConnection *addressed_recipient, DBusMessage *message) { BusConnections *connections; @@ -2201,8 +2202,8 @@ bus_transaction_capture (BusTransaction *transaction, * There's little point, since there is up to 1 per process. */ _dbus_assert (mm != NULL); - if (!bus_matchmaker_get_recipients (mm, connections, sender, NULL, message, - &recipients)) + if (!bus_matchmaker_get_recipients (mm, connections, sender, + addressed_recipient, message, &recipients)) goto out; for (link = _dbus_list_get_first_link (&recipients); @@ -2224,6 +2225,7 @@ out: dbus_bool_t bus_transaction_capture_error_reply (BusTransaction *transaction, + DBusConnection *addressed_recipient, const DBusError *error, DBusMessage *in_reply_to) { @@ -2250,7 +2252,7 @@ bus_transaction_capture_error_reply (BusTransaction *transaction, if (!dbus_message_set_sender (reply, DBUS_SERVICE_DBUS)) goto out; - ret = bus_transaction_capture (transaction, NULL, reply); + ret = bus_transaction_capture (transaction, NULL, addressed_recipient, reply); out: dbus_message_unref (reply); @@ -2292,7 +2294,7 @@ bus_transaction_send_from_driver (BusTransaction *transaction, /* Capture it for monitors, even if the real recipient's receive policy * does not allow it to receive this message from us (which would be odd). */ - if (!bus_transaction_capture (transaction, NULL, message)) + if (!bus_transaction_capture (transaction, NULL, connection, message)) return FALSE; /* If security policy doesn't allow the message, we would silently @@ -2304,7 +2306,8 @@ bus_transaction_send_from_driver (BusTransaction *transaction, transaction, NULL, connection, connection, message, &error)) { - if (!bus_transaction_capture_error_reply (transaction, &error, message)) + if (!bus_transaction_capture_error_reply (transaction, connection, + &error, message)) { bus_context_log (transaction->context, DBUS_SYSTEM_LOG_WARNING, "message from dbus-daemon rejected but not enough " diff --git a/bus/connection.h b/bus/connection.h index 8c68d0a..746f4eb 100644 --- a/bus/connection.h +++ b/bus/connection.h @@ -134,8 +134,10 @@ dbus_bool_t bus_transaction_send (BusTransaction * DBusMessage *message); dbus_bool_t bus_transaction_capture (BusTransaction *transaction, DBusConnection *connection, + DBusConnection *addressed_recipient, DBusMessage *message); dbus_bool_t bus_transaction_capture_error_reply (BusTransaction *transaction, + DBusConnection *addressed_recipient, const DBusError *error, DBusMessage *in_reply_to); dbus_bool_t bus_transaction_send_from_driver (BusTransaction *transaction, diff --git a/bus/dispatch.c b/bus/dispatch.c index edfa1b4..9a6eaaf 100644 --- a/bus/dispatch.c +++ b/bus/dispatch.c @@ -72,8 +72,8 @@ send_one_message (DBusConnection *connection, message, &stack_error)) { - if (!bus_transaction_capture_error_reply (transaction, &stack_error, - message)) + if (!bus_transaction_capture_error_reply (transaction, sender, + &stack_error, message)) { bus_context_log (context, DBUS_SYSTEM_LOG_WARNING, "broadcast rejected, but not enough " @@ -93,8 +93,8 @@ send_one_message (DBusConnection *connection, bus_connection_get_name (connection), bus_connection_get_loginfo (connection)); - if (!bus_transaction_capture_error_reply (transaction, &stack_error, - message)) + if (!bus_transaction_capture_error_reply (transaction, sender, + &stack_error, message)) { bus_context_log (context, DBUS_SYSTEM_LOG_WARNING, "broadcast with Unix fd not delivered, but not " @@ -370,15 +370,15 @@ bus_dispatch (DBusConnection *connection, */ service_name = dbus_message_get_destination (message); - if (!bus_transaction_capture (transaction, connection, message)) - { - BUS_SET_OOM (&error); - goto out; - } - if (service_name && strcmp (service_name, DBUS_SERVICE_DBUS) == 0) /* to bus driver */ { + if (!bus_transaction_capture (transaction, connection, NULL, message)) + { + BUS_SET_OOM (&error); + goto out; + } + if (!bus_context_check_security_policy (context, transaction, connection, NULL, NULL, message, &error)) { @@ -392,6 +392,12 @@ bus_dispatch (DBusConnection *connection, } else if (!bus_connection_is_active (connection)) /* clients must talk to bus driver first */ { + if (!bus_transaction_capture (transaction, connection, NULL, message)) + { + BUS_SET_OOM (&error); + goto out; + } + _dbus_verbose ("Received message from non-registered client. Disconnecting.\n"); dbus_connection_close (connection); goto out; @@ -412,6 +418,14 @@ bus_dispatch (DBusConnection *connection, if (service == NULL && dbus_message_get_auto_start (message)) { BusActivation *activation; + + if (!bus_transaction_capture (transaction, connection, NULL, + message)) + { + BUS_SET_OOM (&error); + goto out; + } + /* We can't do the security policy check here, since the addressed * recipient service doesn't exist yet. We do it before sending the * message after the service has been created. @@ -430,6 +444,13 @@ bus_dispatch (DBusConnection *connection, } else if (service == NULL) { + if (!bus_transaction_capture (transaction, connection, + NULL, message)) + { + BUS_SET_OOM (&error); + goto out; + } + dbus_set_error (&error, DBUS_ERROR_NAME_HAS_NO_OWNER, "Name \"%s\" does not exist", @@ -440,6 +461,21 @@ bus_dispatch (DBusConnection *connection, { addressed_recipient = bus_service_get_primary_owners_connection (service); _dbus_assert (addressed_recipient != NULL); + + if (!bus_transaction_capture (transaction, connection, + addressed_recipient, message)) + { + BUS_SET_OOM (&error); + goto out; + } + } + } + else /* service_name == NULL */ + { + if (!bus_transaction_capture (transaction, connection, NULL, message)) + { + BUS_SET_OOM (&error); + goto out; } } diff --git a/bus/driver.c b/bus/driver.c index 852ac53..eba7b98 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -243,7 +243,7 @@ bus_driver_send_service_owner_changed (const char *service_name, _dbus_assert (dbus_message_has_signature (message, "sss")); - if (!bus_transaction_capture (transaction, NULL, message)) + if (!bus_transaction_capture (transaction, NULL, NULL, message)) goto oom; retval = bus_dispatch_matches (transaction, NULL, NULL, message, error); diff --git a/bus/signals.c b/bus/signals.c index e8def9f..2965f6e 100644 --- a/bus/signals.c +++ b/bus/signals.c @@ -1887,9 +1887,18 @@ match_rule_matches (BusMatchRule *rule, return FALSE; if (addressed_recipient == NULL) - { - if (strcmp (rule->destination, - DBUS_SERVICE_DBUS) != 0) + { + /* If the message is going to be delivered to the dbus-daemon + * itself, its destination will be "org.freedesktop.DBus", + * which we again match against the rule (see bus_dispatch() + * in bus/dispatch.c, which checks for o.fd.DBus first). + * + * If we are monitoring and we don't know who is going to receive + * the message (for instance because they haven't been activated yet), + * assume they will own the requested destination name and no other, + * and match the rule's destination against that. + */ + if (strcmp (rule->destination, destination) != 0) return FALSE; } else diff --git a/test/monitor.c b/test/monitor.c index ad4b195..914e539 100644 --- a/test/monitor.c +++ b/test/monitor.c @@ -92,6 +92,11 @@ static const char * const selective_match_rules[] = { FALSE }; +static const char * const well_known_destination_match_rules[] = { + "destination='com.example.Recipient'", + NULL +}; + static Config forbidding_config = { "valid-config-files/forbidding.conf", NULL, @@ -110,6 +115,12 @@ static Config selective_config = { FALSE }; +static Config well_known_destination_config = { + NULL, + well_known_destination_match_rules, + FALSE +}; + static Config no_rules_config = { NULL, no_match_rules, @@ -415,6 +426,19 @@ activated_filter (DBusConnection *connection, } static void +take_well_known_name (Fixture *f, + DBusConnection *connection, + const char *name) +{ + int ret; + + ret = dbus_bus_request_name (connection, name, + DBUS_NAME_FLAG_DO_NOT_QUEUE, &f->e); + test_assert_no_error (&f->e); + g_assert_cmpint (ret, ==, DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER); +} + +static void setup (Fixture *f, gconstpointer context) { @@ -446,7 +470,8 @@ setup (Fixture *f, } static void -become_monitor (Fixture *f) +become_monitor (Fixture *f, + const Config *config) { DBusMessage *m; DBusPendingCall *pc; @@ -458,8 +483,11 @@ become_monitor (Fixture *f) dbus_connection_set_route_peer_messages (f->monitor, TRUE); - if (f->config != NULL && f->config->match_rules != NULL) - match_rules = f->config->match_rules; + if (config == NULL) + config = f->config; + + if (config != NULL && config->match_rules != NULL) + match_rules = config->match_rules; else match_rules = wildcard_match_rules; @@ -746,7 +774,7 @@ test_become_monitor (Fixture *f, } } - become_monitor (f); + become_monitor (f, NULL); while (!lost_unique || !lost_a || !lost_b || !lost_c) { @@ -848,7 +876,7 @@ test_broadcast (Fixture *f, dbus_bus_add_match (f->recipient, "type='signal'", &f->e); test_assert_no_error (&f->e); - become_monitor (f); + become_monitor (f, NULL); m = dbus_message_new_signal ("/foo", "com.example.bar", "BroadcastSignal1"); dbus_connection_send (f->sender, m, NULL); @@ -896,7 +924,7 @@ test_forbidden_broadcast (Fixture *f, dbus_bus_add_match (f->recipient, "type='signal'", &f->e); test_assert_no_error (&f->e); - become_monitor (f); + become_monitor (f, NULL); m = dbus_message_new_signal ("/foo", "com.example.CannotSend", "BroadcastSignal1"); @@ -959,7 +987,7 @@ test_unicast_signal (Fixture *f, if (f->address == NULL) return; - become_monitor (f); + become_monitor (f, NULL); m = dbus_message_new_signal ("/foo", "com.example.bar", "UnicastSignal1"); if (!dbus_message_set_destination (m, f->recipient_name)) @@ -1010,7 +1038,7 @@ test_forbidden (Fixture *f, if (f->address == NULL) return; - become_monitor (f); + become_monitor (f, NULL); m = dbus_message_new_signal ("/foo", "com.example.CannotSend", "UnicastSignal1"); @@ -1079,7 +1107,7 @@ test_method_call (Fixture *f, if (f->address == NULL) return; - become_monitor (f); + become_monitor (f, NULL); /* regression test for * https://bugs.freedesktop.org/show_bug.cgi?id=90952 */ @@ -1134,7 +1162,7 @@ test_forbidden_method_call (Fixture *f, if (f->address == NULL) return; - become_monitor (f); + become_monitor (f, NULL); m = dbus_message_new_method_call (f->recipient_name, "/foo", "com.example.CannotSend", "Call1"); @@ -1189,7 +1217,7 @@ test_dbus_daemon (Fixture *f, if (f->address == NULL) return; - become_monitor (f); + become_monitor (f, NULL); res = dbus_bus_request_name (f->sender, "com.example.Sender", DBUS_NAME_FLAG_DO_NOT_QUEUE, &f->e); @@ -1266,7 +1294,7 @@ test_selective (Fixture *f, "eavesdrop='true',interface='com.example.Tedious'", &f->e); test_assert_no_error (&f->e); - become_monitor (f); + become_monitor (f, NULL); m = dbus_message_new_signal ("/foo", "com.example.Interesting", "UnicastSignal1"); @@ -1309,6 +1337,129 @@ test_selective (Fixture *f, g_assert (m == NULL); } +static void +test_well_known_destination (Fixture *f, + gconstpointer context) +{ + DBusMessage *m; + + if (f->address == NULL) + return; + + take_well_known_name (f, f->recipient, "com.example.Recipient"); + /* we don't expect_take_well_known_name here because the + * monitor isn't up yet */ + + become_monitor (f, NULL); + + /* The sender sends a message to itself. It will not be observed. */ + m = dbus_message_new_signal ("/foo", "com.example.bar", "Unobserved"); + if (!dbus_message_set_destination (m, f->sender_name)) + g_error ("OOM"); + dbus_connection_send (f->sender, m, NULL); + dbus_message_unref (m); + + /* The sender sends a message to the recipient by well-known name. + * It will be observed. */ + m = dbus_message_new_signal ("/foo", "com.example.bar", "Observed1"); + if (!dbus_message_set_destination (m, "com.example.Recipient")) + g_error ("OOM"); + dbus_connection_send (f->sender, m, NULL); + dbus_message_unref (m); + + /* The sender sends a message to the recipient by unique name. + * It will still be observed. */ + m = dbus_message_new_signal ("/foo", "com.example.bar", "Observed2"); + if (!dbus_message_set_destination (m, f->recipient_name)) + g_error ("OOM"); + dbus_connection_send (f->sender, m, NULL); + dbus_message_unref (m); + + while (g_queue_get_length (&f->monitored) < 2) + test_main_context_iterate (f->ctx, TRUE); + + m = g_queue_pop_head (&f->monitored); + assert_signal (m, f->sender_name, "/foo", "com.example.bar", + "Observed1", "", "com.example.Recipient"); + dbus_message_unref (m); + + m = g_queue_pop_head (&f->monitored); + assert_signal (m, f->sender_name, "/foo", "com.example.bar", + "Observed2", "", f->recipient_name); + dbus_message_unref (m); + + m = g_queue_pop_head (&f->monitored); + g_assert (m == NULL); +} + +static void +test_unique_destination (Fixture *f, + gconstpointer context) +{ + DBusMessage *m; + Config config = { + NULL, + NULL, /* match rules */ + FALSE + }; + const gchar *match_rules[2] = { NULL, NULL }; + gchar *rule; + + if (f->address == NULL) + return; + + take_well_known_name (f, f->recipient, "com.example.Recipient"); + /* we don't expect_take_well_known_name here because the + * monitor isn't up yet */ + + rule = g_strdup_printf ("destination='%s'", f->recipient_name); + /* free it later */ + g_test_queue_free (rule); + match_rules[0] = rule; + config.match_rules = match_rules; + + become_monitor (f, &config); + + /* The sender sends a message to itself. It will not be observed. */ + m = dbus_message_new_signal ("/foo", "com.example.bar", "Unobserved"); + if (!dbus_message_set_destination (m, f->sender_name)) + g_error ("OOM"); + dbus_connection_send (f->sender, m, NULL); + dbus_message_unref (m); + + /* The sender sends a message to the recipient by well-known name. + * It will be observed. */ + m = dbus_message_new_signal ("/foo", "com.example.bar", "Observed1"); + if (!dbus_message_set_destination (m, "com.example.Recipient")) + g_error ("OOM"); + dbus_connection_send (f->sender, m, NULL); + dbus_message_unref (m); + + /* The sender sends a message to the recipient by unique name. + * It will still be observed. */ + m = dbus_message_new_signal ("/foo", "com.example.bar", "Observed2"); + if (!dbus_message_set_destination (m, f->recipient_name)) + g_error ("OOM"); + dbus_connection_send (f->sender, m, NULL); + dbus_message_unref (m); + + while (g_queue_get_length (&f->monitored) < 2) + test_main_context_iterate (f->ctx, TRUE); + + m = g_queue_pop_head (&f->monitored); + assert_signal (m, f->sender_name, "/foo", "com.example.bar", + "Observed1", "", "com.example.Recipient"); + dbus_message_unref (m); + + m = g_queue_pop_head (&f->monitored); + assert_signal (m, f->sender_name, "/foo", "com.example.bar", + "Observed2", "", f->recipient_name); + dbus_message_unref (m); + + m = g_queue_pop_head (&f->monitored); + g_assert (m == NULL); +} + #ifdef DBUS_UNIX /* currently only used for the systemd activation test */ static void @@ -1339,20 +1490,6 @@ expect_new_connection (Fixture *f) /* currently only used for the systemd activation test */ static void -take_well_known_name (Fixture *f, - DBusConnection *connection, - const char *name) -{ - int ret; - - ret = dbus_bus_request_name (connection, name, - DBUS_NAME_FLAG_DO_NOT_QUEUE, &f->e); - test_assert_no_error (&f->e); - g_assert_cmpint (ret, ==, DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER); -} - -/* currently only used for the systemd activation test */ -static void expect_take_well_known_name (Fixture *f, DBusConnection *connection, const char *name) @@ -1392,7 +1529,7 @@ test_activation (Fixture *f, if (f->address == NULL) return; - become_monitor (f); + become_monitor (f, NULL); /* The sender sends a message to an activatable service. */ m = dbus_message_new_signal ("/foo", "com.example.bar", "UnicastSignal1"); @@ -1666,6 +1803,12 @@ main (int argc, setup, test_dbus_daemon, teardown); g_test_add ("/monitor/selective", Fixture, &selective_config, setup, test_selective, teardown); + g_test_add ("/monitor/well-known-destination", + Fixture, &well_known_destination_config, + setup, test_well_known_destination, teardown); + g_test_add ("/monitor/unique-destination", + Fixture, NULL, + setup, test_unique_destination, teardown); g_test_add ("/monitor/wildcard", Fixture, &wildcard_config, setup, test_unicast_signal, teardown); g_test_add ("/monitor/no-rule", Fixture, &no_rules_config, -- 2.7.4