Use DBusString for all relocation and install-root code

This means we handle OOM correctly, and makes it obvious
that we are not overflowing buffers. This change does not
affect the actual content of the strings.

Instead of redefining DBUS_DATADIR to be a function call
(which hides the fact that DBUS_DATADIR is used),
this patch makes each use explicit: DBUS_DATADIR
is always the #define from configure or cmake, before
replacing the prefix.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=83539
Tested-by: Ralf Habacker <ralf.habacker@freenet.de>
Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de>

bus/activation.c

@@ -261,6 +261,7 @@ update_desktop_file_entry (BusActivation       *activation,
   DBusString file_path;
   DBusError tmp_error;
   dbus_bool_t retval;
+  DBusString str;
 
   _DBUS_ASSERT_ERROR_IS_CLEAR (error);
 
@@ -308,9 +309,18 @@ update_desktop_file_entry (BusActivation       *activation,
                                     error))
     goto out;
 
-  exec = _dbus_strdup (_dbus_replace_install_prefix (exec_tmp));
+  if (!_dbus_string_init (&str))
-  dbus_free (exec_tmp);
+    goto out;
-  exec_tmp = NULL;
+
+  if (!_dbus_string_append (&str, exec_tmp) ||
+      !_dbus_replace_install_prefix (&str) ||
+      !_dbus_string_steal_data (&str, &exec))
+    {
+      _dbus_string_free (&str);
+      goto out;
+    }
+
+  _dbus_string_free (&str);
 
   /* user is not _required_ unless we are using system activation */
   if (!bus_desktop_file_get_string (desktop_file,
@@ -466,6 +476,7 @@ update_desktop_file_entry (BusActivation       *activation,
 
 out:
   /* if these have been transferred into entry, the variables will be NULL */
+  dbus_free (exec_tmp);
   dbus_free (name);
   dbus_free (exec);
   dbus_free (user);

bus/config-parser.c

@@ -3402,17 +3402,25 @@ test_default_session_servicedirs (void)
   DBusList *link;
   DBusString progs;
   int i;
+  dbus_bool_t ret = FALSE;
 
 #ifdef DBUS_WIN
   const char *common_progs;
-  char buffer[1024];
+  DBusString install_root_based;
 
-  if (_dbus_get_install_root(buffer, sizeof(buffer)))
+  if (!_dbus_string_init (&install_root_based) ||
+      !_dbus_get_install_root (&install_root_based))
+    _dbus_assert_not_reached ("OOM getting install root");
+
+  if (_dbus_string_get_length (&install_root_based) > 0)
     {
-      strcat(buffer,DBUS_DATADIR);
+      if (!_dbus_string_append (&install_root_based, DBUS_DATADIR) ||
-      strcat(buffer,"/dbus-1/services");
+          !_dbus_string_append (&install_root_based, "/dbus-1/services"))
-      test_session_service_dir_matches[0] = buffer;
+        _dbus_assert_not_reached ("OOM appending to install root");
+
+      test_session_service_dir_matches[0] = _dbus_string_get_const_data (&install_root_based);
     }
+
 #endif
 
   /* On Unix we don't actually use this variable, but it's easier to handle the
@@ -3426,16 +3434,11 @@ test_default_session_servicedirs (void)
   if (common_progs) 
     {
       if (!_dbus_string_append (&progs, common_progs)) 
-        {
+        goto out;
-          _dbus_string_free (&progs);
-          return FALSE;
-        }
 
       if (!_dbus_string_append (&progs, "/dbus-1/services")) 
-        {
+        goto out;
-          _dbus_string_free (&progs);
+
-          return FALSE;
-        }
       test_session_service_dir_matches[1] = _dbus_string_get_const_data(&progs);
     }
 #endif
@@ -3457,8 +3460,7 @@ test_default_session_servicedirs (void)
           printf ("error with default session service directories\n");
 	      dbus_free (link->data);
     	  _dbus_list_free_link (link);
-          _dbus_string_free (&progs);
+          goto out;
-          return FALSE;
         }
  
       dbus_free (link->data);
@@ -3485,8 +3487,7 @@ test_default_session_servicedirs (void)
           printf ("more directories parsed than in match set\n");
           dbus_free (link->data);
           _dbus_list_free_link (link);
-          _dbus_string_free (&progs);
+          goto out;
-          return FALSE;
         }
  
       if (strcmp (test_session_service_dir_matches[i], 
@@ -3497,8 +3498,7 @@ test_default_session_servicedirs (void)
                   test_session_service_dir_matches[i]);
           dbus_free (link->data);
           _dbus_list_free_link (link);
-          _dbus_string_free (&progs);
+          goto out;
-          return FALSE;
         }
 
       ++i;
@@ -3511,13 +3511,17 @@ test_default_session_servicedirs (void)
     {
       printf ("extra data %s in the match set was not matched\n",
               test_session_service_dir_matches[i]);
-
+      goto out;
-      _dbus_string_free (&progs);
-      return FALSE;
     }
 
+  ret = TRUE;
+
+out:
   _dbus_string_free (&progs);
-  return TRUE;
+#ifdef DBUS_WIN
+  _dbus_string_free (&install_root_based);
+#endif
+  return ret;
 }
 
 static const char *test_system_service_dir_matches[] = 

dbus/dbus-sysdeps-util-unix.c

@@ -1272,17 +1272,18 @@ fail:
   return FALSE;
 }
 
-/*
+/**
- * replaces the term DBUS_PREFIX in configure_time_path by the
+ * Replace the DBUS_PREFIX in the given path, in-place, by the
- * current dbus installation directory. On unix this function is a noop
+ * current D-Bus installation directory. On Unix this function
+ * does nothing, successfully.
  *
- * @param configure_time_path
+ * @param path path to edit
- * @return real path
+ * @return #FALSE on OOM
  */
-const char *
+dbus_bool_t
-_dbus_replace_install_prefix (const char *configure_time_path)
+_dbus_replace_install_prefix (DBusString *path)
 {
-  return configure_time_path;
+  return TRUE;
 }
 
 #define DBUS_UNIX_STANDARD_SESSION_SERVICEDIR "/dbus-1/services"

dbus/dbus-sysdeps-util-win.c

@@ -1469,42 +1469,51 @@ _dbus_command_for_pid (unsigned long  pid,
   return FALSE;
 }
 
-/*
+/**
- * replaces the term DBUS_PREFIX in configure_time_path by the
+ * Replace the DBUS_PREFIX in the given path, in-place, by the
- * current dbus installation directory. On unix this function is a noop
+ * current D-Bus installation directory. On Unix this function
+ * does nothing, successfully.
  *
- * @param configure_time_path
+ * @param path path to edit
- * @return real path
+ * @return #FALSE on OOM
  */
-const char *
+dbus_bool_t
-_dbus_replace_install_prefix (const char *configure_time_path)
+_dbus_replace_install_prefix (DBusString *path)
 {
 #ifndef DBUS_PREFIX
-  return configure_time_path;
+  /* leave path unchanged */
+  return TRUE;
 #else
-  static char retval[1000];
+  DBusString runtime_prefix;
-  static char runtime_prefix[1000];
-  int len = 1000;
   int i;
 
-  if (!configure_time_path)
+  if (!_dbus_string_init (&runtime_prefix))
-    return NULL;
+    return FALSE;
 
-  if ((!_dbus_get_install_root(runtime_prefix, len) ||
+  if (!_dbus_get_install_root (&runtime_prefix))
-       strncmp (configure_time_path, DBUS_PREFIX "/",
+    {
-                strlen (DBUS_PREFIX) + 1))) {
+      _dbus_string_free (&runtime_prefix);
-     strncpy (retval, configure_time_path, sizeof (retval) - 1);
+      return FALSE;
-     /* strncpy does not guarantee to 0-terminate the string */
+    }
-     retval[sizeof (retval) - 1] = '\0';
-  } else {
-     size_t remaining;
 
-     strncpy (retval, runtime_prefix, sizeof (retval) - 1);
+  if (_dbus_string_get_length (&runtime_prefix) == 0)
-     retval[sizeof (retval) - 1] = '\0';
+    {
-     remaining = sizeof (retval) - 1 - strlen (retval);
+      /* cannot determine install root, leave path unchanged */
-     strncat (retval,
+      _dbus_string_free (&runtime_prefix);
-         configure_time_path + strlen (DBUS_PREFIX) + 1,
+      return TRUE;
-         remaining);
+    }
+
+  if (_dbus_string_starts_with_c_str (path, DBUS_PREFIX "/"))
+    {
+      /* Replace DBUS_PREFIX "/" with runtime_prefix.
+       * Note unusual calling convention: source is first, then dest */
+      if (!_dbus_string_replace_len (
+            &runtime_prefix, 0, _dbus_string_get_length (&runtime_prefix),
+            path, 0, strlen (DBUS_PREFIX) + 1))
+        {
+          _dbus_string_free (&runtime_prefix);
+          return FALSE;
+        }
     }
 
   /* Somehow, in some situations, backslashes get collapsed in the string.
@@ -1512,30 +1521,16 @@ _dbus_replace_install_prefix (const char *configure_time_path)
    * path separators, convert all backslashes to forward slashes.
    */
 
-  for(i = 0; retval[i] != '\0'; i++) {
+  for (i = 0; i < _dbus_string_get_length (path); i++)
-    if(retval[i] == '\\')
+    {
-      retval[i] = '/';
+      if (_dbus_string_get_byte (path, i) == '\\')
+        _dbus_string_set_byte (path, i, '/');
     }
-  return retval;
+
+  return TRUE;
 #endif
 }
 
-/**
- * return the relocated DATADIR
- *
- * @returns relocated DATADIR static string
- */
-
-static const char *
-_dbus_windows_get_datadir (void)
-{
-	return _dbus_replace_install_prefix(DBUS_DATADIR);
-}
-
-#undef DBUS_DATADIR
-#define DBUS_DATADIR _dbus_windows_get_datadir ()
-
-
 #define DBUS_STANDARD_SESSION_SERVICEDIR "/dbus-1/services"
 #define DBUS_STANDARD_SYSTEM_SERVICEDIR "/dbus-1/system-services"
 
@@ -1583,23 +1578,40 @@ _dbus_get_standard_session_servicedirs (DBusList **dirs)
  the code for accessing services requires absolute base pathes
  in case DBUS_DATADIR is relative make it absolute
 */
-#ifdef DBUS_WIN
   {
     DBusString p;
 
-    _dbus_string_init_const (&p, DBUS_DATADIR);
+    if (!_dbus_string_init (&p))
+      goto oom;
+
+    if (!_dbus_string_append (&p, DBUS_DATADIR) ||
+        !_dbus_replace_install_prefix (&p))
+      {
+        _dbus_string_free (&p);
+        goto oom;
+      }
 
     if (!_dbus_path_is_absolute (&p))
       {
-        char install_root[1000];
+        /* this only works because this is the first thing in the
-        if (_dbus_get_install_root (install_root, sizeof(install_root)))
+         * servicedir_path; if it wasn't, we'd have to use a temporary
-          if (!_dbus_string_append (&servicedir_path, install_root))
+         * string and copy it in */
+        if (!_dbus_get_install_root (&servicedir_path))
+          {
+            _dbus_string_free (&p);
             goto oom;
           }
       }
-#endif
+
-  if (!_dbus_string_append (&servicedir_path, DBUS_DATADIR))
+    if (!_dbus_string_append (&servicedir_path,
+          _dbus_string_get_const_data (&p)))
+      {
+        _dbus_string_free (&p);
         goto oom;
+      }
+
+    _dbus_string_free (&p);
+  }
 
   if (!_dbus_string_append (&servicedir_path, _DBUS_PATH_SEPARATOR))
     goto oom;
@@ -1660,7 +1672,8 @@ _dbus_get_config_file_name (DBusString *str,
 {
   DBusString tmp;
 
-  if (!_dbus_string_append (str, _dbus_windows_get_datadir ()))
+  if (!_dbus_string_append (str, DBUS_DATADIR) ||
+      !_dbus_replace_install_prefix (str))
     return FALSE;
 
   _dbus_string_init_const (&tmp, "dbus-1");

dbus/dbus-sysdeps-win.c

@@ -2812,14 +2812,11 @@ _dbus_get_install_root_as_hash(DBusString *out)
 {
     DBusString install_path;
 
-    char path[MAX_PATH*2];
-    int path_size = sizeof(path);
-
-    if (!_dbus_get_install_root(path,path_size))
-        return FALSE;
-
     _dbus_string_init(&install_path);
-    _dbus_string_append(&install_path,path);
+
+    if (!_dbus_get_install_root (&install_path) ||
+        _dbus_string_get_length (&install_path) == 0)
+        return FALSE;
 
     _dbus_string_init(out);
     _dbus_string_tolower_ascii(&install_path,0,_dbus_string_get_length(&install_path));
@@ -3288,34 +3285,73 @@ _dbus_get_is_errno_eagain_or_ewouldblock (int e)
 }
 
 /**
- * return the absolute path of the dbus installation 
+ * Fill str with the absolute path of the D-Bus installation, or truncate str
+ * to zero length if we cannot determine it.
  *
- * @param prefix buffer for installation path
+ * @param str buffer for installation path
- * @param len length of buffer
+ * @returns #FALSE on OOM, #TRUE if not OOM
- * @returns #FALSE on failure
  */
 dbus_bool_t
-_dbus_get_install_root(char *prefix, int len)
+_dbus_get_install_root (DBusString *str)
 {
-    //To find the prefix, we cut the filename and also \bin\ if present
+    /* this is just an initial guess */
-    DWORD pathLength;
+    DWORD pathLength = MAX_PATH;
     char *lastSlash;
-    SetLastError( 0 );
+    char *prefix;
-    pathLength = GetModuleFileNameA(_dbus_win_get_dll_hmodule(), prefix, len);
+
-    if ( pathLength == 0 || GetLastError() != 0 ) {
+    do
-        *prefix = '\0';
+      {
+        /* allocate enough space for our best guess at the length */
+        if (!_dbus_string_set_length (str, pathLength))
+          {
+            _dbus_string_set_length (str, 0);
             return FALSE;
           }
+
+        SetLastError (0);
+        pathLength = GetModuleFileNameA (_dbus_win_get_dll_hmodule (),
+            _dbus_string_get_data (str), _dbus_string_get_length (str));
+
+        if (pathLength == 0 || GetLastError () != 0)
+          {
+            /* failed, but not OOM */
+            _dbus_string_set_length (str, 0);
+            return TRUE;
+          }
+
+        /* if the return is strictly less than the buffer size, it has
+         * not been truncated, so we can continue */
+        if (pathLength < (DWORD) _dbus_string_get_length (str))
+          {
+            /* reduce the length to match what Windows filled in */
+            if (!_dbus_string_set_length (str, pathLength))
+              {
+                _dbus_string_set_length (str, 0);
+                return FALSE;
+              }
+
+            break;
+          }
+
+        /* else it may have been truncated; try with a larger buffer */
+        pathLength *= 2;
+      }
+    while (TRUE);
+
+    /* the rest of this function works by direct byte manipulation of the
+     * underlying buffer */
+    prefix = _dbus_string_get_data (str);
+
     lastSlash = _mbsrchr(prefix, '\\');
     if (lastSlash == NULL) {
-        *prefix = '\0';
+        /* failed, but not OOM */
-        return FALSE;
+        _dbus_string_set_length (str, 0);
+        return TRUE;
     }
     //cut off binary name
     lastSlash[1] = 0;
 
     //cut possible "\\bin"
-
     //this fails if we are in a double-byte system codepage and the
     //folder's name happens to end with the *bytes*
     //"\\bin"... (I.e. the second byte of some Han character and then
@@ -3327,6 +3363,9 @@ _dbus_get_install_root(char *prefix, int len)
     else if (lastSlash - prefix >= 12 && strnicmp(lastSlash - 12, "\\bin\\release", 12) == 0)
         lastSlash[-11] = 0;
 
+    /* fix up the length to match the byte-manipulation */
+    _dbus_string_set_length (str, strlen (prefix));
+
     return TRUE;
 }
 

dbus/dbus-sysdeps-win.h

@@ -85,7 +85,7 @@ _dbus_win_sid_to_name_and_domain (dbus_uid_t uid,
 dbus_bool_t _dbus_file_exists (const char *filename);
 
 DBUS_PRIVATE_EXPORT
-dbus_bool_t _dbus_get_install_root(char *prefix, int len);
+dbus_bool_t _dbus_get_install_root (DBusString *str);
 
 void        _dbus_threads_windows_init_global (void);
 void        _dbus_threads_windows_ensure_ctor_linked (void);

dbus/dbus-sysdeps.h

@@ -647,15 +647,7 @@ dbus_bool_t _dbus_change_to_daemon_user (const char *user,
 DBUS_PRIVATE_EXPORT
 void _dbus_flush_caches (void);
 
-/*
+dbus_bool_t _dbus_replace_install_prefix (DBusString *path);
- * replaces the term DBUS_PREFIX in configure_time_path by the
- * current dbus installation directory. On unix this function is a noop
- *
- * @param configure_time_path
- * @return real path
- */
-const char *
-_dbus_replace_install_prefix (const char *configure_time_path);
 
 /* Do not set this too high: it is a denial-of-service risk.
  * See <https://bugs.freedesktop.org/show_bug.cgi?id=82820>

test/manual-paths.c

@@ -14,14 +14,31 @@
 
 static dbus_bool_t print_install_root()
 {
-  char runtime_prefix[1000];
+  DBusString runtime_prefix;
 
-  if (!_dbus_get_install_root(runtime_prefix, sizeof(runtime_prefix)))
+  if (!_dbus_string_init (&runtime_prefix))
     {
-      fprintf(stderr, "dbus_get_install_root() failed\n");
+      _dbus_assert_not_reached ("out of memory");
       return FALSE;
     }
-  fprintf(stdout, "dbus_get_install_root() returned '%s'\n", runtime_prefix);
+
+  if (!_dbus_get_install_root (&runtime_prefix))
+    {
+      _dbus_assert_not_reached ("out of memory");
+      _dbus_string_free (&runtime_prefix);
+      return FALSE;
+    }
+
+  if (_dbus_string_get_length (&runtime_prefix) == 0)
+    {
+      fprintf (stderr, "_dbus_get_install_root() failed\n");
+      _dbus_string_free (&runtime_prefix);
+      return FALSE;
+    }
+
+  fprintf (stdout, "_dbus_get_install_root() returned '%s'\n",
+      _dbus_string_get_const_data (&runtime_prefix));
+  _dbus_string_free (&runtime_prefix);
   return TRUE;
 }
 
@@ -46,11 +63,25 @@ static dbus_bool_t print_service_dirs()
 
 static dbus_bool_t print_replace_install_prefix(const char *s)
 {
-  const char *s2 = _dbus_replace_install_prefix(s);
+  DBusString str;
-  if (!s2)
-    return FALSE;
 
-  fprintf(stdout, "replaced '%s' by '%s'\n", s, s2);
+  if (!_dbus_string_init (&str))
+    {
+      _dbus_assert_not_reached ("out of memory");
+      return FALSE;
+    }
+
+  if (!_dbus_string_append (&str, s) ||
+      !_dbus_replace_install_prefix (&str))
+    {
+      _dbus_assert_not_reached ("out of memory");
+      _dbus_string_free (&str);
+      return FALSE;
+    }
+
+  fprintf(stdout, "replaced '%s' by '%s'\n", s,
+      _dbus_string_get_const_data (&str));
+  _dbus_string_free (&str);
   return TRUE;
 }