2003-01-08 Havoc Pennington <hp@pobox.com>

	* dbus/dbus-string.c (_dbus_string_align_length): new function

	* dbus/dbus-test-main.c: move main() for test app here
	* dbus/dbus-test.c
	(dbus_internal_symbol_do_not_use_run_tests): we have to export a
	symbol to run tests, because dbus-test isn't in the main
	library

        Code review nitpicks.

	* dbus/dbus-message.c (dbus_message_write_header): add newlines
	for people with narrow emacs ;-). Assert client_serial was filled
	in. Assert message->name != NULL.
	(dbus_message_append_fields): have "first_field_type" arg separate
	from va list, needed for C++ binding that also uses varargs IIRC
	and helps with type safety
	(dbus_message_new): add @todo about using DBusString to store
	service/name internally
	(dbus_message_new): don't leak ->service and ->name on OOM later
	in the function
	(dbus_message_unref): free the service name
	(dbus_message_get_fields): same change to varargs
	i.e. first_field_type
	(_dbus_message_loader_return_buffer): assert that the message data
	is aligned (if not it's a bug in our code). Put in verbose griping
	about why we set corrupted = TRUE.
	(decode_header_data): add FIXME that char* is evil.  Was going to
	add FIXME about evil locale-specific string.h strncmp, but just
	switched to wacky string-as-uint32 optimization. Move check for
	"no room for field name" above get_const_data_len() to avoid
	assertion failure in get_const_data_len if we have trailing 2
	bytes or the like. Check for service and name fields being
	provided twice. Don't leak service/name on error. Require field
	names to be aligned to 4 bytes.

	* dbus/dbus-marshal.c: move byte swap stuff to header
	(_dbus_pack_int32): uscore-prefix
	(_dbus_unpack_int32): uscore-prefix
	(_dbus_unpack_uint32): export
	(_dbus_demarshal_string): add @todo complaining about use of
	memcpy()
	(_dbus_marshal_get_field_end_pos): add @todo about bad error
	handling allowing corrupt data to go unchecked

12 files changed, 400 insertions, 122 deletions

diff --git a/ChangeLog b/ChangeLog
index 6e88efcf..b62e8f7f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,49 @@
+2003-01-08  Havoc Pennington  <hp@pobox.com>
+
+	* dbus/dbus-string.c (_dbus_string_align_length): new function
+
+	* dbus/dbus-test-main.c: move main() for test app here
+	* dbus/dbus-test.c
+	(dbus_internal_symbol_do_not_use_run_tests): we have to export a
+	symbol to run tests, because dbus-test isn't in the main 
+	library
+
+        Code review nitpicks.
+	
+	* dbus/dbus-message.c (dbus_message_write_header): add newlines
+	for people with narrow emacs ;-). Assert client_serial was filled
+	in. Assert message->name != NULL.
+	(dbus_message_append_fields): have "first_field_type" arg separate
+	from va list, needed for C++ binding that also uses varargs IIRC
+	and helps with type safety
+	(dbus_message_new): add @todo about using DBusString to store
+	service/name internally
+	(dbus_message_new): don't leak ->service and ->name on OOM later
+	in the function
+	(dbus_message_unref): free the service name
+	(dbus_message_get_fields): same change to varargs
+	i.e. first_field_type
+	(_dbus_message_loader_return_buffer): assert that the message data
+	is aligned (if not it's a bug in our code). Put in verbose griping
+	about why we set corrupted = TRUE.
+	(decode_header_data): add FIXME that char* is evil.  Was going to
+	add FIXME about evil locale-specific string.h strncmp, but just
+	switched to wacky string-as-uint32 optimization. Move check for
+	"no room for field name" above get_const_data_len() to avoid
+	assertion failure in get_const_data_len if we have trailing 2
+	bytes or the like. Check for service and name fields being
+	provided twice. Don't leak service/name on error. Require field
+	names to be aligned to 4 bytes.
+
+	* dbus/dbus-marshal.c: move byte swap stuff to header
+	(_dbus_pack_int32): uscore-prefix
+	(_dbus_unpack_int32): uscore-prefix
+	(_dbus_unpack_uint32): export
+	(_dbus_demarshal_string): add @todo complaining about use of
+	memcpy()
+	(_dbus_marshal_get_field_end_pos): add @todo about bad error
+	handling allowing corrupt data to go unchecked
+
 2003-01-08  Havoc Pennington  <hp@redhat.com>
 
 	* dbus/dbus-transport-unix.c (unix_do_iteration): add read/write 
diff --git a/dbus/Makefile.am b/dbus/Makefile.am
index dfc1a561..3457534d 100644
--- a/dbus/Makefile.am
+++ b/dbus/Makefile.am
@@ -32,6 +32,8 @@ libdbus_1_la_SOURCES=				\
 	dbus-server-protected.h			\
 	dbus-server-unix.c			\
 	dbus-server-unix.h			\
+	dbus-test.c				\
+	dbus-test.h				\
 	dbus-threads.c				\
 	dbus-transport.c			\
 	dbus-transport.h			\
@@ -75,8 +77,7 @@ if DBUS_BUILD_TESTS
 noinst_PROGRAMS=dbus-test
 
 dbus_test_SOURCES=				\
-	dbus-test.c				\
-	dbus-test.h
+	dbus-test-main.c
 
 dbus_test_LDADD= $(DBUS_CLIENT_LIBS) libdbus-convenience.la libdbus-1.la
 
diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c
index add963e9..9b672ef6 100644
--- a/dbus/dbus-internals.c
+++ b/dbus/dbus-internals.c
@@ -22,6 +22,7 @@
  */
 #include "dbus-internals.h"
 #include "dbus-protocol.h"
+#include "dbus-test.h"
 #include <stdio.h>
 #include <stdarg.h>
 #include <string.h>
@@ -29,6 +30,7 @@
 #include <errno.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <stdlib.h>
 
 /**
  * @defgroup DBusInternals D-BUS internal implementation details
diff --git a/dbus/dbus-marshal.c b/dbus/dbus-marshal.c
index 14629362..a6055806 100644
--- a/dbus/dbus-marshal.c
+++ b/dbus/dbus-marshal.c
@@ -26,33 +26,6 @@
 
 #include <string.h>
 
-#define DBUS_UINT32_SWAP_LE_BE_CONSTANT(val)	((dbus_uint32_t) ( \
-    (((dbus_uint32_t) (val) & (dbus_uint32_t) 0x000000ffU) << 24) |  \
-    (((dbus_uint32_t) (val) & (dbus_uint32_t) 0x0000ff00U) <<  8) |  \
-    (((dbus_uint32_t) (val) & (dbus_uint32_t) 0x00ff0000U) >>  8) |  \
-    (((dbus_uint32_t) (val) & (dbus_uint32_t) 0xff000000U) >> 24)))
-
-#define DBUS_UINT32_SWAP_LE_BE(val) (DBUS_UINT32_SWAP_LE_BE_CONSTANT (val))
-
-#ifdef WORDS_BIGENDIAN
-#define DBUS_INT32_TO_BE(val)	((dbus_int32_t) (val))
-#define DBUS_UINT32_TO_BE(val)	((dbus_uint32_t) (val))
-#define DBUS_INT32_TO_LE(val)	((dbus_int32_t) DBUS_UINT32_SWAP_LE_BE (val))
-#define DBUS_UINT32_TO_LE(val)	(DBUS_UINT32_SWAP_LE_BE (val))
-#else
-#define DBUS_INT32_TO_LE(val)	((dbus_int32_t) (val))
-#define DBUS_UINT32_TO_LE(val)	((dbus_uint32_t) (val))
-#define DBUS_INT32_TO_BE(val)	((dbus_int32_t) DBUS_UINT32_SWAP_LE_BE (val))
-#define DBUS_UINT32_TO_BE(val)	(DBUS_UINT32_SWAP_LE_BE (val))
-#endif
-
-/* The transformation is symmetric, so the FROM just maps to the TO. */
-#define DBUS_INT32_FROM_LE(val)	 (DBUS_INT32_TO_LE (val))
-#define DBUS_UINT32_FROM_LE(val) (DBUS_UINT32_TO_LE (val))
-#define DBUS_INT32_FROM_BE(val)	 (DBUS_INT32_TO_BE (val))
-#define DBUS_UINT32_FROM_BE(val) (DBUS_UINT32_TO_BE (val))
-
-
 /* from ORBit */
 static void
 swap_bytes (unsigned char *data,
@@ -72,9 +45,27 @@ swap_bytes (unsigned char *data,
     }
 }
 
-static dbus_uint32_t
-unpack_uint32 (int                  byte_order,
-               const unsigned char *data)
+/**
+ * @defgroup DBusMarshal marshaling and unmarshaling
+ * @ingroup  DBusInternals
+ * @brief functions to marshal/unmarshal data from the wire
+ *
+ * Types and functions related to converting primitive data types from
+ * wire format to native machine format, and vice versa.
+ *
+ * @{
+ */
+
+/**
+ * Unpacks a 32 bit unsigned integer from a data pointer
+ *
+ * @param byte_order The byte order to use
+ * @param data the data pointer
+ * @returns the integer
+ */
+dbus_uint32_t
+_dbus_unpack_uint32 (int                  byte_order,
+                     const unsigned char *data)
 {
   _dbus_assert (_DBUS_ALIGN_ADDRESS (data, 4) == data);
   
@@ -85,15 +76,15 @@ unpack_uint32 (int                  byte_order,
 }             
 
 /**
- * Unpacks a 32 bit unsigned integer from a data pointer
+ * Unpacks a 32 bit signed integer from a data pointer
  *
  * @param byte_order The byte order to use
  * @param data the data pointer
  * @returns the integer
  */
 dbus_int32_t
-dbus_unpack_int32 (int                  byte_order,
-		   const unsigned char *data)
+_dbus_unpack_int32 (int                  byte_order,
+                    const unsigned char *data)
 {
   _dbus_assert (_DBUS_ALIGN_ADDRESS (data, 4) == data);
   
@@ -104,27 +95,36 @@ dbus_unpack_int32 (int                  byte_order,
 }
 
 /**
- * @defgroup DBusMarshal marshaling and unmarshaling
- * @ingroup  DBusInternals
- * @brief functions to marshal/unmarshal data from the wire
- *
- * Types and functions related to converting primitive data types from
- * wire format to native machine format, and vice versa.
+ * Packs a 32 bit unsigned integer into a data pointer.
  *
- * @{
+ * @param value the value
+ * @param byte_order the byte order to use
+ * @param data the data pointer
  */
+void
+_dbus_pack_uint32 (dbus_uint32_t   value,
+                   int             byte_order,
+                   unsigned char  *data)
+{
+  _dbus_assert (_DBUS_ALIGN_ADDRESS (data, 4) == data);
+  
+  if ((byte_order) == DBUS_LITTLE_ENDIAN)                  
+    *((dbus_uint32_t*)(data)) = DBUS_UINT32_TO_LE (value);       
+  else
+    *((dbus_uint32_t*)(data)) = DBUS_UINT32_TO_BE (value);
+}
 
 /**
- * Packs a 32 bit unsigned integer into a data pointer.
+ * Packs a 32 bit signed integer into a data pointer.
  *
  * @param value the value
  * @param byte_order the byte order to use
  * @param data the data pointer
  */
 void
-dbus_pack_int32 (dbus_int32_t   value,
-		 int            byte_order,
-		 unsigned char *data)
+_dbus_pack_int32 (dbus_int32_t   value,
+                  int            byte_order,
+                  unsigned char *data)
 {
   _dbus_assert (_DBUS_ALIGN_ADDRESS (data, 4) == data);
   
@@ -147,9 +147,11 @@ _dbus_marshal_double (DBusString *str,
 		      int         byte_order,
 		      double      value)
 {
+  _dbus_assert (sizeof (double) == 8);
+  
   if (!_dbus_string_set_length (str,
 				_DBUS_ALIGN_VALUE (_dbus_string_get_length (str),
-						  sizeof (double))))
+                                                   sizeof (double))))
     return FALSE;
   
   if (byte_order != DBUS_COMPILER_BYTE_ORDER)
@@ -173,7 +175,7 @@ _dbus_marshal_int32  (DBusString   *str,
 {
   if (!_dbus_string_set_length (str,
 				_DBUS_ALIGN_VALUE (_dbus_string_get_length (str),
-						  sizeof (dbus_int32_t))))
+                                                   sizeof (dbus_int32_t))))
     return FALSE;
   
   if (byte_order != DBUS_COMPILER_BYTE_ORDER)
@@ -197,7 +199,7 @@ _dbus_marshal_uint32 (DBusString    *str,
 {
   if (!_dbus_string_set_length (str,
 				_DBUS_ALIGN_VALUE (_dbus_string_get_length (str),
-						  sizeof (dbus_uint32_t))))
+                                                   sizeof (dbus_uint32_t))))
     return FALSE;
 
   if (byte_order != DBUS_COMPILER_BYTE_ORDER)
