2003-09-25 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.

12 files changed, 441 insertions, 244 deletions

diff --git a/ChangeLog b/ChangeLog
index 36abf234..58702c2e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,65 @@
+2003-09-25  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.
+
 2003-09-25  Seth Nickell  <seth@gnome.org>
 
 	* python/dbus.py:
diff --git a/dbus/dbus-auth.c b/dbus/dbus-auth.c
index 90c72fd5..cdfd3bb2 100644
--- a/dbus/dbus-auth.c
+++ b/dbus/dbus-auth.c
@@ -2380,7 +2380,7 @@ process_test_subdir (const DBusString          *test_base_dir,
       goto failed;
     }
 
-  printf ("Testing:\n");
+  printf ("Testing %s:\n", subdir);
   
  next:
   while (_dbus_directory_get_next_file (dir, &filename, &error))
diff --git a/dbus/dbus-marshal.c b/dbus/dbus-marshal.c
index 3d6184e9..cb989891 100644
--- a/dbus/dbus-marshal.c
+++ b/dbus/dbus-marshal.c
@@ -395,6 +395,10 @@ _dbus_marshal_set_uint64 (DBusString          *str,
  * 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
diff --git a/dbus/dbus-message-builder.c b/dbus/dbus-message-builder.c
index 7e2dff0d..c9dc8ca5 100644
--- a/dbus/dbus-message-builder.c
+++ b/dbus/dbus-message-builder.c
@@ -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       *dest,
               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");
diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
index 824d85bf..fe56e011 100644
--- a/dbus/dbus-message.c
+++ b/dbus/dbus-message.c
@@ -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 *message)
   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  *message,
 {
   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 *message,
                      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 *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_string (&message->header, message->byte_order,
@@ -439,8 +410,10 @@ append_string_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
@@ -451,71 +424,205 @@ append_string_field (DBusMessage *message,
   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);
+  return retval;
+}
 
-  message->header_fields[field].offset = -1;
-  
-  adjust_field_offsets (message,
-                        offset - 4,
-                        - 8);
+static dbus_bool_t
+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)
+    {
+      if (!_dbus_string_insert_bytes (&message->header, pos, delta, 0))
+	return FALSE;
+    }
+  else if (delta < 0)
+    {
+      _dbus_string_delete (&message->header, pos, -delta);
+    }
+
+  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);
+
+      if (!re_align_field_recurse (message, field,
+				   pos + padding + (next_offset - old_value_offset)))
+	goto failed;
+    }
+  else
+    {
+      if (!append_header_padding (message))
+	goto failed;
+    }
+
+  message->header_fields[field].name_offset  = offset;
+  message->header_fields[field].value_offset = pos + padding;
+
+  return TRUE;
+
+ failed:
+  if (delta > 0)
+    {
+      _dbus_string_delete (&message->header, pos, delta);
+    }
+  else if (delta < 0)
+    {
+      /* this must succeed because it was allocated on function entry and
+       * DBusString doesn't ever realloc smaller
+       */
+    _dbus_string_insert_bytes (&message->header, pos, -delta, 0);
+    }
 
-  append_header_padding (message);
+  return FALSE;
 }
-#endif
 
-static void
-delete_string_field (DBusMessage *message,
-                     int          field)
+static dbus_bool_t
+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 FALSE;
 
   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);
 
