[glib: 1/7] gdbusaddress, win32: don't rely on short names

[Philip Withnall <pwithnall src gnome org>]

commit 4ed5abda438e7f66c3765c4236cae962b7d8d1bf
Author: Vasily Galkin <galkin-vv ya ru>
Date:   Tue Jan 8 19:51:23 2019 +0300

    gdbusaddress, win32: don't rely on short names
    
    Closes: https://gitlab.gnome.org/GNOME/glib/issues/1566
    
    Short names were used in win32 implementation to allow launching
    on installations where full path to libgio-2.0-0.dll contain spaces.
    However, short names are optional on windows: so if they were disabled
    that method fails - see issue linked above.
    
    Since rundll32 doesn't support neither spaces, nor quotes in cmdline
    this patch changes rundll32 argument to just .\gio-dll-name.dll
    and uses the entire path directory containing gio dll as rundll32
    current directory.
    
    Added comments informing about potential subtleties discovered during
    writing test for gdbusaddress on win32.
    There are not known to have real-world user-visible effect,
    so by now I'm only adding comments without creating issues.

 gio/gdbusaddress.c | 68 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 13 deletions(-)
---
diff --git a/gio/gdbusaddress.c b/gio/gdbusaddress.c
index f377378ec..8b3c858ba 100644
--- a/gio/gdbusaddress.c
+++ b/gio/gdbusaddress.c
@@ -1268,6 +1268,15 @@ read_shm (const char *shm_name)
   if (shared_mem != 0)
     {
       shared_data = MapViewOfFile (shared_mem, FILE_MAP_READ, 0, 0, 0);
+      /* It looks that a race is possible here:
+       * if the dbus process already created mapping but didn't fill it
+       * the code below may read incorrect address.
+       * Also this is a bit complicated by the fact that
+       * any change in the "synchronization contract" between processes
+       * should be accompanied with renaming all of used win32 named objects:
+       * otherwise libgio-2.0-0.dll of different versions shipped with
+       * different apps may break each other due to protocol difference.
+       */
       if (shared_data != NULL)
        {
          res = g_strdup (shared_data);
@@ -1431,6 +1440,12 @@ g_win32_run_session_bus (HWND hwnd, HINSTANCE hinst, char *cmdline, int nCmdShow
       return;
     }
 
+  /* There is a subtle detail with "idle-timeout" signal of dbus daemon:
+   * It is fired on idle after last client disconnection,
+   * but (at least with glib 2.59.1) it is NEVER fired
+   * if no clients connect to daemon at all.
+   * This may lead to infinite run of this daemon process.
+   */
   g_signal_connect (daemon, "idle-timeout", G_CALLBACK (idle_timeout_cb), loop);
 
   if (publish_session_bus (_g_dbus_daemon_get_address (daemon)))
@@ -1449,7 +1464,6 @@ get_session_address_dbus_launch (GError **error)
 {
   HANDLE autolaunch_mutex, init_mutex;
   char *address = NULL;
-  wchar_t gio_path[MAX_PATH+1+200];
 
   autolaunch_mutex = acquire_mutex (DBUS_AUTOLAUNCH_MUTEX);
 
@@ -1462,18 +1476,45 @@ get_session_address_dbus_launch (GError **error)
 
   if (address == NULL)
     {
-      gio_path[MAX_PATH] = 0;
-      if (GetModuleFileNameW (_g_io_win32_get_module (), gio_path, MAX_PATH))
+      /* rundll32 doesn't support neither spaces, nor quotes in cmdline:
+       * https://support.microsoft.com/en-us/help/164787/info-windows-rundll-and-rundll32-interface
+       * Since the dll path may have spaces, it is used as working directory,
+       * and the plain dll name is passed as argument to rundll32 like
+       * "C:\Windows\System32\rundll32.exe" .\libgio-2.0-0.dll,g_win32_run_session_bus
+       *
+       * Using relative path to dll rises a question if correct dll is loaded.
+       * According to docs if relative path like .\libgio-2.0-0.dll is passed
+       * the System32 directory may be searched before current directory:
+       * 
https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
+       * Internally rundll32 uses "undefined behavior" parameter combination
+       * LoadLibraryExW(".\libgio-2.0-0.dll", NULL, LOAD_WITH_ALTERED_SEARCH_PATH)
+       * Actual testing shows that if relative name starts from ".\"
+       * the current directory is searched before System32 (win7, win10 1607).
+       * So wrong dll would be loaded only if the BOTH of the following holds:
+       * - rundll32 will change so it would prefer system32 even for .\ paths
+       * - some app would place libgio-2.0-0.dll in system32 directory
+       *
+       * In point of that using .\libgio-2.0-0.dll looks fine.
+       */
+      wchar_t gio_path[MAX_PATH + 2] = { 0 };
+      int gio_path_len = GetModuleFileNameW (_g_io_win32_get_module (), gio_path, MAX_PATH + 1);
+
+      /* The <= MAX_PATH check prevents truncated path usage */
+      if (gio_path_len > 0 && gio_path_len <= MAX_PATH)
        {
          PROCESS_INFORMATION pi = { 0 };
          STARTUPINFOW si = { 0 };
-         BOOL res;
-         wchar_t gio_path_short[MAX_PATH];
-         wchar_t rundll_path[MAX_PATH*2];
-         wchar_t args[MAX_PATH*4];
-
-         GetShortPathNameW (gio_path, gio_path_short, MAX_PATH);
-
+         BOOL res = FALSE;
+         wchar_t rundll_path[MAX_PATH + 100] = { 0 };
+         wchar_t args[MAX_PATH*2 + 100] = { 0 };
+         /* calculate index of first char of dll file name inside full path */
+         int gio_name_index = gio_path_len;
+         for (; gio_name_index > 0; --gio_name_index)
+         {
+           wchar_t prev_char = gio_path[gio_name_index - 1];
+           if (prev_char == L'\\' || prev_char == L'/')
+             break;
+         }
          GetWindowsDirectoryW (rundll_path, MAX_PATH);
          wcscat (rundll_path, L"\\rundll32.exe");
          if (GetFileAttributesW (rundll_path) == INVALID_FILE_ATTRIBUTES)
@@ -1484,8 +1525,8 @@ get_session_address_dbus_launch (GError **error)
 
          wcscpy (args, L"\"");
          wcscat (args, rundll_path);
-         wcscat (args, L"\" ");
-         wcscat (args, gio_path_short);
+         wcscat (args, L"\" .\\");
+         wcscat (args, gio_path + gio_name_index);
 #if defined(_WIN64) || defined(_M_X64) || defined(_M_AMD64)
          wcscat (args, L",g_win32_run_session_bus");
 #elif defined (_MSC_VER)
@@ -1494,10 +1535,11 @@ get_session_address_dbus_launch (GError **error)
          wcscat (args, L",g_win32_run_session_bus@16");
 #endif
 
+         gio_path[gio_name_index] = L'\0';
          res = CreateProcessW (rundll_path, args,
                                0, 0, FALSE,
                                NORMAL_PRIORITY_CLASS | CREATE_NO_WINDOW | DETACHED_PROCESS,
-                               0, NULL /* TODO: Should be root */,
+                               0, gio_path,
                                &si, &pi);
          if (res)
            address = read_shm (DBUS_DAEMON_ADDRESS_INFO);