2003-10-14 Havoc Pennington <hp@redhat.com>

	* 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 <allow requested_reply="true"/>

	* 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

13 files changed, 454 insertions, 256 deletions

diff --git a/ChangeLog b/ChangeLog
index 123bed04..91618fcb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,33 @@
 2003-10-14  Havoc Pennington  <hp@redhat.com>
 
+	* 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 <allow requested_reply="true"/>
+
+	* 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  <hp@redhat.com>
+
 	* bus/connection.c: implement pending reply tracking using
 	BusExpireList
 
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 <deny> attributes determine whether the deny "matches" a
+The <deny> 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 <deny> or <allow> 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 <allow>, 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 <deny>, 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 <allow>/<deny> 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
@@ -645,6 +645,11 @@
           was successfully processed.
         </para>
         <para>
+          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.
+        </para>
+        <para>
           If a METHOD_CALL message has the flag NO_REPLY_EXPECTED, 
           then as an optimization the application receiving the method 
           call may choose to omit the reply message (regardless of