-  append_header_padding (message);
+      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
+       */
+      if (!append_header_padding (message))
+	_dbus_assert_not_reached ("failed to reappend header padding");
+
+      return TRUE;
+    }
+  else
+    {
+      DBusString deleted;
+      int next_offset = message->header_fields[next_field].name_offset;
+
+      _dbus_assert (next_offset > 0);
+
+      if (!_dbus_string_init (&deleted))
+	goto failed;
+
+      if (!_dbus_string_move_len (&message->header,
+				  offset, next_offset - offset,
+				  &deleted, 0))
+	{
+	  _dbus_string_free (&deleted);
+	  goto failed;
+	}
+
+      /* appends the header padding */
+      if (!re_align_field_recurse (message, next_field, offset))
+	{
+	  /* this must succeed because it was allocated on function entry and
+	   * DBusString doesn't ever realloc smaller
+	   */
+	  if (!_dbus_string_copy (&deleted, 0, &message->header, offset))
+	    _dbus_assert_not_reached ("failed to revert to original field");
+
+	  _dbus_string_free (&deleted);
+	  goto failed;
+	}
+
+      _dbus_string_free (&deleted);
+      
+      message->header_fields[field].name_offset  = -1;
+      message->header_fields[field].value_offset = -1;
+
+      return TRUE;
+
+    failed:
+      /* this must succeed because it was allocated on function entry and
+       * DBusString doesn't ever realloc smaller
+       */
+      if (!append_header_padding (message))
+	_dbus_assert_not_reached ("failed to reappend header padding");
+
+      return FALSE;
+    }
 }
 
 #ifdef DBUS_BUILD_TESTS
@@ -524,20 +631,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 +656,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 +681,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,52 +689,59 @@ 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;
+      char *old_value;
+      int next_field;
+      int next_offset;
       int len;
       
       clear_header_padding (message);
-      
-      old_len = _dbus_string_get_length (&message->header);
+
+      old_value = _dbus_demarshal_string (&message->header,
+					  message->byte_order,
+					  offset,
+					  &next_offset);
+      if (!old_value)
+	goto failed;
 
       len = strlen (value);
-      
+
       _dbus_string_init_const_len (&v, value,
 				   len + 1); /* include nul */
       if (!_dbus_marshal_set_string (&message->header,
                                      message->byte_order,
-                                     offset, &v,
-				     len))
-        goto failed;
-      
-      new_len = _dbus_string_get_length (&message->header);
+                                     offset, &v, len))
+	{
+	  dbus_free (old_value);
+	  goto failed;
+	}
 
-      adjust_field_offsets (message,
-                            offset,
-                            new_len - old_len);
+      next_field = get_next_field (message, field);
+      if (next_field != DBUS_HEADER_FIELD_INVALID)
+	{
+	  /* re-appends the header padding */
+	  if (!re_align_field_recurse (message, next_field, next_offset))
+	    {
+	      len = strlen (old_value);
+
+	      _dbus_string_init_const_len (&v, old_value,
+					  len + 1); /* include nul */
+	      if (!_dbus_marshal_set_string (&message->header,
+					     message->byte_order,
+					     offset, &v, len))
+		_dbus_assert_not_reached ("failed to revert to original string");
+
+	      dbus_free (old_value);
+	      goto failed;
+	    }
+	}
+
+      dbus_free (old_value);
 
-      if (!append_header_padding (message))
-	goto failed;
-      
       return TRUE;
 
     failed:
