interfaces, object paths and activation

Mark McLoughlin mark@skynet.ie
Mon, 22 Sep 2003 21:14:34 +0100


--=-7qz5Qg4f2ryvuFGidLXy
Content-Type: text/plain
Content-Transfer-Encoding: 7bit

Hi,

On Sun, 2003-09-21 at 19:36, Havoc Pennington wrote:
> On Sun, 2003-09-21 at 14:22, Mark McLoughlin wrote:
> > > One question I had - why align the field name to 4 byte boundaries?
> > 
> > 	Yeah, good question. The old code did it this way too so I just made it
> > explicit. But sure, there isn't a good reason from a protocol point of
> > view to do it this way. I think, the only problem this will present is
> > making the field deletion logic cope with this ... but it might not be
> > such a bad idea to re-think that code anyway.
> > 
> > 	Will I go ahead and do that, then ?
> 
> The general rule has been to align everything naturally, so e.g. strings
> and bytes are aligned 1, 32-bit ints aligned 4, doubles aligned 8, etc.
> It makes sense to me to keep it that way, so a 1-byte field name would
> be aligned 1. If you want to code it up I'd be in favor, could save us
> some bytes and keep things consistent.

	So here's a patch to do that. As I thought the header
deletion/modification code was the nasty bit (previously we were saved
by the fact that the field name and all field types were 4-byte aligned)
but it hasn't come out too bad.

	Okay to commit ?

Cheers,
Mark.

--=-7qz5Qg4f2ryvuFGidLXy
Content-Disposition: attachment; filename=dbus-header-field-alignment.diff
Content-Type: text/x-patch; name=dbus-header-field-alignment.diff; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Index: ChangeLog
===================================================================
RCS file: /cvs/dbus/dbus/ChangeLog,v
retrieving revision 1.390.2.40
diff -u -p -r1.390.2.40 ChangeLog
--- ChangeLog	22 Sep 2003 05:45:59 -0000	1.390.2.40
+++ ChangeLog	22 Sep 2003 20:10:05 -0000
@@ -0,0 +1,62 @@
+2003-09-22  Mark McLoughlin  <mark@skynet.ie>
+
+	* doc/dbus-specification.sgml: don't require header fields
+	to be 4-byte aligned and specify that fields should be
+	distinguished from padding by the fact that zero is not
+	a valid field name.
+	
+	* doc/TODO: remove re-alignment item and add item to doc
+	the OBJECT_PATH type.
+	
+	* dbus/dbus-message.c:
+	(HeaderField): rename the original member to value_offset
+	and introduce a name_offset member to keep track of where
+	the field actually begins.
+	(adjust_field_offsets): remove.
+	(append_int_field), (append_uint_field),
+	(append_string_field): don't align the start of the header
+	field to a 4-byte boundary.
+	(get_next_field): impl finding the next marhsalled field
+	after a given field.
+	(re_align_field_recurse): impl re-aligning a number of
+	already marshalled fields.
+	(delete_field): impl deleting a field of any type and
+	re-aligning any following fields.
+	(delete_int_or_uint_field), (delete_string_field): remove.
+	(set_int_field), (set_uint_field): no need to re-check
+	that we have the correct type for the field.
+	(set_string_field): ditto and impl re-aligning any
+	following fields.
+	(decode_header_data): update to take into account that
+	the fields aren't 4-byte aligned any more and the new
+	way to distinguish padding from header fields. Also,
+	don't exit when there is too much header padding.
+	(process_test_subdir): print the directory.
+	(_dbus_message_test): add test to make sure a following
+	field is re-aligned correctly after field deletion.
+	
+	* dbus/dbus-string.[ch]:
+	(_dbus_string_insert_bytes): rename from insert_byte and
+	allow the insert of multiple bytes.
+	(_dbus_string_test): test inserting multiple bytes.
+
+	* dbus/dbus-marshal.c: (_dbus_marshal_set_string): add
+	warning note to docs about having to re-align any
+	marshalled values following the string.
+	
+	* dbus/dbus-message-builder.c:
+	(append_string_field), (_dbus_message_data_load):
+	don't align the header field.
+	
+	* dbus/dbus-auth.c: (process_test_subdir): print the
+	directory.
+	
+	* test/break-loader.c: (randomly_add_one_byte): upd. for
+	insert_byte change.
+	
+	* test/data/invalid-messages/bad-header-field-alignment.message:
+	new test case.
+	
+	* test/data/valid-messages/unknown-header-field.message: shove
+	a dict in the unknown field.
+
Index: dbus/dbus-auth.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-auth.c,v
retrieving revision 1.28.2.2
diff -u -p -r1.28.2.2 dbus-auth.c
--- dbus/dbus-auth.c	7 Sep 2003 23:04:51 -0000	1.28.2.2
+++ dbus/dbus-auth.c	22 Sep 2003 20:10:05 -0000
@@ -2380,7 +2380,7 @@ process_test_subdir (const DBusString   
       goto failed;
     }
 
-  printf ("Testing:\n");
+  printf ("Testing %s:\n", subdir);
   
  next:
   while (_dbus_directory_get_next_file (dir, &filename, &error))
