2005-02-10 Havoc Pennington <hp@redhat.com>

	* test/glib/test-dbus-glib.c (main): fix so this test doesn't fail
	(call dbus_g_proxy_add_signal)

	* dbus/dbus-server-unix.c (_dbus_server_new_for_tcp_socket):
	escape the hostname
	(_dbus_server_new_for_domain_socket): escape the path

	* dbus/dbus-address.c (dbus_address_escape_value): new
	(dbus_address_unescape_value): new
	(dbus_parse_address): unescape values

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

	* doc/dbus-specification.xml: explain how to escape values in
	addresses

10 files changed, 417 insertions, 51 deletions

diff --git a/ChangeLog b/ChangeLog
index a60a7ff3..085e542d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,23 @@
 2005-02-10  Havoc Pennington  <hp@redhat.com>
 
+	* test/glib/test-dbus-glib.c (main): fix so this test doesn't fail
+	(call dbus_g_proxy_add_signal)
+
+	* dbus/dbus-server-unix.c (_dbus_server_new_for_tcp_socket):
+	escape the hostname
+	(_dbus_server_new_for_domain_socket): escape the path
+
+	* dbus/dbus-address.c (dbus_address_escape_value): new
+	(dbus_address_unescape_value): new
+	(dbus_parse_address): unescape values
+
+	* dbus/dbus-string.c (_dbus_string_append_byte_as_hex): new function
+
+	* doc/dbus-specification.xml: explain how to escape values in
+	addresses
+
+2005-02-10  Havoc Pennington  <hp@redhat.com>
+
 	* dbus/dbus-message-factory.c (generate_special): modify test to
 	avoid using a non-basic dict key
 
