From 3251264ac483680b4a5fe808729f7e3b34f41fd4 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Tue, 14 Oct 2003 22:16:03 +0000 Subject: 2003-10-14 Havoc Pennington * bus/bus.c (bus_context_check_security_policy): revamp this to work more sanely with new policy-based requested reply setup * bus/connection.c (bus_transaction_send_from_driver): set bus driver messages as no reply * bus/policy.c (bus_client_policy_check_can_receive): handle a requested_reply attribute on allow/deny rules * bus/system.conf: add * bus/driver.c (bus_driver_handle_message): fix check for replies sent to the bus driver, which was backward. How did this ever work at all though? I think I'm missing something. * dbus/dbus-message.c (decode_header_data): require error and method return messages to have a reply serial field to be valid (_dbus_message_loader_queue_messages): break up this function; validate that reply serial and plain serial are nonzero; clean up the OOM/error handling. (get_uint_field): don't return -1 from this (dbus_message_create_header): fix signed/unsigned bug * bus/connection.c (bus_connections_expect_reply): save serial of the incoming message, not reply serial --- ChangeLog | 28 +++ Makefile.am | 7 + bus/bus.c | 97 ++++++---- bus/config-parser.c | 36 +++- bus/connection.c | 14 +- bus/dbus-daemon-1.1.in | 25 ++- bus/driver.c | 2 +- bus/policy.c | 29 +++ bus/policy.h | 4 +- dbus/dbus-internals.c | 2 +- dbus/dbus-message.c | 453 ++++++++++++++++++++++++++------------------- doc/TODO | 8 - doc/dbus-specification.xml | 5 + 13 files changed, 454 insertions(+), 256 deletions(-) diff --git a/ChangeLog b/ChangeLog index 123bed04..91618fcb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,31 @@ +2003-10-14 Havoc Pennington + + * bus/bus.c (bus_context_check_security_policy): revamp this to + work more sanely with new policy-based requested reply setup + + * bus/connection.c (bus_transaction_send_from_driver): set bus + driver messages as no reply + + * bus/policy.c (bus_client_policy_check_can_receive): handle a + requested_reply attribute on allow/deny rules + + * bus/system.conf: add + + * bus/driver.c (bus_driver_handle_message): fix check for replies + sent to the bus driver, which was backward. How did this ever work + at all though? I think I'm missing something. + + * dbus/dbus-message.c (decode_header_data): require error and + method return messages to have a reply serial field to be valid + (_dbus_message_loader_queue_messages): break up this function; + validate that reply serial and plain serial are nonzero; + clean up the OOM/error handling. + (get_uint_field): don't return -1 from this + (dbus_message_create_header): fix signed/unsigned bug + + * bus/connection.c (bus_connections_expect_reply): save serial of + the incoming message, not reply serial + 2003-10-14 Havoc Pennington * bus/connection.c: implement pending reply tracking using diff --git a/Makefile.am b/Makefile.am index 47329972..200ee64e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -20,6 +20,13 @@ if HAVE_PYTHON PYTHON_SUBDIR=python endif +## really we should require gcj/mcs/python also but since they are +## annoying to install, we don't for now +dist-local: + if test -z "$(QT_SUBDIR)" || test -z "$(GLIB_SUBDIR)" ; then \ + echo "You have to build with Qt and GLib to make dist" ; \ + fi + SUBDIRS=dbus bus doc $(GLIB_SUBDIR) $(GCJ_SUBDIR) $(MONO_SUBDIR) $(QT_SUBDIR) $(PYTHON_SUBDIR) test tools pkgconfigdir = $(libdir)/pkgconfig diff --git a/bus/bus.c b/bus/bus.c index 27506240..a00201af 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -909,6 +909,7 @@ bus_context_check_security_policy (BusContext *context, BusClientPolicy *sender_policy; BusClientPolicy *recipient_policy; int type; + dbus_bool_t requested_reply; type = dbus_message_get_type (message); @@ -919,49 +920,54 @@ bus_context_check_security_policy (BusContext *context, addressed_recipient != NULL || strcmp (dbus_message_get_destination (message), DBUS_SERVICE_ORG_FREEDESKTOP_DBUS) == 0); + switch (type) + { + case DBUS_MESSAGE_TYPE_METHOD_CALL: + case DBUS_MESSAGE_TYPE_SIGNAL: + case DBUS_MESSAGE_TYPE_METHOD_RETURN: + case DBUS_MESSAGE_TYPE_ERROR: + break; + + default: + _dbus_verbose ("security check disallowing message of unknown type %d\n", + type); + + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "Message bus will not accept messages of unknown type\n"); + + return FALSE; + } + + requested_reply = FALSE; + if (sender != NULL) { if (bus_connection_is_active (sender)) { sender_policy = bus_connection_get_policy (sender); _dbus_assert (sender_policy != NULL); - - switch (type) + + /* Fill in requested_reply variable with TRUE if this is a + * reply and the reply was pending. + */ + if (dbus_message_get_reply_serial (message) != 0) { - case DBUS_MESSAGE_TYPE_METHOD_CALL: - case DBUS_MESSAGE_TYPE_SIGNAL: - - /* Continue below to check security policy */ - break; - - case DBUS_MESSAGE_TYPE_METHOD_RETURN: - case DBUS_MESSAGE_TYPE_ERROR: - /* These are only allowed if the reply is listed - * as pending, or the connection is eavesdropping. - * The idea is to prohibit confusing/fake replies. - * FIXME In principle a client that's asked to eavesdrop - * specifically should probably get bogus replies - * even to itself, but here we prohibit that. - */ - if (proposed_recipient != NULL /* not to the bus driver */ && - addressed_recipient == proposed_recipient /* not eavesdropping */ && - !bus_connections_check_reply (bus_connection_get_connections (sender), - transaction, - sender, addressed_recipient, message, - error)) - return FALSE; - - /* Continue below to check security policy, since reply was expected */ - break; - - default: - _dbus_verbose ("security check disallowing message of unknown type\n"); - - dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, - "Message bus will not accept messages of unknown type\n"); - - return FALSE; + addressed_recipient == proposed_recipient /* not eavesdropping */) + { + DBusError error2; + + dbus_error_init (&error2); + requested_reply = bus_connections_check_reply (bus_connection_get_connections (sender), + transaction, + sender, addressed_recipient, message, + &error2); + if (dbus_error_is_set (&error2)) + { + dbus_move_error (&error2, error); + return FALSE; + } + } } } else @@ -992,7 +998,16 @@ bus_context_check_security_policy (BusContext *context, } } else - sender_policy = NULL; + { + sender_policy = NULL; + + /* If the sender is the bus driver, we assume any reply was a + * requested reply as bus driver won't send bogus ones + */ + if (addressed_recipient == proposed_recipient /* not eavesdropping */ && + dbus_message_get_reply_serial (message) != 0) + requested_reply = TRUE; + } _dbus_assert ((sender != NULL && sender_policy != NULL) || (sender == NULL && sender_policy == NULL)); @@ -1050,7 +1065,9 @@ bus_context_check_security_policy (BusContext *context, if (recipient_policy && !bus_client_policy_check_can_receive (recipient_policy, - context->registry, sender, + context->registry, + requested_reply, + sender, addressed_recipient, proposed_recipient, message)) { @@ -1059,14 +1076,16 @@ bus_context_check_security_policy (BusContext *context, "A security policy in place prevents this recipient " "from receiving this message from this sender, " "see message bus configuration file (rejected message " - "had interface \"%s\" member \"%s\" error name \"%s\" destination \"%s\")", + "had interface \"%s\" member \"%s\" error name \"%s\" destination \"%s\" reply serial %u requested_reply=%d)", dbus_message_get_interface (message) ? dbus_message_get_interface (message) : "(unset)", dbus_message_get_member (message) ? dbus_message_get_member (message) : "(unset)", dbus_message_get_error_name (message) ? dbus_message_get_error_name (message) : "(unset)", - dest ? dest : DBUS_SERVICE_ORG_FREEDESKTOP_DBUS); + dest ? dest : DBUS_SERVICE_ORG_FREEDESKTOP_DBUS, + dbus_message_get_reply_serial (message), + requested_reply); _dbus_verbose ("security policy disallowing message due to recipient policy\n"); return FALSE; } diff --git a/bus/config-parser.c b/bus/config-parser.c index b3652591..cc55a492 100644 --- a/bus/config-parser.c +++ b/bus/config-parser.c @@ -837,6 +837,7 @@ append_rule_from_element (BusConfigParser *parser, const char *receive_path; const char *receive_type; const char *eavesdrop; + const char *requested_reply; const char *own; const char *user; const char *group; @@ -859,6 +860,7 @@ append_rule_from_element (BusConfigParser *parser, "receive_path", &receive_path, "receive_type", &receive_type, "eavesdrop", &eavesdrop, + "requested_reply", &requested_reply, "own", &own, "user", &user, "group", &group, @@ -868,7 +870,7 @@ append_rule_from_element (BusConfigParser *parser, if (!(send_interface || send_member || send_error || send_destination || send_type || send_path || receive_interface || receive_member || receive_error || receive_sender || - receive_type || receive_path || eavesdrop || + receive_type || receive_path || eavesdrop || requested_reply || own || user || group)) { dbus_set_error (error, DBUS_ERROR_FAILED, @@ -895,7 +897,7 @@ append_rule_from_element (BusConfigParser *parser, * error * * base send_ can combine with send_destination, send_path, send_type - * base receive_ with receive_sender, receive_path, receive_type, eavesdrop + * base receive_ with receive_sender, receive_path, receive_type, eavesdrop, requested_reply * * user, group, own must occur alone * @@ -908,6 +910,7 @@ append_rule_from_element (BusConfigParser *parser, (send_interface && receive_error) || (send_interface && receive_sender) || (send_interface && eavesdrop) || + (send_interface && requested_reply) || (send_interface && own) || (send_interface && user) || (send_interface && group)) || @@ -918,6 +921,7 @@ append_rule_from_element (BusConfigParser *parser, (send_member && receive_error) || (send_member && receive_sender) || (send_member && eavesdrop) || + (send_member && requested_reply) || (send_member && own) || (send_member && user) || (send_member && group)) || @@ -927,6 +931,7 @@ append_rule_from_element (BusConfigParser *parser, (send_error && receive_error) || (send_error && receive_sender) || (send_error && eavesdrop) || + (send_error && requested_reply) || (send_error && own) || (send_error && user) || (send_error && group)) || @@ -936,6 +941,7 @@ append_rule_from_element (BusConfigParser *parser, (send_destination && receive_error) || (send_destination && receive_sender) || (send_destination && eavesdrop) || + (send_destination && requested_reply) || (send_destination && own) || (send_destination && user) || (send_destination && group)) || @@ -945,6 +951,7 @@ append_rule_from_element (BusConfigParser *parser, (send_type && receive_error) || (send_type && receive_sender) || (send_type && eavesdrop) || + (send_type && requested_reply) || (send_type && own) || (send_type && user) || (send_type && group)) || @@ -954,6 +961,7 @@ append_rule_from_element (BusConfigParser *parser, (send_path && receive_error) || (send_path && receive_sender) || (send_path && eavesdrop) || + (send_path && requested_reply) || (send_path && own) || (send_path && user) || (send_path && group)) || @@ -975,6 +983,10 @@ append_rule_from_element (BusConfigParser *parser, ((eavesdrop && own) || (eavesdrop && user) || (eavesdrop && group)) || + + ((requested_reply && own) || + (requested_reply && user) || + (requested_reply && group)) || ((own && user) || (own && group)) || @@ -1047,7 +1059,7 @@ append_rule_from_element (BusConfigParser *parser, goto nomem; } else if (receive_interface || receive_member || receive_error || receive_sender || - receive_path || receive_type || eavesdrop) + receive_path || receive_type || eavesdrop || requested_reply) { int message_type; @@ -1083,8 +1095,18 @@ append_rule_from_element (BusConfigParser *parser, strcmp (eavesdrop, "false") == 0)) { dbus_set_error (error, DBUS_ERROR_FAILED, - "Bad value \"%s\" for eavesdrop attribute, must be true or false", - eavesdrop); + "Bad value \"%s\" for %s attribute, must be true or false", + "eavesdrop", eavesdrop); + return FALSE; + } + + if (requested_reply && + !(strcmp (requested_reply, "true") == 0 || + strcmp (requested_reply, "false") == 0)) + { + dbus_set_error (error, DBUS_ERROR_FAILED, + "Bad value \"%s\" for %s attribute, must be true or false", + "requested_reply", requested_reply); return FALSE; } @@ -1094,6 +1116,9 @@ append_rule_from_element (BusConfigParser *parser, if (eavesdrop) rule->d.receive.eavesdrop = (strcmp (eavesdrop, "true") == 0); + + if (requested_reply) + rule->d.receive.requested_reply = (strcmp (requested_reply, "true") == 0); rule->d.receive.message_type = message_type; rule->d.receive.path = _dbus_strdup (receive_path); @@ -1101,6 +1126,7 @@ append_rule_from_element (BusConfigParser *parser, rule->d.receive.member = _dbus_strdup (receive_member); rule->d.receive.error = _dbus_strdup (receive_error); rule->d.receive.origin = _dbus_strdup (receive_sender); + if (receive_path && rule->d.receive.path == NULL) goto nomem; if (receive_interface && rule->d.receive.interface == NULL) diff --git a/bus/connection.c b/bus/connection.c index 2ae1d7d2..1e562427 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -1510,7 +1510,7 @@ bus_connections_expect_reply (BusConnections *connections, if (dbus_message_get_no_reply (reply_to_this)) return TRUE; /* we won't allow a reply, since client doesn't care for one. */ - reply_serial = dbus_message_get_reply_serial (reply_to_this); + reply_serial = dbus_message_get_serial (reply_to_this); link = _dbus_list_get_first_link (&connections->pending_replies->items); while (link != NULL) @@ -1651,13 +1651,8 @@ bus_connections_check_reply (BusConnections *connections, if (link == NULL) { - _dbus_verbose ("No pending reply expected, disallowing this reply\n"); - - dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, - "%s message sent with reply serial %u, but no such reply was requested (or it has timed out already)\n", - dbus_message_get_type (reply) == DBUS_MESSAGE_TYPE_METHOD_RETURN ? - "method return" : "error", - reply_serial); + _dbus_verbose ("No pending reply expected\n"); + return FALSE; } @@ -1807,6 +1802,9 @@ bus_transaction_send_from_driver (BusTransaction *transaction, if (!dbus_message_set_sender (message, DBUS_SERVICE_ORG_FREEDESKTOP_DBUS)) return FALSE; + /* bus driver never wants a reply */ + dbus_message_set_no_reply (message, TRUE); + /* If security policy doesn't allow the message, we silently * eat it; the driver doesn't care about getting a reply. */ diff --git a/bus/dbus-daemon-1.1.in b/bus/dbus-daemon-1.1.in index 06bbbd13..7e186e51 100644 --- a/bus/dbus-daemon-1.1.in +++ b/bus/dbus-daemon-1.1.in @@ -356,7 +356,8 @@ The possible attributes of these elements are: receive_sender="service_name" receive_type="method_call" | "method_return" | "signal" | "error" receive_path="/path/name" - + + requested_reply="true" | "false" eavesdrop="true" | "false" own="servicename" @@ -377,7 +378,7 @@ Examples: .fi .PP -The attributes determine whether the deny "matches" a +The element's attributes determine whether the deny "matches" a particular action. If it matches, the action is denied (unless later rules in the config file allow it). @@ -408,6 +409,26 @@ also, but here it means that the rule applies always, even when not eavesdropping. The eavesdrop attribute can only be combined with receive rules (with receive_* attributes). + +.PP +The requested_reply attribute works similarly to the eavesdrop +attribute. It controls whether the or matches a reply +that is expected (corresponds to a previous method call message). +This attribute only makes sense for reply messages (errors and method +returns), and is ignored for other message types. + +.PP +For , requested_reply="true" is the default and indicates that +only requested replies are allowed by the +rule. requested_reply="false" means that the rule allows any reply +even if unexpected. + +.PP +For , requested_reply="false" is the default but indicates that +the rule matches only when the reply was not +requested. requested_reply="true" indicates that the rule applies +always, regardless of pending reply state. + .PP user and group denials mean that the given user or group may not connect to the message bus. diff --git a/bus/driver.c b/bus/driver.c index 791fcd69..4d345698 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -814,7 +814,7 @@ bus_driver_handle_message (DBusConnection *connection, /* security checks should have kept this from getting here */ _dbus_assert (sender != NULL || strcmp (name, "Hello") == 0); - if (dbus_message_get_reply_serial (message) == 0) + if (dbus_message_get_reply_serial (message) != 0) { _dbus_verbose ("Client sent a reply to the bus driver, ignoring it\n"); return TRUE; diff --git a/bus/policy.c b/bus/policy.c index 71137ca9..63131aca 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -56,6 +56,10 @@ bus_policy_rule_new (BusPolicyRuleType type, break; case BUS_POLICY_RULE_RECEIVE: rule->d.receive.message_type = DBUS_MESSAGE_TYPE_INVALID; + /* allow rules default to TRUE (only requested replies allowed) + * deny rules default to FALSE (only unrequested replies denied) + */ + rule->d.receive.requested_reply = rule->allow; break; case BUS_POLICY_RULE_OWN: break; @@ -919,6 +923,7 @@ bus_client_policy_check_can_send (BusClientPolicy *policy, dbus_bool_t bus_client_policy_check_can_receive (BusClientPolicy *policy, BusRegistry *registry, + dbus_bool_t requested_reply, DBusConnection *sender, DBusConnection *addressed_recipient, DBusConnection *proposed_recipient, @@ -978,6 +983,30 @@ bus_client_policy_check_can_receive (BusClientPolicy *policy, _dbus_verbose (" (policy) skipping deny rule since it only applies to eavesdropping\n"); continue; } + + /* If it's a reply, the requested_reply flag kicks in */ + if (dbus_message_get_reply_serial (message) != 0) + { + /* for allow, requested_reply=true means the rule applies + * only when reply was requested. requested_reply=false means + * always allow. + */ + if (!requested_reply && rule->allow && rule->d.receive.requested_reply) + { + _dbus_verbose (" (policy) skipping allow rule since it only applies to requested replies\n"); + continue; + } + + /* for deny, requested_reply=false means the rule applies only + * when the reply was not requested. requested_reply=true means the + * rule always applies. + */ + if (requested_reply && !rule->allow && !rule->d.receive.requested_reply) + { + _dbus_verbose (" (policy) skipping deny rule since it only applies to unrequested replies\n"); + continue; + } + } if (rule->d.receive.path != NULL) { diff --git a/bus/policy.h b/bus/policy.h index 63981cc0..17dfbf27 100644 --- a/bus/policy.h +++ b/bus/policy.h @@ -61,7 +61,7 @@ struct BusPolicyRule char *interface; char *member; char *error; - char *destination; + char *destination; } send; struct @@ -75,6 +75,7 @@ struct BusPolicyRule char *error; char *origin; unsigned int eavesdrop : 1; + unsigned int requested_reply : 1; } receive; struct @@ -134,6 +135,7 @@ dbus_bool_t bus_client_policy_check_can_send (BusClientPolicy *policy, DBusMessage *message); dbus_bool_t bus_client_policy_check_can_receive (BusClientPolicy *policy, BusRegistry *registry, + dbus_bool_t requested_reply, DBusConnection *sender, DBusConnection *addressed_recipient, DBusConnection *proposed_recipient, diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c index cf1cc391..44f3ff47 100644 --- a/dbus/dbus-internals.c +++ b/dbus/dbus-internals.c @@ -424,7 +424,7 @@ _dbus_header_field_to_string (int header_field) #ifndef DBUS_DISABLE_CHECKS /** String used in _dbus_return_if_fail macro */ const char _dbus_return_if_fail_warning_format[] = -"Arguments to %s were incorrect, assertion \"%s\" failed in file %s line %d.\n" +"Arguments to %s() were incorrect, assertion \"%s\" failed in file %s line %d.\n" "This is normally a bug in some application using the D-BUS library.\n"; #endif diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index 45309766..c7e0b8cf 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -217,7 +217,7 @@ get_uint_field (DBusMessage *message, offset = message->header_fields[field].value_offset; if (offset < 0) - return -1; /* useless if -1 is a valid value of course */ + return 0; /* useless if 0 is a valid value of course */ return _dbus_demarshal_uint32 (&message->header, message->byte_order, @@ -808,9 +808,11 @@ dbus_message_set_reply_serial (DBusMessage *message, } /** - * Returns the serial of a message or -1 if none has been specified. + * Returns the serial of a message or 0 if none has been specified. * The message's serial number is provided by the application sending - * the message and is used to identify replies to this message. + * the message and is used to identify replies to this message. All + * messages received on a connection will have a serial, but messages + * you haven't sent yet may return 0. * * @param message the message * @returns the client serial @@ -822,8 +824,7 @@ dbus_message_get_serial (DBusMessage *message) } /** - * Returns the serial that the message is - * a reply to or 0 if none. + * Returns the serial that the message is a reply to or 0 if none. * * @param message the message * @returns the reply serial @@ -957,13 +958,16 @@ dbus_message_create_header (DBusMessage *message, if (!_dbus_string_append_byte (&message->header, DBUS_MAJOR_PROTOCOL_VERSION)) return FALSE; + /* header length */ if (!_dbus_marshal_uint32 (&message->header, message->byte_order, 0)) return FALSE; + /* body length */ if (!_dbus_marshal_uint32 (&message->header, message->byte_order, 0)) return FALSE; - if (!_dbus_marshal_int32 (&message->header, message->byte_order, -1)) + /* serial */ + if (!_dbus_marshal_uint32 (&message->header, message->byte_order, 0)) return FALSE; /* Marshal all the fields (Marshall Fields?) */ @@ -5024,8 +5028,18 @@ decode_header_data (const DBusString *data, _dbus_verbose ("No error-name field provided\n"); return FALSE; } + if (fields[DBUS_HEADER_FIELD_REPLY_SERIAL].value_offset < 0) + { + _dbus_verbose ("No reply serial field provided in error\n"); + return FALSE; + } break; case DBUS_MESSAGE_TYPE_METHOD_RETURN: + if (fields[DBUS_HEADER_FIELD_REPLY_SERIAL].value_offset < 0) + { + _dbus_verbose ("No reply serial field provided in method return\n"); + return FALSE; + } break; default: /* An unknown type, spec requires us to ignore it */ @@ -5059,6 +5073,243 @@ _dbus_message_loader_return_buffer (DBusMessageLoader *loader, loader->buffer_outstanding = FALSE; } +static dbus_bool_t +load_one_message (DBusMessageLoader *loader, + int byte_order, + int message_type, + int header_len, + int body_len) +{ + DBusMessage *message; + HeaderField fields[DBUS_HEADER_FIELD_LAST + 1]; + int i; + int next_arg; + dbus_bool_t oom; + int header_padding; + + message = NULL; + oom = FALSE; + +#if 0 + _dbus_verbose_bytes_of_string (&loader->data, 0, header_len + body_len); +#endif + + if (!decode_header_data (&loader->data, + header_len, byte_order, + message_type, + fields, &header_padding)) + { + _dbus_verbose ("Header was invalid\n"); + loader->corrupted = TRUE; + goto failed; + } + + next_arg = header_len; + while (next_arg < (header_len + body_len)) + { + int type; + int prev = next_arg; + + if (!_dbus_marshal_validate_type (&loader->data, next_arg, + &type, &next_arg)) + { + _dbus_verbose ("invalid typecode at offset %d\n", prev); + loader->corrupted = TRUE; + goto failed; + } + + if (!_dbus_marshal_validate_arg (&loader->data, + byte_order, + 0, + type, -1, + next_arg, + &next_arg)) + { + _dbus_verbose ("invalid type data at %d, next_arg\n", next_arg); + loader->corrupted = TRUE; + goto failed; + } + + _dbus_assert (next_arg > prev); + } + + if (next_arg > (header_len + body_len)) + { + _dbus_verbose ("end of last arg at %d but message has len %d+%d=%d\n", + next_arg, header_len, body_len, + header_len + body_len); + loader->corrupted = TRUE; + goto failed; + } + + message = dbus_message_new_empty_header (); + if (message == NULL) + { + _dbus_verbose ("Failed to allocate empty message\n"); + oom = TRUE; + goto failed; + } + + message->byte_order = byte_order; + message->header_padding = header_padding; + + /* Copy in the offsets we found */ + i = 0; + while (i <= DBUS_HEADER_FIELD_LAST) + { + message->header_fields[i] = fields[i]; + ++i; + } + + if (!_dbus_list_append (&loader->messages, message)) + { + _dbus_verbose ("Failed to append new message to loader queue\n"); + oom = TRUE; + goto failed; + } + + _dbus_assert (_dbus_string_get_length (&message->header) == 0); + _dbus_assert (_dbus_string_get_length (&message->body) == 0); + + _dbus_assert (_dbus_string_get_length (&loader->data) >= + (header_len + body_len)); + + if (!_dbus_string_move_len (&loader->data, 0, header_len, &message->header, 0)) + { + _dbus_verbose ("Failed to move header into new message\n"); + oom = TRUE; + goto failed; + } + + if (!_dbus_string_move_len (&loader->data, 0, body_len, &message->body, 0)) + { + _dbus_verbose ("Failed to move body into new message\n"); + + oom = TRUE; + goto failed; + } + + _dbus_assert (_dbus_string_get_length (&message->header) == header_len); + _dbus_assert (_dbus_string_get_length (&message->body) == body_len); + + /* Fill in caches (we checked the types of these fields + * earlier) + */ + message->reply_serial = get_uint_field (message, + DBUS_HEADER_FIELD_REPLY_SERIAL); + + message->client_serial = _dbus_demarshal_uint32 (&message->header, + message->byte_order, + CLIENT_SERIAL_OFFSET, + NULL); + if (message->client_serial == 0 || + (message->header_fields[DBUS_HEADER_FIELD_REPLY_SERIAL].value_offset >= 0 && message->reply_serial == 0)) + { + _dbus_verbose ("client_serial = %d reply_serial = %d, one of these no good\n", + message->client_serial, + message->reply_serial); + + loader->corrupted = TRUE; + goto failed; + } + + /* Fill in signature (FIXME should do this during validation, + * but I didn't want to spend time on it since we want to change + * the wire format to contain the signature anyway) + */ + { + DBusMessageIter iter; + + dbus_message_iter_init (message, &iter); + + do + { + int t; + + t = dbus_message_iter_get_arg_type (&iter); + if (t == DBUS_TYPE_INVALID) + break; + + if (!_dbus_string_append_byte (&message->signature, + t)) + { + _dbus_verbose ("failed to append type byte to signature\n"); + oom = TRUE; + goto failed; + } + + if (t == DBUS_TYPE_ARRAY) + { + DBusMessageIter child_iter; + int array_type = t; + + child_iter = iter; + + while (array_type == DBUS_TYPE_ARRAY) + { + DBusMessageIter parent_iter = child_iter; + dbus_message_iter_init_array_iterator (&parent_iter, + &child_iter, + &array_type); + + if (!_dbus_string_append_byte (&message->signature, + array_type)) + { + _dbus_verbose ("failed to append array type byte to signature\n"); + + oom = TRUE; + goto failed; + } + } + } + } + while (dbus_message_iter_next (&iter)); + } + + _dbus_verbose ("Loaded message %p\n", message); + + _dbus_assert (!oom); + _dbus_assert (!loader->corrupted); + + return TRUE; + + failed: + + /* Clean up */ + + if (message != NULL) + { + /* Put the data back so we can try again later if it was an OOM issue */ + if (_dbus_string_get_length (&message->body) > 0) + { + dbus_bool_t result; + + result = _dbus_string_copy_len (&message->body, 0, body_len, + &loader->data, 0); + + _dbus_assert (result); /* because DBusString never reallocs smaller */ + } + + if (_dbus_string_get_length (&message->header) > 0) + { + dbus_bool_t result; + + result = _dbus_string_copy_len (&message->header, 0, header_len, + &loader->data, 0); + + _dbus_assert (result); /* because DBusString never reallocs smaller */ + } + + /* does nothing if the message isn't in the list */ + _dbus_list_remove_last (&loader->messages, message); + + dbus_message_unref (message); + } + + + return !oom; +} + /** * Converts buffered data into messages. * @@ -5075,14 +5326,10 @@ _dbus_message_loader_return_buffer (DBusMessageLoader *loader, dbus_bool_t _dbus_message_loader_queue_messages (DBusMessageLoader *loader) { - if (loader->corrupted) - return TRUE; - - while (_dbus_string_get_length (&loader->data) >= 16) + while (!loader->corrupted && _dbus_string_get_length (&loader->data) >= 16) { - DBusMessage *message; const char *header_data; - int byte_order, message_type, header_len, body_len, header_padding; + int byte_order, message_type, header_len, body_len; dbus_uint32_t header_len_unsigned, body_len_unsigned; header_data = _dbus_string_get_const_data_len (&loader->data, 0, 16); @@ -5166,185 +5413,9 @@ _dbus_message_loader_queue_messages (DBusMessageLoader *loader) if (_dbus_string_get_length (&loader->data) >= (header_len + body_len)) { - HeaderField fields[DBUS_HEADER_FIELD_LAST + 1]; - int i; - int next_arg; - -#if 0 - _dbus_verbose_bytes_of_string (&loader->data, 0, header_len + body_len); -#endif - if (!decode_header_data (&loader->data, - header_len, byte_order, - message_type, - fields, &header_padding)) - { - _dbus_verbose ("Header was invalid\n"); - loader->corrupted = TRUE; - return TRUE; - } - - next_arg = header_len; - while (next_arg < (header_len + body_len)) - { - int type; - int prev = next_arg; - - if (!_dbus_marshal_validate_type (&loader->data, next_arg, - &type, &next_arg)) - { - _dbus_verbose ("invalid typecode at offset %d\n", prev); - loader->corrupted = TRUE; - return TRUE; - } - - if (!_dbus_marshal_validate_arg (&loader->data, - byte_order, - 0, - type, -1, - next_arg, - &next_arg)) - { - _dbus_verbose ("invalid type data at %d, next_arg\n", next_arg); - loader->corrupted = TRUE; - return TRUE; - } - - _dbus_assert (next_arg > prev); - } - - if (next_arg > (header_len + body_len)) - { - _dbus_verbose ("end of last arg at %d but message has len %d+%d=%d\n", - next_arg, header_len, body_len, - header_len + body_len); - loader->corrupted = TRUE; - return TRUE; - } - - message = dbus_message_new_empty_header (); - if (message == NULL) - { - _dbus_verbose ("Failed to allocate empty message\n"); - return FALSE; - } - - message->byte_order = byte_order; - message->header_padding = header_padding; - - /* Copy in the offsets we found */ - i = 0; - while (i <= DBUS_HEADER_FIELD_LAST) - { - message->header_fields[i] = fields[i]; - ++i; - } - - if (!_dbus_list_append (&loader->messages, message)) - { - _dbus_verbose ("Failed to append new message to loader queue\n"); - dbus_message_unref (message); - return FALSE; - } - - _dbus_assert (_dbus_string_get_length (&message->header) == 0); - _dbus_assert (_dbus_string_get_length (&message->body) == 0); - - _dbus_assert (_dbus_string_get_length (&loader->data) >= - (header_len + body_len)); - - if (!_dbus_string_move_len (&loader->data, 0, header_len, &message->header, 0)) - { - _dbus_verbose ("Failed to move header into new message\n"); - _dbus_list_remove_last (&loader->messages, message); - dbus_message_unref (message); - return FALSE; - } - - if (!_dbus_string_move_len (&loader->data, 0, body_len, &message->body, 0)) - { - dbus_bool_t result; - - _dbus_verbose ("Failed to move body into new message\n"); - - /* put the header back, we'll try again later */ - result = _dbus_string_copy_len (&message->header, 0, header_len, - &loader->data, 0); - _dbus_assert (result); /* because DBusString never reallocs smaller */ - - _dbus_list_remove_last (&loader->messages, message); - dbus_message_unref (message); - return FALSE; - } - - _dbus_assert (_dbus_string_get_length (&message->header) == header_len); - _dbus_assert (_dbus_string_get_length (&message->body) == body_len); - - /* Fill in caches (we checked the types of these fields - * earlier) - */ - message->reply_serial = get_uint_field (message, - DBUS_HEADER_FIELD_REPLY_SERIAL); - - message->client_serial = _dbus_demarshal_uint32 (&message->header, - message->byte_order, - CLIENT_SERIAL_OFFSET, - NULL); - - /* Fill in signature (FIXME should do this during validation, - * but I didn't want to spend time on it since we want to change - * the wire format to contain the signature anyway) - */ - { - DBusMessageIter iter; - - dbus_message_iter_init (message, &iter); - - do - { - int t; - - t = dbus_message_iter_get_arg_type (&iter); - if (t == DBUS_TYPE_INVALID) - break; - - if (!_dbus_string_append_byte (&message->signature, - t)) - { - _dbus_verbose ("failed to append type byte to signature\n"); - _dbus_list_remove_last (&loader->messages, message); - dbus_message_unref (message); - return FALSE; - } - - if (t == DBUS_TYPE_ARRAY) - { - DBusMessageIter child_iter; - int array_type = t; - - child_iter = iter; - - while (array_type == DBUS_TYPE_ARRAY) - { - DBusMessageIter parent_iter = child_iter; - dbus_message_iter_init_array_iterator (&parent_iter, - &child_iter, - &array_type); - - if (!_dbus_string_append_byte (&message->signature, - array_type)) - { - _dbus_verbose ("failed to append array type byte to signature\n"); - _dbus_list_remove_last (&loader->messages, message); - dbus_message_unref (message); - return FALSE; - } - } - } - } - while (dbus_message_iter_next (&iter)); - } - - _dbus_verbose ("Loaded message %p\n", message); + if (!load_one_message (loader, byte_order, message_type, + header_len, body_len)) + return FALSE; } else return TRUE; diff --git a/doc/TODO b/doc/TODO index 54794433..6257b098 100644 --- a/doc/TODO +++ b/doc/TODO @@ -117,11 +117,3 @@ we need to have a test for it in the test suite. - the max_replies_per_connection resource limit isn't implemented - - - the pending reply tracking isn't quite right. It currently simply - blocks any reply if one wasn't pending. Instead, it needs to - allow any reply if one was pending, and block otherwise. - Suggest changing the check_reply() call to just get a boolean - was_pending_reply value, pass that in to the policy - engine, and allow / elements to match based on - whether it was a pending reply. diff --git a/doc/dbus-specification.xml b/doc/dbus-specification.xml index 698e35a2..807769b7 100644 --- a/doc/dbus-specification.xml +++ b/doc/dbus-specification.xml @@ -644,6 +644,11 @@ reply is expected, so the caller will know the method was successfully processed. + + The METHOD_RETURN or ERROR reply message MUST have the REPLY_SERIAL + header field. If this field is missing, it should be treated as + a corrupt message. + If a METHOD_CALL message has the flag NO_REPLY_EXPECTED, then as an optimization the application receiving the method -- cgit