@@ -323,7 +325,7 @@ _dbus_demarshal_int32  (DBusString *str,
   if (new_pos)
     *new_pos = pos + sizeof (dbus_int32_t);
 
-  return dbus_unpack_int32 (byte_order, buffer);
+  return _dbus_unpack_int32 (byte_order, buffer);
 }
 
 /**
@@ -350,7 +352,7 @@ _dbus_demarshal_uint32  (DBusString *str,
   if (new_pos)
     *new_pos = pos + sizeof (dbus_uint32_t);
 
-  return unpack_uint32 (byte_order, buffer);
+  return _dbus_unpack_uint32 (byte_order, buffer);
 }
 
 /**
@@ -360,6 +362,9 @@ _dbus_demarshal_uint32  (DBusString *str,
  * that it's  valid UTF-8, and maybe "fix" the string
  * if it's broken?
  *
+ * @todo Should probably demarshal to a DBusString,
+ * having memcpy() in here is Evil(tm).
+ *
  * @param str the string containing the data
  * @param byte_order the byte order
  * @param pos the position in the string
@@ -434,6 +439,12 @@ _dbus_demarshal_byte_array (DBusString *str,
  * Returns the position right after the end position 
  * end position of a field
  *
+ * @todo warns on invalid type in a message, but
+ * probably the whole message needs to be dumped,
+ * or we might even drop the connection due
+ * to bad protocol. Needs better error handling.
+ * Possible security issue.
+ *
  * @param str a string
  * @param byte_order the byte order to use
  * @param pos the pos where the field starts
@@ -564,8 +575,8 @@ _dbus_verbose_bytes (const unsigned char *data,
         {
           if (i > 3)
             _dbus_verbose ("big: %d little: %d",
-                     unpack_uint32 (DBUS_BIG_ENDIAN, &data[i-4]),
-                     unpack_uint32 (DBUS_LITTLE_ENDIAN, &data[i-4]));
+                           _dbus_unpack_uint32 (DBUS_BIG_ENDIAN, &data[i-4]),
+                           _dbus_unpack_uint32 (DBUS_LITTLE_ENDIAN, &data[i-4]));
           
           _dbus_verbose ("\n");
         }
diff --git a/dbus/dbus-marshal.h b/dbus/dbus-marshal.h
index 4f1ba37e..1c4c0e40 100644
--- a/dbus/dbus-marshal.h
+++ b/dbus/dbus-marshal.h
@@ -39,11 +39,42 @@
 #define DBUS_COMPILER_BYTE_ORDER DBUS_LITTLE_ENDIAN
 #endif
 
-void         dbus_pack_int32   (dbus_int32_t         value,
-				int                  byte_order,
-				unsigned char       *data);
-dbus_int32_t dbus_unpack_int32 (int                  byte_order,
-				const unsigned char *data);
+#define DBUS_UINT32_SWAP_LE_BE_CONSTANT(val)	((dbus_uint32_t) ( \
+    (((dbus_uint32_t) (val) & (dbus_uint32_t) 0x000000ffU) << 24) |  \
+    (((dbus_uint32_t) (val) & (dbus_uint32_t) 0x0000ff00U) <<  8) |  \
+    (((dbus_uint32_t) (val) & (dbus_uint32_t) 0x00ff0000U) >>  8) |  \
+    (((dbus_uint32_t) (val) & (dbus_uint32_t) 0xff000000U) >> 24)))
+
+#define DBUS_UINT32_SWAP_LE_BE(val) (DBUS_UINT32_SWAP_LE_BE_CONSTANT (val))
+
+#ifdef WORDS_BIGENDIAN
+#define DBUS_INT32_TO_BE(val)	((dbus_int32_t) (val))
+#define DBUS_UINT32_TO_BE(val)	((dbus_uint32_t) (val))
+#define DBUS_INT32_TO_LE(val)	((dbus_int32_t) DBUS_UINT32_SWAP_LE_BE (val))
+#define DBUS_UINT32_TO_LE(val)	(DBUS_UINT32_SWAP_LE_BE (val))
+#else
+#define DBUS_INT32_TO_LE(val)	((dbus_int32_t) (val))
+#define DBUS_UINT32_TO_LE(val)	((dbus_uint32_t) (val))
+#define DBUS_INT32_TO_BE(val)	((dbus_int32_t) DBUS_UINT32_SWAP_LE_BE (val))
+#define DBUS_UINT32_TO_BE(val)	(DBUS_UINT32_SWAP_LE_BE (val))
+#endif
+
+/* The transformation is symmetric, so the FROM just maps to the TO. */
+#define DBUS_INT32_FROM_LE(val)	 (DBUS_INT32_TO_LE (val))
+#define DBUS_UINT32_FROM_LE(val) (DBUS_UINT32_TO_LE (val))
+#define DBUS_INT32_FROM_BE(val)	 (DBUS_INT32_TO_BE (val))
+#define DBUS_UINT32_FROM_BE(val) (DBUS_UINT32_TO_BE (val))
+
+void          _dbus_pack_int32    (dbus_int32_t         value,
+                                   int                  byte_order,
+                                   unsigned char       *data);
+dbus_int32_t  _dbus_unpack_int32  (int                  byte_order,
+                                   const unsigned char *data);
+void          _dbus_pack_uint32   (dbus_uint32_t        value,
+                                   int                  byte_order,
+                                   unsigned char       *data);
+dbus_uint32_t _dbus_unpack_uint32 (int                  byte_order,
+                                   const unsigned char *data);
 
       
 dbus_bool_t _dbus_marshal_double     (DBusString          *str,
diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
index 49269247..041bf667 100644
--- a/dbus/dbus-message.c
+++ b/dbus/dbus-message.c
@@ -76,6 +76,11 @@ struct DBusMessage
   unsigned int locked : 1; /**< Message being sent, no modifications allowed. */
 };
 
