From 379d01e318de1ec511cfeb1858004223b604dee4 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 30 Sep 2015 16:35:49 +0100 Subject: [PATCH] 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 --- bus/activation.c | 17 +++++- bus/config-parser.c | 52 +++++++++-------- dbus/dbus-sysdeps-util-unix.c | 17 +++--- dbus/dbus-sysdeps-util-win.c | 133 +++++++++++++++++++++++------------------- dbus/dbus-sysdeps-win.c | 83 +++++++++++++++++++------- dbus/dbus-sysdeps-win.h | 2 +- dbus/dbus-sysdeps.h | 10 +--- test/manual-paths.c | 47 ++++++++++++--- 8 files changed, 226 insertions(+), 135 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index 679a40e..3642b74 100644 --- a/bus/activation.c +++ b/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)); - dbus_free (exec_tmp); - exec_tmp = NULL; + if (!_dbus_string_init (&str)) + goto out; + + 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); diff --git a/bus/config-parser.c b/bus/config-parser.c index 3f59730..3cd17cc 100644 --- a/bus/config-parser.c +++ b/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); - strcat(buffer,"/dbus-1/services"); - test_session_service_dir_matches[0] = buffer; + if (!_dbus_string_append (&install_root_based, DBUS_DATADIR) || + !_dbus_string_append (&install_root_based, "/dbus-1/services")) + _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)) - { - _dbus_string_free (&progs); - return FALSE; - } + goto out; if (!_dbus_string_append (&progs, "/dbus-1/services")) - { - _dbus_string_free (&progs); - return FALSE; - } + goto out; + 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); - return FALSE; + goto out; } 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); - return FALSE; + goto out; } 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); - return FALSE; + goto out; } ++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]); - - _dbus_string_free (&progs); - return FALSE; + goto out; } - + + 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[] = diff --git a/dbus/dbus-sysdeps-util-unix.c b/dbus/dbus-sysdeps-util-unix.c index 06a790a..9d22069 100644 --- a/dbus/dbus-sysdeps-util-unix.c +++ b/dbus/dbus-sysdeps-util-unix.c @@ -1272,17 +1272,18 @@ fail: return FALSE; } -/* - * replaces the term DBUS_PREFIX in configure_time_path by the - * current dbus installation directory. On unix this function is a noop +/** + * Replace the DBUS_PREFIX in the given path, in-place, by the + * current D-Bus installation directory. On Unix this function + * does nothing, successfully. * - * @param configure_time_path - * @return real path + * @param path path to edit + * @return #FALSE on OOM */ -const char * -_dbus_replace_install_prefix (const char *configure_time_path) +dbus_bool_t +_dbus_replace_install_prefix (DBusString *path) { - return configure_time_path; + return TRUE; } #define DBUS_UNIX_STANDARD_SESSION_SERVICEDIR "/dbus-1/services" diff --git a/dbus/dbus-sysdeps-util-win.c b/dbus/dbus-sysdeps-util-win.c index 96100d4..71296fa 100644 --- a/dbus/dbus-sysdeps-util-win.c +++ b/dbus/dbus-sysdeps-util-win.c @@ -1469,73 +1469,68 @@ _dbus_command_for_pid (unsigned long pid, return FALSE; } -/* - * replaces the term DBUS_PREFIX in configure_time_path by the - * current dbus installation directory. On unix this function is a noop +/** + * Replace the DBUS_PREFIX in the given path, in-place, by the + * current D-Bus installation directory. On Unix this function + * does nothing, successfully. * - * @param configure_time_path - * @return real path + * @param path path to edit + * @return #FALSE on OOM */ -const char * -_dbus_replace_install_prefix (const char *configure_time_path) +dbus_bool_t +_dbus_replace_install_prefix (DBusString *path) { #ifndef DBUS_PREFIX - return configure_time_path; + /* leave path unchanged */ + return TRUE; #else - static char retval[1000]; - static char runtime_prefix[1000]; - int len = 1000; + DBusString runtime_prefix; int i; - if (!configure_time_path) - return NULL; - - if ((!_dbus_get_install_root(runtime_prefix, len) || - strncmp (configure_time_path, DBUS_PREFIX "/", - strlen (DBUS_PREFIX) + 1))) { - strncpy (retval, configure_time_path, sizeof (retval) - 1); - /* 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); - retval[sizeof (retval) - 1] = '\0'; - remaining = sizeof (retval) - 1 - strlen (retval); - strncat (retval, - configure_time_path + strlen (DBUS_PREFIX) + 1, - remaining); - } + if (!_dbus_string_init (&runtime_prefix)) + return FALSE; + + if (!_dbus_get_install_root (&runtime_prefix)) + { + _dbus_string_free (&runtime_prefix); + return FALSE; + } + + if (_dbus_string_get_length (&runtime_prefix) == 0) + { + /* cannot determine install root, leave path unchanged */ + _dbus_string_free (&runtime_prefix); + return TRUE; + } + + 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. * Since windows C library accepts both forward and backslashes as * path separators, convert all backslashes to forward slashes. */ - for(i = 0; retval[i] != '\0'; i++) { - if(retval[i] == '\\') - retval[i] = '/'; - } - return retval; -#endif -} - -/** - * return the relocated DATADIR - * - * @returns relocated DATADIR static string - */ + for (i = 0; i < _dbus_string_get_length (path); i++) + { + if (_dbus_string_get_byte (path, i) == '\\') + _dbus_string_set_byte (path, i, '/'); + } -static const char * -_dbus_windows_get_datadir (void) -{ - return _dbus_replace_install_prefix(DBUS_DATADIR); + return TRUE; +#endif } -#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]; - if (_dbus_get_install_root (install_root, sizeof(install_root))) - if (!_dbus_string_append (&servicedir_path, install_root)) + /* this only works because this is the first thing in the + * servicedir_path; if it wasn't, we'd have to use a temporary + * string and copy it in */ + if (!_dbus_get_install_root (&servicedir_path)) + { + _dbus_string_free (&p); goto oom; + } } + + if (!_dbus_string_append (&servicedir_path, + _dbus_string_get_const_data (&p))) + { + _dbus_string_free (&p); + goto oom; + } + + _dbus_string_free (&p); } -#endif - if (!_dbus_string_append (&servicedir_path, DBUS_DATADIR)) - goto oom; 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"); diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index 69644b0..7ddb7fd 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -2812,15 +2812,12 @@ _dbus_get_install_root_as_hash(DBusString *out) { DBusString install_path; - char path[MAX_PATH*2]; - int path_size = sizeof(path); + _dbus_string_init(&install_path); - if (!_dbus_get_install_root(path,path_size)) + if (!_dbus_get_install_root (&install_path) || + _dbus_string_get_length (&install_path) == 0) return FALSE; - _dbus_string_init(&install_path); - _dbus_string_append(&install_path,path); - _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 len length of buffer - * @returns #FALSE on failure + * @param str buffer for installation path + * @returns #FALSE on OOM, #TRUE if not OOM */ 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 - DWORD pathLength; + /* this is just an initial guess */ + DWORD pathLength = MAX_PATH; char *lastSlash; - SetLastError( 0 ); - pathLength = GetModuleFileNameA(_dbus_win_get_dll_hmodule(), prefix, len); - if ( pathLength == 0 || GetLastError() != 0 ) { - *prefix = '\0'; - return FALSE; - } + char *prefix; + + do + { + /* 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'; - return FALSE; + /* failed, but not OOM */ + _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; } diff --git a/dbus/dbus-sysdeps-win.h b/dbus/dbus-sysdeps-win.h index e9b30d7..0b5d8f0 100644 --- a/dbus/dbus-sysdeps-win.h +++ b/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); diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h index fb338df..bd0356e 100644 --- a/dbus/dbus-sysdeps.h +++ b/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); -/* - * 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); +dbus_bool_t _dbus_replace_install_prefix (DBusString *path); /* Do not set this too high: it is a denial-of-service risk. * See diff --git a/test/manual-paths.c b/test/manual-paths.c index e392c5c..c86ed6b 100644 --- a/test/manual-paths.c +++ b/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); - if (!s2) - return FALSE; + DBusString str; + + 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, s2); + fprintf(stdout, "replaced '%s' by '%s'\n", s, + _dbus_string_get_const_data (&str)); + _dbus_string_free (&str); return TRUE; } -- 2.5.3