diff --git a/dbus/dbus-address.c b/dbus/dbus-address.c
index 44675197..d5c38a84 100644
--- a/dbus/dbus-address.c
+++ b/dbus/dbus-address.c
@@ -2,7 +2,7 @@
 /* dbus-address.c  Server address parser.
  *
  * Copyright (C) 2003  CodeFactory AB
- * Copyright (C) 2004  Red Hat, Inc.
+ * Copyright (C) 2004,2005  Red Hat, Inc.
  *
  * Licensed under the Academic Free License version 2.1
  * 
@@ -140,7 +140,7 @@ dbus_address_entry_get_method (DBusAddressEntry *entry)
  *
  * @param entry the entry.
  * @param key the key.
- * @returns the key value. This string must not be fred.
+ * @returns the key value. This string must not be freed.
  */
 const char *
 dbus_address_entry_get_value (DBusAddressEntry *entry,
@@ -165,15 +165,150 @@ dbus_address_entry_get_value (DBusAddressEntry *entry,
   return NULL;
 }
 
+#define _DBUS_ADDRESS_OPTIONALLY_ESCAPED_BYTE(b)        \
+         (((b) >= 'a' && (b) <= 'z') ||                 \
+          ((b) >= 'A' && (b) <= 'Z') ||                 \
+          ((b) >= '0' && (b) <= '9') ||                 \
+          (b) == '-' ||                                 \
+          (b) == '_' ||                                 \
+          (b) == '/' ||                                 \
+          (b) == '\\' ||                                \
+          (b) == '.')
+
+/**
+ * Appends an escaped version of one string to another string,
+ * using the D-BUS address escaping mechanism
+ *
+ * @param escaped the string to append to
+ * @param unescaped the string to escape
+ * @returns #FALSE if no memory
+ */
+dbus_bool_t
+_dbus_address_append_escaped (DBusString       *escaped,
+                              const DBusString *unescaped)
+{
+  const char *p;
+  const char *end;
+  dbus_bool_t ret;
+  int orig_len;
+
+  ret = FALSE;
+
+  orig_len = _dbus_string_get_length (escaped);
+  p = _dbus_string_get_const_data (unescaped);
+  end = p + _dbus_string_get_length (unescaped);
+  while (p != end)
+    {
+      if (_DBUS_ADDRESS_OPTIONALLY_ESCAPED_BYTE (*p))
+        {
+          if (!_dbus_string_append_byte (escaped, *p))
+            goto out;
+        }
+      else
+        {
+          if (!_dbus_string_append_byte (escaped, '%'))
+            goto out;
+          if (!_dbus_string_append_byte_as_hex (escaped, *p))
+            goto out;
+        }
+      
+      ++p;
+    }
+
+  ret = TRUE;
+  
+ out:
+  if (!ret)
+    _dbus_string_set_length (escaped, orig_len);
+  return ret;
+}
+
+static dbus_bool_t
+append_unescaped_value (DBusString       *unescaped,
+                        const DBusString *escaped,
+                        int               escaped_start,
+                        int               escaped_len,
+                        DBusError        *error)
+{
+  const char *p;
+  const char *end;
+  dbus_bool_t ret;
+  
+  ret = FALSE;
+
+  p = _dbus_string_get_const_data (escaped) + escaped_start;
+  end = p + escaped_len;
+  while (p != end)
+    {
+      if (_DBUS_ADDRESS_OPTIONALLY_ESCAPED_BYTE (*p))
+        {
+          if (!_dbus_string_append_byte (unescaped, *p))
+            goto out;
+        }
+      else if (*p == '%')
+        {
+          /* Efficiency is king */
+          char buf[3];
+          DBusString hex;
+          int hex_end;
+          
+          ++p;
+
+          if ((p + 2) > end)
+            {
+              dbus_set_error (error, DBUS_ERROR_BAD_ADDRESS,
+                              "In D-BUS address, percent character was not followed by two hex digits");
+              goto out;
+            }
+            
+          buf[0] = *p;
+          ++p;
+          buf[1] = *p;
+          buf[2] = '\0';
+
+          _dbus_string_init_const (&hex, buf);
+
+          if (!_dbus_string_hex_decode (&hex, 0, &hex_end,
+                                        unescaped,
+                                        _dbus_string_get_length (unescaped)))
+            goto out;
+
+          if (hex_end != 2)
+            {
+              dbus_set_error (error, DBUS_ERROR_BAD_ADDRESS,
+                              "In D-BUS address, percent character was followed by characters other than hex digits");
+              goto out;
+            }
+        }
+      else
+        {
+          /* Error, should have been escaped */
+          dbus_set_error (error, DBUS_ERROR_BAD_ADDRESS,
+                          "In D-BUS address, character '%c' should have been escaped\n",
+                          *p);
+          goto out;
+        }
+      
+      ++p;
+    }
+
+  ret = TRUE;
+  
+ out:
+  if (!ret && error && !dbus_error_is_set (error))
+    _DBUS_SET_OOM (error);
+
+  _dbus_assert (ret || error == NULL || dbus_error_is_set (error));
+  
+  return ret;
+}
+
 /**
  * Parses an address string of the form:
  *
  * method:key=value,key=value;method:key=value
  *
  * @todo document address format in the specification
- *
- * @todo need to be able to escape ';' and ',' in the
- * key values, and the parsing needs to handle that.
  * 
  * @param address the address.
  * @param entry return location to an array of entries.
@@ -306,9 +441,10 @@ dbus_parse_address (const char         *address,
 		  goto error;
 		}
 
-	      if (!_dbus_string_copy_len (&str, equals_pos + 1, comma_pos - equals_pos - 1, value, 0))
+	      if (!append_unescaped_value (value, &str, equals_pos + 1,
+                                           comma_pos - equals_pos - 1, error))
 		{
-		  dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);		  
+                  _dbus_assert (error == NULL || dbus_error_is_set (error));
 		  _dbus_string_free (key);
 		  _dbus_string_free (value);
 
@@ -386,11 +522,124 @@ dbus_parse_address (const char         *address,
   
 }
 
+/**
+ * Escapes the given string as a value in a key=value pair
+ * for a D-BUS address.
+ *
+ * @param value the unescaped value
+ * @returns newly-allocated escaped value or #NULL if no memory
+ */
+char*
+dbus_address_escape_value (const char *value)
+{
+  DBusString escaped;
+  DBusString unescaped;
+  char *ret;
+
+  ret = NULL;
+
+  _dbus_string_init_const (&unescaped, value);
+  
+  if (!_dbus_string_init (&escaped))
+    return NULL;
+
+  if (!_dbus_address_append_escaped (&escaped, &unescaped))
+    goto out;
+  
+  if (!_dbus_string_steal_data (&escaped, &ret))
+    goto out;
+
+ out:
+  _dbus_string_free (&escaped);
+  return ret;
+}
+
+/**
+ * Unescapes the given string as a value in a key=value pair
+ * for a D-BUS address.
+ *
+ * @param value the escaped value
+ * @param error error to set if the unescaping fails
+ * @returns newly-allocated unescaped value or #NULL if no memory
+ */
+char*
+dbus_address_unescape_value (const char *value,
+                             DBusError  *error)
+{
+  DBusString unescaped;
+  DBusString escaped;
+  char *ret;
+  
+  ret = NULL;
+
+  _dbus_string_init_const (&escaped, value);
+  
+  if (!_dbus_string_init (&unescaped))
+    return NULL;
+
+  if (!append_unescaped_value (&unescaped, &escaped,
+                               0, _dbus_string_get_length (&escaped),
+                               error))
+    goto out;
+  
+  if (!_dbus_string_steal_data (&unescaped, &ret))
+    goto out;
+
+ out:
+  if (ret == NULL && error && !dbus_error_is_set (error))
+    _DBUS_SET_OOM (error);
+
+  _dbus_assert (ret != NULL || error == NULL || dbus_error_is_set (error));
+  
+  _dbus_string_free (&unescaped);
+  return ret;
+}
 
 /** @} */ /* End of public API */
 
 #ifdef DBUS_BUILD_TESTS
 #include "dbus-test.h"
+#include <stdlib.h>
+
+typedef struct
+{
+  const char *escaped;
+  const char *unescaped;
+} EscapeTest;
+
+static const EscapeTest escape_tests[] = {
+  { "abcde", "abcde" },
+  { "", "" },
+  { "%20%20", "  " },
+  { "%24", "$" },
+  { "%25", "%" },
+  { "abc%24", "abc$" },
+  { "%24abc", "$abc" },
+  { "abc%24abc", "abc$abc" },
+  { "/", "/" },
+  { "-", "-" },
+  { "_", "_" },
+  { "A", "A" },
+  { "I", "I" },
+  { "Z", "Z" },
+  { "a", "a" },
+  { "i", "i" },
+  { "z", "z" }
+};
+
+static const char* invalid_escaped_values[] = {
+  "%a",
+  "%q",
+  "%az",
+  "%%",
+  "%$$",
+  "abc%a",
+  "%axyz",
+  "%",
+  "$",
+  " ",
+  "*"
+};
 
 dbus_bool_t
 _dbus_address_test (void)
@@ -398,8 +647,69 @@ _dbus_address_test (void)
   DBusAddressEntry **entries;
   int len;  
   DBusError error;
+  int i;
 
   dbus_error_init (&error);
+
+  i = 0;
+  while (i < _DBUS_N_ELEMENTS (escape_tests))
+    {
+      const EscapeTest *test = &escape_tests[i];
+      char *escaped;
+      char *unescaped;
+
+      escaped = dbus_address_escape_value (test->unescaped);
+      if (escaped == NULL)
+        _dbus_assert_not_reached ("oom");
+
+      if (strcmp (escaped, test->escaped) != 0)
+        {
+          _dbus_warn ("Escaped '%s' as '%s' should have been '%s'\n",
+                      test->unescaped, escaped, test->escaped);
+          exit (1);
+        }
+      dbus_free (escaped);
+
+      unescaped = dbus_address_unescape_value (test->escaped, &error);
+      if (unescaped == NULL)
+        {
+          _dbus_warn ("Failed to unescape '%s': %s\n",
+                      test->escaped, error.message);
+          dbus_error_free (&error);
+          exit (1);
+        }
+
+      if (strcmp (unescaped, test->unescaped) != 0)
+        {
+          _dbus_warn ("Unescaped '%s' as '%s' should have been '%s'\n",
+                      test->escaped, unescaped, test->unescaped);
+          exit (1);
+        }
+      dbus_free (unescaped);
+      
+      ++i;
+    }
+
+  i = 0;
+  while (i < _DBUS_N_ELEMENTS (invalid_escaped_values))
+    {
+      char *unescaped;
+
+      unescaped = dbus_address_unescape_value (invalid_escaped_values[i],
+                                               &error);
+      if (unescaped != NULL)
+        {
+          _dbus_warn ("Should not have successfully unescaped '%s' to '%s'\n",
+                      invalid_escaped_values[i], unescaped);
+          dbus_free (unescaped);
+          exit (1);
+        }
+
+      _dbus_assert (dbus_error_is_set (&error));
+      dbus_error_free (&error);
+
+      ++i;
+    }
   
   if (!dbus_parse_address ("unix:path=/tmp/foo;debug:name=test,sliff=sloff;",
 			   &entries, &len, &error))
diff --git a/dbus/dbus-address.h b/dbus/dbus-address.h
index a98682e6..b82495d0 100644
--- a/dbus/dbus-address.h
+++ b/dbus/dbus-address.h
@@ -41,8 +41,9 @@ const char *dbus_address_entry_get_value  (DBusAddressEntry   *entry,
 const char *dbus_address_entry_get_method (DBusAddressEntry   *entry);
 void        dbus_address_entries_free     (DBusAddressEntry  **entries);
 
-
-
+char* dbus_address_escape_value   (const char *value);
+char* dbus_address_unescape_value (const char *value,
+                                   DBusError  *error);
 
 #endif /* DBUS_ADDRESS_H */
 
diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h
index 3f0a34fe..be19e1bc 100644
--- a/dbus/dbus-internals.h
+++ b/dbus/dbus-internals.h
@@ -284,6 +284,9 @@ _DBUS_DECLARE_GLOBAL_LOCK (message_cache);
 
 dbus_bool_t _dbus_threads_init_debug (void);
 
+dbus_bool_t   _dbus_address_append_escaped (DBusString       *escaped,
+                                            const DBusString *unescaped);
+
 DBUS_END_DECLS
 
 #endif /* DBUS_INTERNALS_H */
diff --git a/dbus/dbus-server-debug-pipe.c b/dbus/dbus-server-debug-pipe.c
index 0f8f1eda..090274aa 100644
--- a/dbus/dbus-server-debug-pipe.c
+++ b/dbus/dbus-server-debug-pipe.c
@@ -134,6 +134,7 @@ _dbus_server_debug_pipe_new (const char     *server_name,
 {
   DBusServerDebugPipe *debug_server;
   DBusString address;
+  DBusString name_str;
   
   _DBUS_ASSERT_ERROR_IS_CLEAR (error);
   
@@ -154,8 +155,9 @@ _dbus_server_debug_pipe_new (const char     *server_name,
   if (!_dbus_string_init (&address))
     goto nomem_1;
 
+  _dbus_string_init_const (&name_str, server_name);
   if (!_dbus_string_append (&address, "debug-pipe:name=") ||
-      !_dbus_string_append (&address, server_name))
+      !_dbus_address_append_escaped (&address, &name_str))
     goto nomem_2;
   
   debug_server->name = _dbus_strdup (server_name);
diff --git a/dbus/dbus-server-unix.c b/dbus/dbus-server-unix.c
index 12f4ca6a..f576b427 100644
--- a/dbus/dbus-server-unix.c
+++ b/dbus/dbus-server-unix.c
@@ -279,6 +279,7 @@ _dbus_server_new_for_domain_socket (const char     *path,
   int listen_fd;
   DBusString address;
   char *path_copy;
+  DBusString path_str;
   
   _DBUS_ASSERT_ERROR_IS_CLEAR (error);
 
@@ -288,11 +289,12 @@ _dbus_server_new_for_domain_socket (const char     *path,
       return NULL;
     }
 
+  _dbus_string_init_const (&path_str, path);
   if ((abstract &&
        !_dbus_string_append (&address, "unix:abstract=")) ||
       (!abstract &&
        !_dbus_string_append (&address, "unix:path=")) ||
-      !_dbus_string_append (&address, path))
+      !_dbus_address_append_escaped (&address, &path_str))
     {
       dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
       goto failed_0;
@@ -355,6 +357,7 @@ _dbus_server_new_for_tcp_socket (const char     *host,
   DBusServer *server;
   int listen_fd;
   DBusString address;
+  DBusString host_str;
   
   _DBUS_ASSERT_ERROR_IS_CLEAR (error);
 
@@ -366,9 +369,10 @@ _dbus_server_new_for_tcp_socket (const char     *host,
 
   if (host == NULL)
     host = "localhost";
-  
+
+  _dbus_string_init_const (&host_str, host);
   if (!_dbus_string_append (&address, "tcp:host=") ||
-      !_dbus_string_append (&address, host) ||
+      !_dbus_address_append_escaped (&address, &host_str) ||
       !_dbus_string_append (&address, ",port=") ||
       !_dbus_string_append_int (&address, port))
     {
diff --git a/dbus/dbus-string.c b/dbus/dbus-string.c
index f64d4746..5088bca9 100644
--- a/dbus/dbus-string.c
+++ b/dbus/dbus-string.c
@@ -2237,6 +2237,38 @@ _dbus_string_starts_with_c_str (const DBusString *a,
 #endif /* DBUS_BUILD_TESTS */
 
 /**
+ * Appends a two-character hex digit to a string, where the hex digit
+ * has the value of the given byte.
+ *
+ * @param str the string
+ * @param byte the byte
+ * @returns #FALSE if no memory
+ */
+dbus_bool_t
+_dbus_string_append_byte_as_hex (DBusString *str,
+                                 int         byte)
+{
+  const char hexdigits[16] = {
+    '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
+    'a', 'b', 'c', 'd', 'e', 'f'
+  };
+
+  if (!_dbus_string_append_byte (str,
+                                 hexdigits[(byte >> 4)]))
+    return FALSE;
+  
+  if (!_dbus_string_append_byte (str,
+                                 hexdigits[(byte & 0x0f)]))
+    {
+      _dbus_string_set_length (str,
+                               _dbus_string_get_length (str) - 1);
+      return FALSE;
+    }
+
+  return TRUE;
+}
+
+/**
  * Encodes a string in hex, the way MD5 and SHA-1 are usually
  * encoded. (Each byte is two hex digits.)
  *
@@ -2253,10 +2285,6 @@ _dbus_string_hex_encode (const DBusString *source,
                          int               insert_at)
 {
   DBusString result;
-  const char hexdigits[16] = {
-    '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
-    'a', 'b', 'c', 'd', 'e', 'f'
-  };
   const unsigned char *p;
   const unsigned char *end;
   dbus_bool_t retval;
@@ -2274,14 +2302,9 @@ _dbus_string_hex_encode (const DBusString *source,
   
   while (p != end)
     {
-      if (!_dbus_string_append_byte (&result,
-                                     hexdigits[(*p >> 4)]))
+      if (!_dbus_string_append_byte_as_hex (&result, *p))
         goto out;
       
-      if (!_dbus_string_append_byte (&result,
-                                     hexdigits[(*p & 0x0f)]))
-        goto out;
-
       ++p;
     }
 
diff --git a/dbus/dbus-string.h b/dbus/dbus-string.h
index 84098226..522a99b7 100644
--- a/dbus/dbus-string.h
+++ b/dbus/dbus-string.h
@@ -253,6 +253,8 @@ dbus_bool_t   _dbus_string_pop_line              (DBusString        *source,
                                                   DBusString        *dest);
 void          _dbus_string_delete_first_word     (DBusString        *str);
 void          _dbus_string_delete_leading_blanks (DBusString        *str);
+dbus_bool_t   _dbus_string_append_byte_as_hex    (DBusString        *str,
+                                                  int                byte);
 dbus_bool_t   _dbus_string_hex_encode            (const DBusString  *source,
                                                   int                start,
                                                   DBusString        *dest,
diff --git a/doc/dbus-specification.xml b/doc/dbus-specification.xml
index 75ffeea7..b56e0fc5 100644
--- a/doc/dbus-specification.xml
+++ b/doc/dbus-specification.xml
@@ -2131,7 +2131,7 @@
     <para>
       Server addresses consist of a transport name followed by a colon, and
       then an optional, comma-separated list of keys and values in the form key=value.
-      [FIXME how do you escape colon, comma, and semicolon in the values of the key=value pairs?]
+      Each value is escaped.
     </para>
     <para>
       For example: 
@@ -2139,6 +2139,33 @@
       Which is the address to a unix socket with the path /tmp/dbus-test.
     </para>
     <para>
+      Value escaping is similar to URI escaping but simpler.
+      <itemizedlist>
+        <listitem>
+          <para>
+            The set of optionally-escaped bytes is:
+            <literal>[0-9A-Za-z_-/.\]</literal>. To escape, each
+            <emphasis>byte</emphasis> (note, not character) which is not in the
+            set of optionally-escaped bytes must be replaced with an ASCII
+            percent (<literal>%</literal>) and the value of the byte in hex.
+            The hex value must always be two digits, even if the first digit is
+            zero. The optionally-escaped bytes may be escaped if desired.
+          </para>
+        </listitem>
+        <listitem>
+          <para>
+            To unescape, append each byte in the value; if a byte is an ASCII
+            percent (<literal>%</literal>) character then append the following
+            hex value instead. It is an error if a <literal>%</literal> byte
+            does not have two hex digits following. It is an error if a
+            non-optionally-escaped byte is seen unescaped.
+          </para>
+        </listitem>
+      </itemizedlist>
+      The set of optionally-escaped bytes is intended to preserve address 
+      readability and convenience.
+    </para>
+    <para>
       [FIXME clarify if attempting to connect to each is a requirement 
       or just a suggestion]
       When connecting to a server, multiple server addresses can be
diff --git a/test/glib/test-dbus-glib.c b/test/glib/test-dbus-glib.c
index 5f565a0a..cee9316b 100644
--- a/test/glib/test-dbus-glib.c
+++ b/test/glib/test-dbus-glib.c
@@ -16,35 +16,9 @@ timed_exit (gpointer loop)
 
 static void
 foo_signal_handler (DBusGProxy  *proxy,
+                    double       d,
                     void        *user_data)
 {
-#if 0
-  double d;
-  
-  /* FIXME - need to fix up dbus_gproxy_signal_connect() to be able to
-   * get signal args
-   */
-  
-  DBusError derror;
-  
-  if (!dbus_message_is_signal (signal,
-                               "org.freedesktop.TestSuite",
-                               "Foo"))
-    {
-      g_printerr ("Signal handler received the wrong message\n");
-      exit (1);
-    }
-
-  dbus_error_init (&derror);
-  if (!dbus_message_get_args (signal, &derror, DBUS_TYPE_DOUBLE,
-                              &d, DBUS_TYPE_INVALID))
-    {
-      g_printerr ("failed to get signal args: %s\n", derror.message);
-      dbus_error_free (&derror);
-      exit (1);
-    }
-#endif
-
   n_times_foo_received += 1;
 
   g_main_loop_quit (loop);
@@ -225,6 +199,8 @@ main (int argc, char **argv)
 
   /* Test oneway call and signal handling */
 
+  dbus_g_proxy_add_signal (proxy, "Foo", DBUS_TYPE_DOUBLE_AS_STRING);
+  
   dbus_g_proxy_connect_signal (proxy, "Foo",
                                G_CALLBACK (foo_signal_handler),
                                NULL, NULL);