Index: dbus/dbus-marshal.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-marshal.c,v
retrieving revision 1.41.2.7
diff -u -p -r1.41.2.7 dbus-marshal.c
--- dbus/dbus-marshal.c	7 Sep 2003 23:04:52 -0000	1.41.2.7
+++ dbus/dbus-marshal.c	22 Sep 2003 20:10:05 -0000
@@ -395,6 +395,10 @@ _dbus_marshal_set_uint64 (DBusString    
  * an existing string or the wrong length will be deleted
  * and replaced with the new string.
  *
+ * Note: no attempt is made by this function to re-align
+ * any data which has been already marshalled after this
+ * string. Use with caution.
+ *
  * @param str the string to write the marshalled string to
  * @param offset the byte offset where string should be written
  * @param byte_order the byte order to use
Index: dbus/dbus-message-builder.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-message-builder.c,v
retrieving revision 1.18.2.6
diff -u -p -r1.18.2.6 dbus-message-builder.c
--- dbus/dbus-message-builder.c	21 Sep 2003 18:43:20 -0000	1.18.2.6
+++ dbus/dbus-message-builder.c	22 Sep 2003 20:10:05 -0000
@@ -300,12 +300,6 @@ append_string_field (DBusString *dest,
 {
   int len;
   
-  if (!_dbus_string_align_length (dest, 4))
-    {
-      _dbus_warn ("could not align field name\n");
-      return FALSE;
-    }
-
   if (!_dbus_string_append_byte (dest, field))
     {
       _dbus_warn ("couldn't append field name byte\n");
@@ -710,11 +704,6 @@ _dbus_message_data_load (DBusString     
               goto parse_failed;
             }
 
-          if (unalign)
-            unalign = FALSE;
-          else
-            _dbus_string_align_length (dest, 4);
-
           if (!_dbus_string_append_byte (dest, field))
 	    {
               _dbus_warn ("could not append header field name byte\n");
Index: dbus/dbus-message.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-message.c,v
retrieving revision 1.102.2.11
diff -u -p -r1.102.2.11 dbus-message.c
--- dbus/dbus-message.c	21 Sep 2003 18:43:20 -0000	1.102.2.11
+++ dbus/dbus-message.c	22 Sep 2003 20:10:05 -0000
@@ -47,9 +47,8 @@
  */
 typedef struct
 {
-  int offset; /**< Offset to start of field (location of name of field
-               * for named fields)
-               */
+  int name_offset;  /**< Offset to name of field */
+  int value_offset; /**< Offset to value of field */
 } HeaderField;
 
 /** Offset to byte order from start of header */
@@ -183,26 +182,6 @@ append_header_padding (DBusMessage *mess
   return TRUE;
 }
 
-static void
-adjust_field_offsets (DBusMessage *message,
-                      int          offsets_after,
-                      int          delta)
-{
-  int i;
-
-  if (delta == 0)
-    return;
-  
-  i = 0;
-  while (i <= DBUS_HEADER_FIELD_LAST)
-    {
-      if (message->header_fields[i].offset > offsets_after)
-        message->header_fields[i].offset += delta;
-
-      ++i;
-    }
-}
-
 #ifdef DBUS_BUILD_TESTS
 /* tests-only until it's actually used */
 static dbus_int32_t
@@ -213,7 +192,7 @@ get_int_field (DBusMessage *message,
 
   _dbus_assert (field <= DBUS_HEADER_FIELD_LAST);
   
-  offset = message->header_fields[field].offset;
+  offset = message->header_fields[field].value_offset;
   
   if (offset < 0)
     return -1; /* useless if -1 is a valid value of course */
@@ -233,7 +212,7 @@ get_uint_field (DBusMessage *message,
   
   _dbus_assert (field <= DBUS_HEADER_FIELD_LAST);
   
-  offset = message->header_fields[field].offset;
+  offset = message->header_fields[field].value_offset;
   
   if (offset < 0)
     return -1; /* useless if -1 is a valid value of course */
@@ -252,7 +231,7 @@ get_string_field (DBusMessage *message,
   int offset;
   const char *data;
 
-  offset = message->header_fields[field].offset;
+  offset = message->header_fields[field].value_offset;
 
   _dbus_assert (field <= DBUS_HEADER_FIELD_LAST);
   
@@ -283,7 +262,7 @@ get_path_field_decomposed (DBusMessage  
 {
   int offset;
 
-  offset = message->header_fields[field].offset;
+  offset = message->header_fields[field].value_offset;
 
   _dbus_assert (field <= DBUS_HEADER_FIELD_LAST);
   
@@ -306,16 +285,12 @@ append_int_field (DBusMessage *message,
                   int          field,
                   int          value)
 {
-  int orig_len;
-
   _dbus_assert (!message->locked);
 
   clear_header_padding (message);
   
-  orig_len = _dbus_string_get_length (&message->header);
-  
-  if (!_dbus_string_align_length (&message->header, 4))
-    goto failed;  
+  message->header_fields[field].name_offset =
+    _dbus_string_get_length (&message->header);
   
   if (!_dbus_string_append_byte (&message->header, field))
     goto failed;
@@ -326,7 +301,7 @@ append_int_field (DBusMessage *message,
   if (!_dbus_string_align_length (&message->header, 4))
     goto failed;
   
-  message->header_fields[field].offset =
+  message->header_fields[field].value_offset =
     _dbus_string_get_length (&message->header);
   
   if (!_dbus_marshal_int32 (&message->header, message->byte_order,
@@ -339,8 +314,10 @@ append_int_field (DBusMessage *message,
   return TRUE;
   
  failed:
-  message->header_fields[field].offset = -1;
-  _dbus_string_set_length (&message->header, orig_len);
+  _dbus_string_set_length (&message->header,
+			   message->header_fields[field].name_offset);
+  message->header_fields[field].name_offset  = -1;
+  message->header_fields[field].value_offset = -1;
 
   /* this must succeed because it was allocated on function entry and
    * DBusString doesn't ever realloc smaller
@@ -356,16 +333,12 @@ append_uint_field (DBusMessage *message,
                    int          field,
 		   int          value)
 {
-  int orig_len;
-
   _dbus_assert (!message->locked);
 
   clear_header_padding (message);
   
-  orig_len = _dbus_string_get_length (&message->header);
-  
-  if (!_dbus_string_align_length (&message->header, 4))
-    goto failed;  
+  message->header_fields[field].name_offset =
+    _dbus_string_get_length (&message->header);
   
   if (!_dbus_string_append_byte (&message->header, field))
     goto failed;
@@ -376,7 +349,7 @@ append_uint_field (DBusMessage *message,
   if (!_dbus_string_align_length (&message->header, 4))
     goto failed;
   
-  message->header_fields[field].offset =
+  message->header_fields[field].value_offset =
     _dbus_string_get_length (&message->header);
   
   if (!_dbus_marshal_uint32 (&message->header, message->byte_order,
@@ -389,8 +362,10 @@ append_uint_field (DBusMessage *message,
   return TRUE;
   
  failed:
-  message->header_fields[field].offset = -1;
-  _dbus_string_set_length (&message->header, orig_len);
+  _dbus_string_set_length (&message->header,
+			   message->header_fields[field].name_offset);
+  message->header_fields[field].name_offset  = -1;
+  message->header_fields[field].value_offset = -1;
 
   /* this must succeed because it was allocated on function entry and
    * DBusString doesn't ever realloc smaller
@@ -406,16 +381,12 @@ append_string_field (DBusMessage *messag
                      int          type,
                      const char  *value)
 {
-  int orig_len;
-
   _dbus_assert (!message->locked);
 
   clear_header_padding (message);
   
-  orig_len = _dbus_string_get_length (&message->header);
-
-  if (!_dbus_string_align_length (&message->header, 4))
-    goto failed;
+  message->header_fields[field].name_offset =
+    _dbus_string_get_length (&message->header);
   
   if (!_dbus_string_append_byte (&message->header, field))
     goto failed;
@@ -426,7 +397,7 @@ append_string_field (DBusMessage *messag
   if (!_dbus_string_align_length (&message->header, 4))
     goto failed;
   
-  message->header_fields[field].offset =
+  message->header_fields[field].value_offset =
     _dbus_string_get_length (&message->header);
   
   if (!_dbus_marshal_string (&message->header, message->byte_order,
@@ -439,8 +410,10 @@ append_string_field (DBusMessage *messag
   return TRUE;
   
  failed:
-  message->header_fields[field].offset = -1;
-  _dbus_string_set_length (&message->header, orig_len);
+  _dbus_string_set_length (&message->header,
+			   message->header_fields[field].name_offset);
+  message->header_fields[field].name_offset  = -1;
+  message->header_fields[field].value_offset = -1;
 
   /* this must succeed because it was allocated on function entry and
    * DBusString doesn't ever realloc smaller
@@ -451,70 +424,137 @@ append_string_field (DBusMessage *messag
   return FALSE;
 }
 
-#ifdef DBUS_BUILD_TESTS
-/* This isn't used, but building it when tests are enabled just to
- * keep it compiling if we need it in future
- */
-static void
-delete_int_or_uint_field (DBusMessage *message,
-                          int          field)
+static int
+get_next_field (DBusMessage *message,
+		int          field)
 {
-  int offset = message->header_fields[field].offset;
+  int offset = message->header_fields[field].name_offset;
+  int closest;
+  int i;
+  int retval = DBUS_HEADER_FIELD_INVALID;
 
-  _dbus_assert (!message->locked);
-  
-  if (offset < 0)
-    return;  
+  i = 0;
+  closest = _DBUS_INT_MAX;
+  while (i < DBUS_HEADER_FIELD_LAST)
+    {
+      if (message->header_fields[i].name_offset > offset &&
+	  message->header_fields[i].name_offset < closest)
+	{
+	  closest = message->header_fields[i].name_offset;
+	  retval = i;
+	}
+      ++i;
+    }
 
-  clear_header_padding (message);
-  
-  /* The field typecode and name take up 4 bytes */
-  _dbus_string_delete (&message->header,
-                       offset - 4,
-                       8);
-
-  message->header_fields[field].offset = -1;
-  
-  adjust_field_offsets (message,
-                        offset - 4,
-                        - 8);
+  return retval;
+}
 
-  append_header_padding (message);
+static void
+re_align_field_recurse (DBusMessage *message,
+			int          field,
+			int          offset)
+{
+  int old_name_offset  = message->header_fields[field].name_offset;
+  int old_value_offset = message->header_fields[field].value_offset;
+  int prev_padding, padding, delta;
+  int type;
+  int next_field;
+  int pos = offset;
+
+  /* padding between the typecode byte and the value itself */
+  prev_padding = old_value_offset - old_name_offset + 2;
+
+  pos++;
+  type = _dbus_string_get_byte (&message->header, pos);
+
+  pos++;
+  switch (type)
+    {
+    case DBUS_TYPE_NIL:
+    case DBUS_TYPE_BYTE:
+    case DBUS_TYPE_BOOLEAN:
+      padding = 0;
+      break;
+    case DBUS_TYPE_INT32:
+    case DBUS_TYPE_UINT32:
+    case DBUS_TYPE_STRING:
+    case DBUS_TYPE_OBJECT_PATH:
+      padding = _DBUS_ALIGN_VALUE (pos, 4) - pos;
+      break;
+    case DBUS_TYPE_INT64:
+    case DBUS_TYPE_UINT64:
+    case DBUS_TYPE_DOUBLE:
+      padding = _DBUS_ALIGN_VALUE (pos, 8) - pos;
+      break;
+    case DBUS_TYPE_NAMED:
+    case DBUS_TYPE_ARRAY:
+    case DBUS_TYPE_DICT:
+      _dbus_assert_not_reached ("no defined header fields may contain a named, array or dict value");
+      break;
+    case DBUS_TYPE_INVALID:
+    default:
+      _dbus_assert_not_reached ("invalid type in marshalled header");
+      break;
+    }
+
+  delta = padding - prev_padding;
+  if (delta > 0)
+    _dbus_string_insert_bytes (&message->header, pos, delta, 0);
+  else if (delta < 0)
+    _dbus_string_delete (&message->header, pos, -delta);
+
+  pos += padding;
+
+  next_field = get_next_field (message, field);
+  if (next_field != DBUS_HEADER_FIELD_INVALID)
+    {
+      int next_offset = message->header_fields[next_field].name_offset;
+
+      _dbus_assert (next_offset > 0);
+
+      re_align_field_recurse (message, field,
+			      pos + (next_offset - old_value_offset));
+    }
+
+  message->header_fields[field].name_offset  = offset;
+  message->header_fields[field].value_offset = pos;
 }
-#endif
 
 static void
-delete_string_field (DBusMessage *message,
-                     int          field)
+delete_field (DBusMessage *message,
+	      int          field)
 {
-  int offset = message->header_fields[field].offset;
-  int len;
-  int delete_len;
-  
+  int offset = message->header_fields[field].name_offset;
+  int next_field;
+
   _dbus_assert (!message->locked);
   
   if (offset < 0)
-    return;
+    return;  
 
   clear_header_padding (message);
-  
-  get_string_field (message, field, &len);
-  
-  /* The field typecode and name take up 4 bytes, and the nul
-   * termination is 1 bytes, string length integer is 4 bytes
-   */
-  delete_len = 4 + 4 + 1 + len;
-  
-  _dbus_string_delete (&message->header,
-                       offset - 4,
-                       delete_len);
 
-  message->header_fields[field].offset = -1;
-  
-  adjust_field_offsets (message,
-                        offset - 4,
-                        - delete_len);
+  next_field = get_next_field (message, field);
+  if (next_field == DBUS_HEADER_FIELD_INVALID)
+    {
+      _dbus_string_set_length (&message->header, offset);
+    }
+  else
+    {
+      int next_offset = message->header_fields[next_field].name_offset;
 
+      _dbus_assert (next_offset > 0);
+
+      _dbus_string_delete (&message->header,
+			   offset,
+			   next_offset - offset);
+
+      re_align_field_recurse (message, next_field, offset);
+    }
+  
+  message->header_fields[field].name_offset  = -1;
+  message->header_fields[field].value_offset = -1;
+  
   append_header_padding (message);
 }
 
@@ -524,20 +564,14 @@ set_int_field (DBusMessage *message,
                int          field,
                int          value)
 {
-  int offset = message->header_fields[field].offset;
+  int offset = message->header_fields[field].value_offset;
 
   _dbus_assert (!message->locked);
   
   if (offset < 0)
     {
       /* need to append the field */
-
-      switch (field)
-        {
-        default:
-          _dbus_assert_not_reached ("appending an int field we don't support appending");
-          return FALSE;
-        }
+      return append_int_field (message, field, value);
     }
   else
     {
@@ -555,23 +589,14 @@ set_uint_field (DBusMessage  *message,
                 int           field,
                 dbus_uint32_t value)
 {
-  int offset = message->header_fields[field].offset;
+  int offset = message->header_fields[field].value_offset;
 
   _dbus_assert (!message->locked);
   
   if (offset < 0)
     {
       /* need to append the field */
-
-      switch (field)
-        {
-        case DBUS_HEADER_FIELD_REPLY_SERIAL:
-          return append_uint_field (message, field, value);
-
-        default:
-          _dbus_assert_not_reached ("appending a uint field we don't support appending");
-          return FALSE;
-        }
+      return append_uint_field (message, field, value);
     }
   else
     {
@@ -589,7 +614,7 @@ set_string_field (DBusMessage *message,
                   int          type,
                   const char  *value)
 {
-  int offset = message->header_fields[field].offset;
+  int offset = message->header_fields[field].value_offset;
 
   _dbus_assert (!message->locked);
   _dbus_assert (value != NULL);
@@ -597,32 +622,15 @@ set_string_field (DBusMessage *message,
   if (offset < 0)
     {      
       /* need to append the field */
-
-      switch (field)
-        {
-        case DBUS_HEADER_FIELD_PATH:
-        case DBUS_HEADER_FIELD_SENDER_SERVICE:
-        case DBUS_HEADER_FIELD_INTERFACE:
-        case DBUS_HEADER_FIELD_MEMBER:
-        case DBUS_HEADER_FIELD_ERROR_NAME:
-        case DBUS_HEADER_FIELD_SERVICE:
-          return append_string_field (message, field, type, value);
-
-        default:
-          _dbus_assert_not_reached ("appending a string field we don't support appending");
-          return FALSE;
-        }
+      return append_string_field (message, field, type, value);
     }
   else
     {
       DBusString v;
-      int old_len;
-      int new_len;
       int len;
+      int next_field;
       
       clear_header_padding (message);
-      
-      old_len = _dbus_string_get_length (&message->header);
 
       len = strlen (value);
       
@@ -633,12 +641,14 @@ set_string_field (DBusMessage *message,
                                      offset, &v,
 				     len))
         goto failed;
-      
-      new_len = _dbus_string_get_length (&message->header);
 
-      adjust_field_offsets (message,
-                            offset,
-                            new_len - old_len);
+      next_field = get_next_field (message, field);
+      if (next_field != DBUS_HEADER_FIELD_INVALID)
+	{
+	  re_align_field_recurse (message,
+				  next_field,
+				  offset + 4 + len + 1);
+	}
 
       if (!append_header_padding (message))
 	goto failed;
@@ -983,7 +993,8 @@ dbus_message_new_empty_header (void)
   i = 0;
   while (i <= DBUS_HEADER_FIELD_LAST)
     {
-      message->header_fields[i].offset = -1;
+      message->header_fields[i].name_offset  = -1;
+      message->header_fields[i].value_offset = -1;
       ++i;
     }
   
@@ -1281,7 +1292,7 @@ dbus_message_copy (const DBusMessage *me
 
   for (i = 0; i <= DBUS_HEADER_FIELD_LAST; i++)
     {
-      retval->header_fields[i].offset = message->header_fields[i].offset;
+      retval->header_fields[i] = message->header_fields[i];
     }
   
   return retval;
@@ -1390,7 +1401,7 @@ dbus_message_set_path (DBusMessage   *me
   
   if (object_path == NULL)
     {
-      delete_string_field (message, DBUS_HEADER_FIELD_PATH);
+      delete_field (message, DBUS_HEADER_FIELD_PATH);
       return TRUE;
     }
   else
@@ -1464,7 +1475,7 @@ dbus_message_set_interface (DBusMessage 
   
   if (interface == NULL)
     {
-      delete_string_field (message, DBUS_HEADER_FIELD_INTERFACE);
+      delete_field (message, DBUS_HEADER_FIELD_INTERFACE);
       return TRUE;
     }
   else
@@ -1512,7 +1523,7 @@ dbus_message_set_member (DBusMessage  *m
   
   if (member == NULL)
     {
-      delete_string_field (message, DBUS_HEADER_FIELD_MEMBER);
+      delete_field (message, DBUS_HEADER_FIELD_MEMBER);
       return TRUE;
     }
   else
@@ -1559,8 +1570,7 @@ dbus_message_set_error_name (DBusMessage
   
   if (error_name == NULL)
     {
-      delete_string_field (message,
-			   DBUS_HEADER_FIELD_ERROR_NAME);
+      delete_field (message, DBUS_HEADER_FIELD_ERROR_NAME);
       return TRUE;
     }
   else
@@ -1604,7 +1614,7 @@ dbus_message_set_destination (DBusMessag
   
   if (destination == NULL)
     {
-      delete_string_field (message, DBUS_HEADER_FIELD_SERVICE);
+      delete_field (message, DBUS_HEADER_FIELD_SERVICE);
       return TRUE;
     }
   else
@@ -4081,8 +4091,7 @@ dbus_message_set_sender (DBusMessage  *m
 
   if (sender == NULL)
     {
-      delete_string_field (message,
-			   DBUS_HEADER_FIELD_SENDER_SERVICE);
+      delete_field (message, DBUS_HEADER_FIELD_SENDER_SERVICE);
       return TRUE;
     }
   else
@@ -4550,7 +4559,7 @@ decode_string_field (const DBusString   
   _dbus_assert (header_field != NULL);
   _dbus_assert (field_data != NULL);
   
-  if (header_field->offset >= 0)
+  if (header_field->name_offset >= 0)
     {
       _dbus_verbose ("%s field provided twice\n",
 		     _dbus_header_field_to_string (field));
@@ -4575,12 +4584,13 @@ decode_string_field (const DBusString   
   _dbus_string_init_const (field_data,
                            _dbus_string_get_const_data (data) + string_data_pos);
 
-  header_field->offset = _DBUS_ALIGN_VALUE (pos, 4);
+  header_field->name_offset  = pos;
+  header_field->value_offset = _DBUS_ALIGN_VALUE (pos, 4);
   
 #if 0
   _dbus_verbose ("Found field %s at offset %d\n",
                  _dbus_header_field_to_string (field),
-		 header_field->offset);
+		 header_field->value_offset);
 #endif
 
   return TRUE;
@@ -4609,29 +4619,18 @@ decode_header_data (const DBusString   *
   i = 0;
   while (i <= DBUS_HEADER_FIELD_LAST)
     {
-      fields[i].offset = -1;
+      fields[i].name_offset  = -1;
+      fields[i].value_offset = -1;
       ++i;
     }
   
-  /* Now handle the named fields. A real named field is at least 1
-   * byte for the name, plus a type code (1 byte) plus padding, plus
-   * the field value. So if we have less than 8 bytes left, it must
-   * be alignment padding, not a field. While >= 8 bytes can't be
-   * entirely alignment padding.
-   */  
   pos = 16;
-  while ((pos + 7) < header_len)
+  while (pos < header_len)
     {
-      pos = _DBUS_ALIGN_VALUE (pos, 4);
-      
-      if ((pos + 1) > header_len)
-        {
-          _dbus_verbose ("not enough space remains in header for header field value\n");
-          return FALSE;
-        }
-      
       field = _dbus_string_get_byte (data, pos);
-      pos += 1;
+      if (field == DBUS_HEADER_FIELD_INVALID)
+	break; /* Must be padding */
+      pos++;
 
       if (!_dbus_marshal_validate_type (data, pos, &type, &pos))
 	{
@@ -4737,7 +4736,7 @@ decode_header_data (const DBusString   *
            * type.
            */
           
-          if (fields[field].offset >= 0)
+          if (fields[field].name_offset >= 0)
             {
               _dbus_verbose ("path field provided twice\n");
               return FALSE;
@@ -4748,15 +4747,16 @@ decode_header_data (const DBusString   *
               return FALSE;
             }
 
-          fields[field].offset = _DBUS_ALIGN_VALUE (pos, 4);
+          fields[field].name_offset  = pos;
+          fields[field].value_offset = _DBUS_ALIGN_VALUE (pos, 4);
 
           /* No forging signals from the local path */
           {
             const char *s;
             s = _dbus_string_get_const_data_len (data,
-                                                 fields[field].offset,
+                                                 fields[field].value_offset,
                                                  _dbus_string_get_length (data) -
-                                                 fields[field].offset);
+                                                 fields[field].value_offset);
             if (strcmp (s, DBUS_PATH_ORG_FREEDESKTOP_LOCAL) == 0)
               {
                 _dbus_verbose ("Message is on the local path\n");
@@ -4765,11 +4765,11 @@ decode_header_data (const DBusString   *
           }
           
           _dbus_verbose ("Found path at offset %d\n",
-                         fields[field].offset);
+                         fields[field].value_offset);
 	  break;
           
 	case DBUS_HEADER_FIELD_REPLY_SERIAL:
-          if (fields[field].offset >= 0)
+          if (fields[field].name_offset >= 0)
             {
               _dbus_verbose ("reply field provided twice\n");
               return FALSE;
@@ -4781,10 +4781,11 @@ decode_header_data (const DBusString   *
               return FALSE;
             }
           
-          fields[field].offset = _DBUS_ALIGN_VALUE (pos, 4);
+          fields[field].name_offset  = pos;
+          fields[field].value_offset = _DBUS_ALIGN_VALUE (pos, 4);
 
           _dbus_verbose ("Found reply serial at offset %d\n",
-                         fields[field].offset);
+                         fields[field].value_offset);
 	  break;
 
         default:
@@ -4798,7 +4799,11 @@ decode_header_data (const DBusString   *
   if (pos < header_len)
     {
       /* Alignment padding, verify that it's nul */
-      _dbus_assert ((header_len - pos) < 8);
+      if ((header_len - pos) >= 8)
+	{
+	  _dbus_verbose ("too much header alignment padding\n");
+	  return FALSE;
+	}
 
       if (!_dbus_string_validate_nul (data,
                                       pos, (header_len - pos)))
@@ -4813,25 +4818,25 @@ decode_header_data (const DBusString   *
     {
     case DBUS_MESSAGE_TYPE_SIGNAL:
     case DBUS_MESSAGE_TYPE_METHOD_CALL:
-      if (fields[DBUS_HEADER_FIELD_PATH].offset < 0)
+      if (fields[DBUS_HEADER_FIELD_PATH].value_offset < 0)
         {
           _dbus_verbose ("No path field provided\n");
           return FALSE;
         }
       /* FIXME make this optional, at least for method calls */
-      if (fields[DBUS_HEADER_FIELD_INTERFACE].offset < 0)
+      if (fields[DBUS_HEADER_FIELD_INTERFACE].value_offset < 0)
         {
           _dbus_verbose ("No interface field provided\n");
           return FALSE;
         }
-      if (fields[DBUS_HEADER_FIELD_MEMBER].offset < 0)
+      if (fields[DBUS_HEADER_FIELD_MEMBER].value_offset < 0)
         {
           _dbus_verbose ("No member field provided\n");
           return FALSE;
         }
       break;
     case DBUS_MESSAGE_TYPE_ERROR:
-      if (fields[DBUS_HEADER_FIELD_ERROR_NAME].offset < 0)
+      if (fields[DBUS_HEADER_FIELD_ERROR_NAME].value_offset < 0)
         {
           _dbus_verbose ("No error-name field provided\n");
           return FALSE;
@@ -6069,7 +6074,7 @@ process_test_subdir (const DBusString   
       goto failed;
     }
 
-  printf ("Testing:\n");
+  printf ("Testing %s:\n", subdir);
   
  next:
   while (_dbus_directory_get_next_file (dir, &filename, &error))
@@ -6432,11 +6437,14 @@ _dbus_message_test (const char *test_dat
   _dbus_assert (dbus_message_is_method_call (message, "Foo.TestInterface",
                                              "TestMethod"));
   _dbus_message_set_serial (message, 1234);
-  dbus_message_set_sender (message, "org.foo.bar");
-  _dbus_assert (dbus_message_has_sender (message, "org.foo.bar"));
+  /* string length including nul byte not a multiple of 4 */
+  dbus_message_set_sender (message, "org.foo.bar1");
+  _dbus_assert (dbus_message_has_sender (message, "org.foo.bar1"));
+  dbus_message_set_reply_serial (message, 5678);
   dbus_message_set_sender (message, NULL);
-  _dbus_assert (!dbus_message_has_sender (message, "org.foo.bar"));
+  _dbus_assert (!dbus_message_has_sender (message, "org.foo.bar1"));
   _dbus_assert (dbus_message_get_serial (message) == 1234);
+  _dbus_assert (dbus_message_get_reply_serial (message) == 5678);
   _dbus_assert (dbus_message_has_destination (message, "org.freedesktop.DBus.TestService"));
 
   _dbus_assert (dbus_message_get_no_reply (message) == FALSE);
Index: dbus/dbus-string.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-string.c,v
retrieving revision 1.41.2.7
diff -u -p -r1.41.2.7 dbus-string.c
--- dbus/dbus-string.c	7 Sep 2003 23:04:52 -0000	1.41.2.7
+++ dbus/dbus-string.c	22 Sep 2003 20:10:05 -0000
@@ -562,26 +562,30 @@ _dbus_string_get_byte (const DBusString 
 }
 
 /**
- * Inserts the given byte at the given position.
+ * Inserts a number of bytes of a given value at the
+ * given position.
  *
  * @param str the string
  * @param i the position
+ * @param n_bytes number of bytes
  * @param byte the value to insert
  * @returns #TRUE on success
  */
 dbus_bool_t
-_dbus_string_insert_byte (DBusString   *str,
-                          int           i,
-                          unsigned char byte)
+_dbus_string_insert_bytes (DBusString   *str,
+			   int           i,
+			   int           n_bytes,
+			   unsigned char byte)
 {
   DBUS_STRING_PREAMBLE (str);
   _dbus_assert (i <= real->len);
   _dbus_assert (i >= 0);
+  _dbus_assert (n_bytes > 0);
   
-  if (!open_gap (1, real, i))
+  if (!open_gap (n_bytes, real, i))
     return FALSE;
   
-  real->str[i] = byte;
+  memset (real->str + i, byte, n_bytes);
 
   return TRUE;
 }
@@ -3572,23 +3576,26 @@ _dbus_string_test (void)
   _dbus_string_set_byte (&str, 1, 'q');
   _dbus_assert (_dbus_string_get_byte (&str, 1) == 'q');
 
-  if (!_dbus_string_insert_byte (&str, 0, 255))
+  if (!_dbus_string_insert_bytes (&str, 0, 1, 255))
     _dbus_assert_not_reached ("can't insert byte");
 
-  if (!_dbus_string_insert_byte (&str, 2, 'Z'))
+  if (!_dbus_string_insert_bytes (&str, 2, 4, 'Z'))
     _dbus_assert_not_reached ("can't insert byte");
 
-  if (!_dbus_string_insert_byte (&str, _dbus_string_get_length (&str), 'W'))
+  if (!_dbus_string_insert_bytes (&str, _dbus_string_get_length (&str), 1, 'W'))
     _dbus_assert_not_reached ("can't insert byte");
   
   _dbus_assert (_dbus_string_get_byte (&str, 0) == 255);
   _dbus_assert (_dbus_string_get_byte (&str, 1) == 'H');
   _dbus_assert (_dbus_string_get_byte (&str, 2) == 'Z');
-  _dbus_assert (_dbus_string_get_byte (&str, 3) == 'q');
-  _dbus_assert (_dbus_string_get_byte (&str, 4) == 'l');
-  _dbus_assert (_dbus_string_get_byte (&str, 5) == 'l');
-  _dbus_assert (_dbus_string_get_byte (&str, 6) == 'o');
-  _dbus_assert (_dbus_string_get_byte (&str, 7) == 'W');
+  _dbus_assert (_dbus_string_get_byte (&str, 3) == 'Z');
+  _dbus_assert (_dbus_string_get_byte (&str, 4) == 'Z');
+  _dbus_assert (_dbus_string_get_byte (&str, 5) == 'Z');
+  _dbus_assert (_dbus_string_get_byte (&str, 6) == 'q');
+  _dbus_assert (_dbus_string_get_byte (&str, 7) == 'l');
+  _dbus_assert (_dbus_string_get_byte (&str, 8) == 'l');
+  _dbus_assert (_dbus_string_get_byte (&str, 9) == 'o');
+  _dbus_assert (_dbus_string_get_byte (&str, 10) == 'W');
 
   _dbus_string_free (&str);
   
Index: dbus/dbus-string.h
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-string.h,v
retrieving revision 1.20.2.4
diff -u -p -r1.20.2.4 dbus-string.h
--- dbus/dbus-string.h	7 Sep 2003 23:04:52 -0000	1.20.2.4
+++ dbus/dbus-string.h	22 Sep 2003 20:10:05 -0000
@@ -72,8 +72,9 @@ void          _dbus_string_set_byte     
                                                   unsigned char      byte);
 unsigned char _dbus_string_get_byte              (const DBusString  *str,
                                                   int                start);
-dbus_bool_t   _dbus_string_insert_byte           (DBusString        *str,
+dbus_bool_t   _dbus_string_insert_bytes          (DBusString        *str,
                                                   int                i,
+						  int                n_bytes,
                                                   unsigned char      byte);
 dbus_bool_t   _dbus_string_steal_data            (DBusString        *str,
                                                   char             **data_return);
Index: doc/TODO
===================================================================
RCS file: /cvs/dbus/dbus/doc/TODO,v
retrieving revision 1.22.2.8
diff -u -p -r1.22.2.8 TODO
--- doc/TODO	22 Sep 2003 03:11:12 -0000	1.22.2.8
+++ doc/TODO	22 Sep 2003 20:10:05 -0000
@@ -90,8 +90,6 @@
  - document the auth protocol as a set of states and transitions, and
    then reimplement it in those terms
 
- - Header fields names are required to be aligned on a 4 byte boundary
-   at the moment. No alignment should be neccessary.
- 
  - dbus_gproxy or dbus_g_proxy?
 
+ - The OBJECT_PATH type is not documented in the spec.
Index: doc/dbus-specification.sgml
===================================================================
RCS file: /cvs/dbus/dbus/doc/dbus-specification.sgml,v
retrieving revision 1.39.2.7
diff -u -p -r1.39.2.7 dbus-specification.sgml
--- doc/dbus-specification.sgml	21 Sep 2003 18:43:20 -0000	1.39.2.7
+++ doc/dbus-specification.sgml	22 Sep 2003 20:10:05 -0000
@@ -265,11 +265,10 @@
       </para>
 
       <para>
-        Header fields MUST be aligned to a 4-byte boundary. Header field
-        names MUST consist of a single byte, possible values of which are
-        defined below. Following the name, the field MUST have a type code
-        represented as a single unsigned byte, and then a properly-aligned
-        value of that type.  See <xref
+        Header field names MUST consist of a single byte, possible values
+        of which are defined below. Following the name, the field MUST have
+        a type code represented as a single unsigned byte, and then a
+        properly-aligned value of that type.  See <xref
         linkend="message-protocol-arguments"> for a description of how each
         type is encoded. If an implementation sees a header field name that
         it does not understand, it MUST ignore that field.
@@ -358,10 +357,9 @@
         buffer while keeping data types aligned, the total length of the header
         must be a multiple of 8 bytes.  To achieve this, the header MUST be padded
         with nul bytes to align its total length on an 8-byte boundary. 
-        The minimum number of padding bytes MUST be used. Because all possible 
-        named fields use at least 8 bytes, implementations can distinguish 
-        padding (which must be less than 8 bytes) from additional named fields
-        (which must be at least 8 bytes).
+        The minimum number of padding bytes MUST be used. Because zero is an
+        invalid field name, implementations can distinguish padding (which must be
+        zero initialized) from additional named fields.
       </para>
     </sect2>
 
Index: test/break-loader.c
===================================================================
RCS file: /cvs/dbus/dbus/test/break-loader.c,v
retrieving revision 1.7
diff -u -p -r1.7 break-loader.c
--- test/break-loader.c	22 Apr 2003 19:34:33 -0000	1.7
+++ test/break-loader.c	22 Sep 2003 20:10:05 -0000
@@ -287,8 +287,8 @@ randomly_add_one_byte (const DBusString 
 
   i = random_int_in_range (0, _dbus_string_get_length (mutated));
 
-  _dbus_string_insert_byte (mutated, i,
-                            random_int_in_range (0, 256));
+  _dbus_string_insert_bytes (mutated, i, 1,
+			     random_int_in_range (0, 256));
 }
 
 static void
Index: test/data/invalid-messages/bad-header-field-alignment.message
===================================================================
RCS file: test/data/invalid-messages/bad-header-field-alignment.message
diff -N test/data/invalid-messages/bad-header-field-alignment.message
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ test/data/invalid-messages/bad-header-field-alignment.message	22 Sep 2003 20:10:05 -0000
@@ -0,0 +1,34 @@
+## last field incorrectly aligned to 4 bytes
+
+## VALID_HEADER includes a LENGTH Header and LENGTH Body
+VALID_HEADER method_call
+
+HEADER_FIELD INTERFACE
+TYPE STRING
+STRING 'org.freedesktop.Foo'
+
+HEADER_FIELD MEMBER
+TYPE STRING
+STRING 'Bar'
+
+HEADER_FIELD UNKNOWN
+TYPE STRING
+STRING 'a'
+
+ALIGN 4
+
+HEADER_FIELD UNKNOWN
+TYPE ARRAY
+TYPE BYTE
+ALIGN 4
+LENGTH ThisByteArray
+START_LENGTH ThisByteArray
+BYTE 1
+BYTE 2
+END_LENGTH ThisByteArray
+
+
+ALIGN 8
+END_LENGTH Header
+START_LENGTH Body
+END_LENGTH Body
Index: test/data/valid-messages/unknown-header-field.message
===================================================================
RCS file: /cvs/dbus/dbus/test/data/valid-messages/unknown-header-field.message,v
retrieving revision 1.1.2.3
diff -u -p -r1.1.2.3 unknown-header-field.message
--- test/data/valid-messages/unknown-header-field.message	21 Sep 2003 18:43:20 -0000	1.1.2.3
+++ test/data/valid-messages/unknown-header-field.message	22 Sep 2003 20:10:05 -0000
@@ -3,9 +3,16 @@
 ## VALID_HEADER includes a LENGTH Header and LENGTH Body
 VALID_HEADER method_call
 REQUIRED_FIELDS
+
 HEADER_FIELD UNKNOWN
+TYPE DICT
+LENGTH Dict
+START_LENGTH Dict
+STRING 'int32'
 TYPE INT32
-INT32 0xfeeb
+INT32 0x12345678
+END_LENGTH Dict
+
 ALIGN 8
 END_LENGTH Header
 START_LENGTH Body

--=-7qz5Qg4f2ryvuFGidLXy--