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

2 files changed, 263 insertions, 192 deletions

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;