@@ -983,7 +1082,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 +1381,7 @@ dbus_message_copy (const DBusMessage *message)
 
   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 +1490,7 @@ dbus_message_set_path (DBusMessage   *message,
   
   if (object_path == NULL)
     {
-      delete_string_field (message, DBUS_HEADER_FIELD_PATH);
+      delete_field (message, DBUS_HEADER_FIELD_PATH);
       return TRUE;
     }
   else
@@ -1464,7 +1564,7 @@ dbus_message_set_interface (DBusMessage  *message,
   
   if (interface == NULL)
     {
-      delete_string_field (message, DBUS_HEADER_FIELD_INTERFACE);
+      delete_field (message, DBUS_HEADER_FIELD_INTERFACE);
       return TRUE;
     }
   else
@@ -1512,7 +1612,7 @@ dbus_message_set_member (DBusMessage  *message,
   
   if (member == NULL)
     {
-      delete_string_field (message, DBUS_HEADER_FIELD_MEMBER);
+      delete_field (message, DBUS_HEADER_FIELD_MEMBER);
       return TRUE;
     }
   else
@@ -1559,8 +1659,7 @@ dbus_message_set_error_name (DBusMessage  *message,
   
   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 +1703,7 @@ dbus_message_set_destination (DBusMessage  *message,
   
   if (destination == NULL)
     {
-      delete_string_field (message, DBUS_HEADER_FIELD_SERVICE);
+      delete_field (message, DBUS_HEADER_FIELD_SERVICE);
       return TRUE;
     }
   else
@@ -4081,8 +4180,7 @@ dbus_message_set_sender (DBusMessage  *message,
 
   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 +4648,7 @@ decode_string_field (const DBusString   *data,
   _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 +4673,13 @@ decode_string_field (const DBusString   *data,
   _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 +4708,18 @@ decode_header_data (const DBusString   *data,
   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 +4825,7 @@ decode_header_data (const DBusString   *data,
            * type.
            */
           
-          if (fields[field].offset >= 0)
+          if (fields[field].name_offset >= 0)
             {
               _dbus_verbose ("path field provided twice\n");
               return FALSE;
@@ -4748,15 +4836,16 @@ decode_header_data (const DBusString   *data,
               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 +4854,11 @@ decode_header_data (const DBusString   *data,
           }
           
           _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 +4870,11 @@ decode_header_data (const DBusString   *data,
               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 +4888,11 @@ decode_header_data (const DBusString   *data,
   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 +4907,25 @@ decode_header_data (const DBusString   *data,
     {
     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, only 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 +6163,7 @@ process_test_subdir (const DBusString          *test_base_dir,
       goto failed;
     }
 
-  printf ("Testing:\n");
+  printf ("Testing %s:\n", subdir);
   
  next:
   while (_dbus_directory_get_next_file (dir, &filename, &error))
@@ -6432,11 +6526,14 @@ _dbus_message_test (const char *test_data_dir)
   _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);
diff --git a/dbus/dbus-string.c b/dbus/dbus-string.c
index 40363686..628cf861 100644
--- a/dbus/dbus-string.c
+++ b/dbus/dbus-string.c
@@ -562,26 +562,30 @@ _dbus_string_get_byte (const DBusString  *str,
 }
 
 /**
- * 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);
   
diff --git a/dbus/dbus-string.h b/dbus/dbus-string.h
index 70e83b33..0b7be8b0 100644
--- a/dbus/dbus-string.h
+++ b/dbus/dbus-string.h
@@ -72,8 +72,9 @@ void          _dbus_string_set_byte              (DBusString        *str,
                                                   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);
diff --git a/doc/TODO b/doc/TODO
index 1935ff9f..c70cc929 100644
--- a/doc/TODO
+++ b/doc/TODO
@@ -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.
diff --git a/doc/dbus-specification.sgml b/doc/dbus-specification.sgml
index a2dd1b13..c772b3e5 100644
--- a/doc/dbus-specification.sgml
+++ b/doc/dbus-specification.sgml
@@ -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>
 
diff --git a/test/break-loader.c b/test/break-loader.c
index ebe2606b..3771d7cc 100644
--- a/test/break-loader.c
+++ b/test/break-loader.c
@@ -287,8 +287,8 @@ randomly_add_one_byte (const DBusString *orig_data,
 
   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
diff --git a/test/data/invalid-messages/bad-header-field-alignment.message b/test/data/invalid-messages/bad-header-field-alignment.message
new file mode 100644
index 00000000..75776a37
--- /dev/null
+++ b/test/data/invalid-messages/bad-header-field-alignment.message
@@ -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
diff --git a/test/data/valid-messages/unknown-header-field.message b/test/data/valid-messages/unknown-header-field.message
index 973def68..ac7d624c 100644
--- a/test/data/valid-messages/unknown-header-field.message
+++ b/test/data/valid-messages/unknown-header-field.message
@@ -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