+/**
+ * @brief Internals of DBusMessageIter
+ * 
+ * Object representing a position in a message. All fields are internal.
+ */
 struct DBusMessageIter
 {
   int refcount; /**< Reference count */
@@ -125,7 +130,7 @@ _dbus_message_set_client_serial (DBusMessage  *message,
 static void
 dbus_message_write_header (DBusMessage *message)
 {
-  char *header;
+  char *len_data;
   
   _dbus_assert (message->client_serial != -1);
   
@@ -135,21 +140,28 @@ dbus_message_write_header (DBusMessage *message)
   /* We just lengthen the string here and pack in the real length later */
   _dbus_string_lengthen (&message->header, 4);
   
-  _dbus_marshal_int32 (&message->header, DBUS_COMPILER_BYTE_ORDER, _dbus_string_get_length (&message->body));
+  _dbus_marshal_int32 (&message->header, DBUS_COMPILER_BYTE_ORDER,
+                       _dbus_string_get_length (&message->body));
 
   /* Marshal client serial */
-  _dbus_marshal_int32 (&message->header, DBUS_COMPILER_BYTE_ORDER, message->client_serial);
+  _dbus_assert (message->client_serial >= 0);
+  _dbus_marshal_int32 (&message->header, DBUS_COMPILER_BYTE_ORDER,
+                       message->client_serial);
 
   /* Marshal message service */
   if (message->service)
     {
+      _dbus_string_align_length (&message->header, 4);
       _dbus_string_append_len (&message->header, DBUS_HEADER_FIELD_SERVICE, 4);
       _dbus_string_append_byte (&message->header, DBUS_TYPE_STRING);
       
-      _dbus_marshal_string (&message->header, DBUS_COMPILER_BYTE_ORDER, message->service);
+      _dbus_marshal_string (&message->header, DBUS_COMPILER_BYTE_ORDER,
+                            message->service);
     }
 
   /* Marshal message name */
+  _dbus_assert (message->name != NULL);
+  _dbus_string_align_length (&message->header, 4);
   _dbus_string_append_len (&message->header, DBUS_HEADER_FIELD_NAME, 4);
   _dbus_string_append_byte (&message->header, DBUS_TYPE_STRING);
 
@@ -161,12 +173,14 @@ dbus_message_write_header (DBusMessage *message)
       _dbus_string_append_len (&message->header, DBUS_HEADER_FIELD_REPLY, 4);
 
       _dbus_string_append_byte (&message->header, DBUS_TYPE_INT32);
-      _dbus_marshal_int32 (&message->header, DBUS_COMPILER_BYTE_ORDER, message->reply_serial);
+      _dbus_marshal_int32 (&message->header, DBUS_COMPILER_BYTE_ORDER,
+                           message->reply_serial);
     }
   
   /* Fill in the length */
-  _dbus_string_get_data_len (&message->header, &header, 4, 4);
-  dbus_pack_int32 (_dbus_string_get_length (&message->header), DBUS_COMPILER_BYTE_ORDER, header);
+  _dbus_string_get_data_len (&message->header, &len_data, 4, 4);
+  _dbus_pack_int32 (_dbus_string_get_length (&message->header),
+                    DBUS_COMPILER_BYTE_ORDER, len_data);
 }
 
 /**
@@ -213,8 +227,9 @@ _dbus_message_lock (DBusMessage  *message)
  * Constructs a new message. Returns #NULL if memory
  * can't be allocated for the message.
  *
+ * @todo use DBusString internally to store service and name.
+ *
  * @param service service that the message should be sent to
- * should be sent to
  * @param name name of the message
  * @returns a new DBusMessage, free with dbus_message_unref()
  * @see dbus_message_unref()
@@ -240,12 +255,16 @@ dbus_message_new (const char *service,
   
   if (!_dbus_string_init (&message->header, _DBUS_MAX_MESSAGE_LENGTH))
     {
+      dbus_free (message->service);
+      dbus_free (message->name);
       dbus_free (message);
       return NULL;
     }
 
   if (!_dbus_string_init (&message->body, _DBUS_MAX_MESSAGE_LENGTH))
     {
+      dbus_free (message->service);
+      dbus_free (message->name);
       _dbus_string_free (&message->header);
       dbus_free (message);
       return NULL;
@@ -285,7 +304,8 @@ dbus_message_unref (DBusMessage *message)
     {
       _dbus_string_free (&message->header);
       _dbus_string_free (&message->body);
-      
+
+      dbus_free (message->service);
       dbus_free (message->name);
       dbus_free (message);
     }
@@ -310,18 +330,22 @@ dbus_message_get_name (DBusMessage *message)
  * The list is terminated with 0.
  *
  * @param message the message
- * @param ... list of fields.
+ * @param type of the first field
+ * @param ... value of first field, list of additional type-value pairs
  * @returns #TRUE on success
  */
 dbus_bool_t
 dbus_message_append_fields (DBusMessage *message,
+                            int first_field_type,
 			    ...)
 {
   dbus_bool_t retval;
   va_list var_args;
 
-  va_start (var_args, message);
-  retval = dbus_message_append_fields_valist (message, var_args);
+  va_start (var_args, first_field_type);
+  retval = dbus_message_append_fields_valist (message,
+                                              first_field_type,
+                                              var_args);
   va_end (var_args);
 
   return retval;
@@ -332,18 +356,20 @@ dbus_message_append_fields (DBusMessage *message,
  *
  * @see dbus_message_append_fields.  
  * @param message the message
- * @param var_args list of type/value pairs
+ * @param first_field_type type of first field
+ * @param var_args value of first field, then list of type/value pairs
  * @returns #TRUE on success
  */
 dbus_bool_t
 dbus_message_append_fields_valist (DBusMessage *message,
+                                   int          first_field_type,
 				   va_list      var_args)
 {
   int type, old_len;
 
   old_len = _dbus_string_get_length (&message->body);
   
-  type = va_arg (var_args, int);
+  type = first_field_type;
 
   while (type != 0)
     {
@@ -375,9 +401,8 @@ dbus_message_append_fields_valist (DBusMessage *message,
 
 	    if (!dbus_message_append_byte_array (message, data, len))
 	      goto enomem;
-
-	    break;
 	  }
+          break;
 	default:
 	  _dbus_warn ("Unknown field type %d\n", type);
 	}
@@ -516,18 +541,20 @@ dbus_message_append_byte_array (DBusMessage         *message,
  * stored. The list is terminated with 0.
  *
  * @param message the message
- * @param ... list of fields.
+ * @param first_field_type the first field type
+ * @param ... location for first field value, then list of type-location pairs
  * @returns #TRUE on success
  */
 dbus_bool_t
 dbus_message_get_fields (DBusMessage *message,
+                         int          first_field_type,
 			 ...)
 {
   dbus_bool_t retval;
   va_list var_args;
 
-  va_start (var_args, message);
-  retval = dbus_message_get_fields_valist (message, var_args);
+  va_start (var_args, first_field_type);
+  retval = dbus_message_get_fields_valist (message, first_field_type, var_args);
   va_end (var_args);
 
   return retval;
@@ -536,13 +563,22 @@ dbus_message_get_fields (DBusMessage *message,
 /**
  * This function takes a va_list for use by language bindings
  *
+ * @todo this function (or some lower-level non-convenience function)
+ * needs better error handling; should allow the application to
+ * distinguish between out of memory, and bad data from the remote
+ * app. It also needs to not leak a bunch of args when it gets
+ * to the arg that's bad, as that would be a security hole
+ * (allow one app to force another to leak memory)
+ *
  * @see dbus_message_get_fields
  * @param message the message
- * @param var_args list of type/pointer pairs
+ * @param first_field_type type of the first field
+ * @param var_args return location for first field, followed by list of type/location pairs
  * @returns #TRUE on success
  */
 dbus_bool_t
 dbus_message_get_fields_valist (DBusMessage *message,
+                                int          first_field_type,
 				va_list      var_args)
 {
   int spec_type, msg_type, i;
@@ -553,7 +589,7 @@ dbus_message_get_fields_valist (DBusMessage *message,
   if (iter == NULL)
     return FALSE;
   
-  spec_type = va_arg (var_args, int);
+  spec_type = first_field_type;
   i = 0;
   
   while (spec_type != 0)
@@ -632,7 +668,7 @@ dbus_message_get_fields_valist (DBusMessage *message,
 	{
 	  _dbus_warn ("More fields than exist in the message were specified\n");
 
-	  dbus_message_iter_unref (iter);	  
+	  dbus_message_iter_unref (iter);  
 	  return FALSE;
 	}
       i++;
@@ -646,6 +682,12 @@ dbus_message_get_fields_valist (DBusMessage *message,
  * Returns a DBusMessageIter representing the fields of the
  * message passed in.
  *
+ * @todo IMO the message iter should follow the GtkTextIter pattern,
+ * a static object with a "stamp" value used to detect invalid
+ * iter uses (uninitialized or after changing the message).
+ * ref/unref is kind of annoying to deal with, and slower too.
+ * This implies not ref'ing the message from the iter.
+ *
  * @param message the message
  * @returns a new iter.
  */
@@ -783,7 +825,7 @@ dbus_message_iter_get_string (DBusMessageIter *iter)
   _dbus_assert (dbus_message_iter_get_field_type (iter) == DBUS_TYPE_STRING);
 
   return _dbus_demarshal_string (&iter->message->body, iter->message->byte_order,
-				iter->pos + 1, NULL);
+                                 iter->pos + 1, NULL);
 }
 
 /**
@@ -987,6 +1029,31 @@ _dbus_message_loader_get_buffer (DBusMessageLoader  *loader,
  */
 #define DBUS_MINIMUM_HEADER_SIZE 16
 
+/** Pack four characters as in "abcd" into a uint32 */
+#define FOUR_CHARS_TO_UINT32(a, b, c, d)                \
+                      ((((dbus_uint32_t)a) << 24) |     \
+                       (((dbus_uint32_t)b) << 16) |     \
+                       (((dbus_uint32_t)c) << 8)  |     \
+                       ((dbus_uint32_t)d))
+
+/** DBUS_HEADER_FIELD_NAME packed into a dbus_uint32_t */
+#define DBUS_HEADER_FIELD_NAME_AS_UINT32    \
+  FOUR_CHARS_TO_UINT32 ('n', 'a', 'm', 'e')
+
+/** DBUS_HEADER_FIELD_SERVICE packed into a dbus_uint32_t */
+#define DBUS_HEADER_FIELD_SERVICE_AS_UINT32 \
+  FOUR_CHARS_TO_UINT32 ('s', 'r', 'v', 'c')
+
+/** DBUS_HEADER_FIELD_REPLY packed into a dbus_uint32_t */
+#define DBUS_HEADER_FIELD_REPLY_AS_UINT32   \
+  FOUR_CHARS_TO_UINT32 ('r', 'p', 'l', 'y')
+
+/* FIXME should be using DBusString for the stuff we demarshal.  char*
+ * evil. Also, out of memory handling here seems suboptimal.
+ * Should probably report it as a distinct error from "corrupt message,"
+ * so we can postpone parsing this message. Also, we aren't
+ * checking for demarshal failure everywhere.
+ */
 static dbus_bool_t
 decode_header_data (DBusString   *data,
 		    int		  header_len,
@@ -1007,27 +1074,49 @@ decode_header_data (DBusString   *data,
   /* Now handle the fields */
   while (pos < header_len)
     {
+      pos = _DBUS_ALIGN_VALUE (pos, 4);
+      
+      if ((pos + 4) > header_len)
+        return FALSE;      
+      
       _dbus_string_get_const_data_len (data, &field, pos, 4);
       pos += 4;
 
-      if (pos > header_len)
-	  return FALSE;
-      
-      if (strncmp (field, DBUS_HEADER_FIELD_SERVICE, 4) == 0)
-	{
+      _dbus_assert (_DBUS_ALIGN_ADDRESS (field, 4) == field);
+
+      /* I believe FROM_BE is right, but if not we'll find out
+       * I guess. ;-)
+       */
+      switch (DBUS_UINT32_FROM_BE (*(int*)field))
+        {
+        case DBUS_HEADER_FIELD_SERVICE_AS_UINT32:
+          if (*service != NULL)
+            {
+              _dbus_verbose ("%s field provided twice\n",
+                             DBUS_HEADER_FIELD_SERVICE);
+              goto failed;
+            }
+          
 	  *service = _dbus_demarshal_string (data, byte_order, pos + 1, &new_pos);
-	}
-      else if (strncmp (field, DBUS_HEADER_FIELD_NAME, 4) == 0)
-	{
+          /* FIXME check for demarshal failure SECURITY */
+          break;
+        case DBUS_HEADER_FIELD_NAME_AS_UINT32:
+          if (*name != NULL)
+            {
+              _dbus_verbose ("%s field provided twice\n",
+                             DBUS_HEADER_FIELD_NAME);
+              goto failed;
+            }
+          
 	  *name = _dbus_demarshal_string (data, byte_order, pos + 1, &new_pos);
-	}
-      else
-	{
-	  _dbus_verbose ("Encountered an unknown header field: %c%c%c%c\n",
+          /* FIXME check for demarshal failure SECURITY */
+          break;
+        default:
+	  _dbus_verbose ("Ignoring an unknown header field: %c%c%c%c\n",
 			 field[0], field[1], field[2], field[3]);
 	  
 	  if (!_dbus_marshal_get_field_end_pos (data, byte_order, pos, &new_pos))
-	    return FALSE;
+            goto failed;
 	}
 
       if (new_pos > header_len)
@@ -1037,7 +1126,11 @@ decode_header_data (DBusString   *data,
     }
   
   return TRUE;
-  
+
+ failed:
+  dbus_free (*service);
+  dbus_free (*name);
+  return FALSE;
 }
 
 /**
@@ -1070,22 +1163,28 @@ _dbus_message_loader_return_buffer (DBusMessageLoader  *loader,
       int byte_order, header_len, body_len;
       
       _dbus_string_get_const_data_len (&loader->data, &header_data, 0, 16);
+
+      _dbus_assert (_DBUS_ALIGN_ADDRESS (header_data, 4) == header_data);
+      
       byte_order = header_data[0];
 
       if (byte_order != DBUS_LITTLE_ENDIAN &&
 	  byte_order != DBUS_BIG_ENDIAN)
 	{
+          _dbus_verbose ("Message with bad byte order '%c' received\n",
+                         byte_order);
 	  loader->corrupted = TRUE;
 	  return;
 	}
 
-      header_len = dbus_unpack_int32 (byte_order, header_data + 4);
-      body_len = dbus_unpack_int32 (byte_order, header_data + 8);
+      header_len = _dbus_unpack_int32 (byte_order, header_data + 4);
+      body_len = _dbus_unpack_int32 (byte_order, header_data + 8);
 
       if (header_len + body_len > _DBUS_MAX_MESSAGE_LENGTH)
 	{
+          _dbus_verbose ("Message claimed length header = %d body = %d exceeds max message length %d\n",
+                         header_len, body_len, _DBUS_MAX_MESSAGE_LENGTH);
 	  loader->corrupted = TRUE;
-
 	  return;
 	}
 
@@ -1093,21 +1192,24 @@ _dbus_message_loader_return_buffer (DBusMessageLoader  *loader,
 	{
 	  dbus_int32_t client_serial;
 	  char *service, *name;
-	  
-	  if (!decode_header_data (&loader->data, header_len, byte_order,
+
+          /* FIXME right now if this doesn't have enough memory, the
+           * loader becomes corrupted. Instead we should just not
+           * parse this message for now.
+           */
+ 	  if (!decode_header_data (&loader->data, header_len, byte_order,
 				   &client_serial, &service, &name))
 	    {
 	      loader->corrupted = TRUE;
-
 	      return;
 	    }
 
-	  message = dbus_message_new (service, name);
-	  dbus_free (service);
+  	  message = dbus_message_new (service, name);
+          dbus_free (service);
 	  dbus_free (name);
 	  
 	  if (message == NULL)
-	    break; /* ugh, postpone this I guess. */
+            break; /* ugh, postpone this I guess. */
 
 	  _dbus_string_copy (&loader->data, header_len, &message->body, 0);
 	  _dbus_message_set_client_serial (message, client_serial);
diff --git a/dbus/dbus-message.h b/dbus/dbus-message.h
index 10fb48a3..f1b49f9b 100644
--- a/dbus/dbus-message.h
+++ b/dbus/dbus-message.h
@@ -46,8 +46,10 @@ const char*  dbus_message_get_name (DBusMessage *message);
 
 
 dbus_bool_t dbus_message_append_fields        (DBusMessage         *message,
+                                               int                  first_field_type,
 					       ...);
 dbus_bool_t dbus_message_append_fields_valist (DBusMessage         *message,
+                                               int                  first_field_type,
 					       va_list              var_args);
 dbus_bool_t dbus_message_append_int32         (DBusMessage         *message,
 					       dbus_int32_t         value);
@@ -65,8 +67,10 @@ dbus_bool_t dbus_message_append_byte_array    (DBusMessage         *message,
 DBusMessageIter *dbus_message_get_fields_iter     (DBusMessage     *message);
 
 dbus_bool_t dbus_message_get_fields          (DBusMessage *message,
+                                              int          first_field_type,
 					      ...);
 dbus_bool_t dbus_message_get_fields_valist   (DBusMessage *message,
+                                              int          first_field_type,
 					      va_list      var_args);
 
 void        dbus_message_iter_ref            (DBusMessageIter *iter);
diff --git a/dbus/dbus-string.c b/dbus/dbus-string.c
index 6dc28577..c2f4150a 100644
--- a/dbus/dbus-string.c
+++ b/dbus/dbus-string.c
@@ -526,6 +526,41 @@ _dbus_string_set_length (DBusString *str,
   return set_length (real, length);
 }
 
+/**
+ * Align the length of a string to a specific alignment (typically 4 or 8)
+ * by appending nul bytes to the string.
+ *
+ * @param str a string
+ * @param alignment the alignment
+ * @returns #FALSE if no memory
+ */
+dbus_bool_t
+_dbus_string_align_length (DBusString *str,
+                           int         alignment)
+{
+  int new_len;
+  int delta;
+  DBUS_STRING_PREAMBLE (str);
+  _dbus_assert (alignment >= 1);
+  _dbus_assert (alignment <= 16); /* arbitrary */
+
+  new_len = _DBUS_ALIGN_VALUE (real->len, alignment);
+
+  delta = new_len - real->len;
+  _dbus_assert (delta >= 0);
+
+  if (delta == 0)
+    return TRUE;
+
+  if (!set_length (real, new_len))
+    return FALSE;
+
+  memset (real->str + (new_len - delta),
+          '\0', delta);
+
+  return TRUE;
+}
+
 static dbus_bool_t
 append (DBusRealString *real,
         const char     *buffer,
diff --git a/dbus/dbus-string.h b/dbus/dbus-string.h
index 471ac0c2..b2335086 100644
--- a/dbus/dbus-string.h
+++ b/dbus/dbus-string.h
@@ -72,12 +72,14 @@ dbus_bool_t _dbus_string_steal_data_len     (DBusString        *str,
 
 int  _dbus_string_get_length         (const DBusString  *str);
 
-dbus_bool_t _dbus_string_lengthen   (DBusString *str,
-                                     int         additional_length);
-void        _dbus_string_shorten    (DBusString *str,
-                                     int         length_to_remove);
-dbus_bool_t _dbus_string_set_length (DBusString *str,
-                                     int         length);
+dbus_bool_t _dbus_string_lengthen     (DBusString *str,
+                                       int         additional_length);
+void        _dbus_string_shorten      (DBusString *str,
+                                       int         length_to_remove);
+dbus_bool_t _dbus_string_set_length   (DBusString *str,
+                                       int         length);
+dbus_bool_t _dbus_string_align_length (DBusString *str,
+                                       int         alignment);
 
 dbus_bool_t _dbus_string_append         (DBusString    *str,
                                          const char    *buffer);
diff --git a/dbus/dbus-test-main.c b/dbus/dbus-test-main.c
new file mode 100644
index 00000000..8ed77cce
--- /dev/null
+++ b/dbus/dbus-test-main.c
@@ -0,0 +1,36 @@
+/* -*- mode: C; c-file-style: "gnu" -*- */
+/* dbus-test.c  Program to run all tests
+ *
+ * Copyright (C) 2002  Red Hat Inc.
+ *
+ * Licensed under the Academic Free License version 1.2
+ * 
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+
+#include "dbus-types.h"
+#include "dbus-test.h"
+#include <stdio.h>
+#include <stdlib.h>
+
+int
+main (int    argc,
+      char **argv)
+{
+  dbus_internal_symbol_do_not_use_run_tests ();
+  return 0;
+}
diff --git a/dbus/dbus-test.c b/dbus/dbus-test.c
index 2866e084..a219069f 100644
--- a/dbus/dbus-test.c
+++ b/dbus/dbus-test.c
@@ -21,7 +21,6 @@
  *
  */
 
-#include "dbus-types.h"
 #include "dbus-test.h"
 #include <stdio.h>
 #include <stdlib.h>
@@ -33,34 +32,41 @@ die (const char *failure)
   exit (1);
 }
 
-int
-main (int    argc,
-      char **argv)
+/**
+ * An exported symbol to be run in order to execute
+ * unit tests. Should not be used by
+ * any app other than our test app, this symbol
+ * won't exist in some builds of the library.
+ * (with --enable-tests=no)
+ */
+void
+dbus_internal_symbol_do_not_use_run_tests (void)
 {
-  printf ("%s: running string tests\n", argv[0]);
+  printf ("%s: running string tests\n", "dbus-test");
   if (!_dbus_string_test ())
     die ("strings");
   
-  printf ("%s: running marshalling tests\n", argv[0]);
+  printf ("%s: running marshalling tests\n", "dbus-test");
   if (!_dbus_marshal_test ())
     die ("marshalling");
 
-  printf ("%s: running message tests\n", argv[0]);
+  printf ("%s: running message tests\n", "dbus-test");
   if (!_dbus_message_test ())
     die ("messages");
   
-  printf ("%s: running memory pool tests\n", argv[0]);
+  printf ("%s: running memory pool tests\n", "dbus-test");
   if (!_dbus_mem_pool_test ())
     die ("memory pools");
   
-  printf ("%s: running linked list tests\n", argv[0]);
+  printf ("%s: running linked list tests\n", "dbus-test");
   if (!_dbus_list_test ())
     die ("lists");
 
-  printf ("%s: running hash table tests\n", argv[0]);
+  printf ("%s: running hash table tests\n", "dbus-test");
   if (!_dbus_hash_test ())
     die ("hash tables");
   
-  printf ("%s: completed successfully\n", argv[0]);
-  return 0;
+  printf ("%s: completed successfully\n", "dbus-test");
 }
+
+
diff --git a/dbus/dbus-test.h b/dbus/dbus-test.h
index 881d0e42..64faf12b 100644
--- a/dbus/dbus-test.h
+++ b/dbus/dbus-test.h
@@ -33,4 +33,6 @@ dbus_bool_t _dbus_mem_pool_test (void);
 dbus_bool_t _dbus_string_test   (void);
 dbus_bool_t _dbus_message_test  (void);
 
+void dbus_internal_symbol_do_not_use_run_tests (void);
+
 #endif /* DBUS_TEST_H */