New internal function.
authorTor Lillqvist <tml@novell.com>
Sun, 24 Feb 2008 21:15:47 +0000 (21:15 +0000)
committerTor Lillqvist <tml@src.gnome.org>
Sun, 24 Feb 2008 21:15:47 +0000 (21:15 +0000)
2008-02-24  Tor Lillqvist  <tml@novell.com>

* glib/gutils.c (_glib_get_installation_directory): New internal function.

* glib/gspawn-win32.c: When spawning the helper process, use an
explicit full path. (#518292)

* glib/gspawn-win32.c
* glib/gspawn-win32-helper.c: Fix race condition when using the
helper process: When the helper process writes the handle of the
actual started user process to the parent process, it must be
duplicated in the parent process with DuplicateHandle() so that it
is a valid handle in that process. However, if the helper process
has happened to exit before the DuplicateHandle() call, the
duplication will fail. Thus we must synchronise the helper
process's exit. Use another pipe for this.

Take care not to inherit the writing end of this pipe to the
helper process. Also, in the helper process, take care not to
inherit either of the pipes used for communication with the parent
process to the started user process.

svn path=/trunk/; revision=6575

ChangeLog
glib/gspawn-win32-helper.c
glib/gspawn-win32.c
glib/gutils.c

index 27a1ae3..794ba71 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,27 @@
 2008-02-24  Tor Lillqvist  <tml@novell.com>
 
+       * glib/gutils.c (_glib_get_installation_directory): New internal function.
+
+       * glib/gspawn-win32.c: When spawning the helper process, use an
+       explicit full path. (#518292)
+
+       * glib/gspawn-win32.c
+       * glib/gspawn-win32-helper.c: Fix race condition when using the
+       helper process: When the helper process writes the handle of the
+       actual started user process to the parent process, it must be
+       duplicated in the parent process with DuplicateHandle() so that it
+       is a valid handle in that process. However, if the helper process
+       has happened to exit before the DuplicateHandle() call, the
+       duplication will fail. Thus we must synchronise the helper
+       process's exit. Use another pipe for this.
+
+       Take care not to inherit the writing end of this pipe to the
+       helper process. Also, in the helper process, take care not to
+       inherit either of the pipes used for communication with the parent
+       process to the started user process.
+
+2008-02-24  Tor Lillqvist  <tml@novell.com>
+
        * glib/gmain.c (g_poll) [Win32]: Use alertable wait functions so
        that I/O completion routines or user-mode Asynchronous Procedure
        Calls can be run. (#517484, Vlad Grecescu)
index c10a59a..1a717ed 100644 (file)
@@ -21,6 +21,8 @@
 
 #include "config.h"
 
+#include <fcntl.h>
+
 #undef G_LOG_DOMAIN
 #include "glib.h"
 #define GSPAWN_HELPER
@@ -151,7 +153,8 @@ WinMain (struct HINSTANCE__ *hInstance,
         char               *lpszCmdLine,
         int                 nCmdShow)
 {
-  int child_err_report_fd;
+  int child_err_report_fd = -1;
+  int helper_sync_fd = -1;
   int i;
   int fd;
   int mode;
@@ -164,6 +167,7 @@ WinMain (struct HINSTANCE__ *hInstance,
   int argc;
   wchar_t **wargv, **wenvp;
   _startupinfo si = { 0 };
+  char c;
 
   g_assert (__argc >= ARG_COUNT);
 
@@ -188,6 +192,14 @@ WinMain (struct HINSTANCE__ *hInstance,
   if (__argv[ARG_CHILD_ERR_REPORT][strlen (__argv[ARG_CHILD_ERR_REPORT]) - 1] == '#')
     argv_zero_offset++;
 
+  /* argv[ARG_HELPER_SYNC] is the file descriptor number we read a
+   * byte that tells us it is OK to exit. We have to wait until the
+   * parent allows us to exit, so that the parent has had time to
+   * duplicate the process handle we sent it. Duplicating a handle
+   * from another process works only if that other process exists.
+   */
+  helper_sync_fd = atoi (__argv[ARG_HELPER_SYNC]);
+
   /* argv[ARG_STDIN..ARG_STDERR] are the file descriptor numbers that
    * should be dup2'd to 0, 1 and 2. '-' if the corresponding fd
    * should be left alone, and 'z' if it should be connected to the
@@ -270,9 +282,15 @@ WinMain (struct HINSTANCE__ *hInstance,
    */
   if (__argv[ARG_CLOSE_DESCRIPTORS][0] == 'y')
     for (i = 3; i < 1000; i++) /* FIXME real limit? */
-      if (i != child_err_report_fd)
+      if (i != child_err_report_fd && i != helper_sync_fd)
        close (i);
 
+  /* We don't want our child to inherit the error report and
+   * helper sync fds.
+   */
+  child_err_report_fd = dup_noninherited (child_err_report_fd, _O_WRONLY);
+  helper_sync_fd = dup_noninherited (helper_sync_fd, _O_RDONLY);
+
   /* __argv[ARG_WAIT] is "w" to wait for the program to exit */
   if (__argv[ARG_WAIT][0] == 'w')
     mode = P_WAIT;
@@ -307,5 +325,8 @@ WinMain (struct HINSTANCE__ *hInstance,
     write (child_err_report_fd, &handle, sizeof (handle));
   else
     write (child_err_report_fd, &zero, sizeof (zero));
+
+  read (helper_sync_fd, &c, 1);
+
   return 0;
 }
index 297c9de..dded807 100644 (file)
@@ -30,7 +30,7 @@
  *   before starting the child process. (There might be several threads
  *   running, and the current directory is common for all threads.)
  *
- * Thus, we must in most cases use a helper program to handle closing
+ * Thus, we must in many cases use a helper program to handle closing
  * of (inherited) file descriptors and changing of directory. The
  * helper process is also needed if the standard input, standard
  * output, or standard error of the process to be run are supposed to
 #include <io.h>
 #include <process.h>
 #include <direct.h>
-
-#ifdef __MINGW32__
-/* Mingw doesn't have prototypes for these */
-int _wspawnvpe (int, const wchar_t *, const wchar_t **, const wchar_t **);
-int _wspawnvp (int, const wchar_t *, const wchar_t **);
-int _wspawnve (int, const wchar_t *, const wchar_t **, const wchar_t **);
-int _wspawnv (int, const wchar_t *, const wchar_t **);
-#endif
+#include <wchar.h>
 
 #ifdef G_SPAWN_WIN32_DEBUG
   static int debug = 1;
@@ -96,6 +89,7 @@ enum
 
 enum {
   ARG_CHILD_ERR_REPORT = 1,
+  ARG_HELPER_SYNC,
   ARG_STDIN,
   ARG_STDOUT,
   ARG_STDERR,
@@ -107,6 +101,19 @@ enum {
   ARG_COUNT = ARG_PROGRAM
 };
 
+static int
+dup_noninherited (int fd,
+                 int mode)
+{
+  HANDLE filehandle;
+
+  DuplicateHandle (GetCurrentProcess (), (LPHANDLE) _get_osfhandle (fd),
+                  GetCurrentProcess (), &filehandle,
+                  0, FALSE, DUPLICATE_SAME_ACCESS);
+  close (fd);
+  return _open_osfhandle ((long) filehandle, mode | _O_NOINHERIT);
+}
+
 #ifndef GSPAWN_HELPER
 
 #define HELPER_PROCESS "gspawn-win32-helper"
@@ -527,6 +534,7 @@ do_spawn_with_pipes (gint                 *exit_status,
   int stdout_pipe[2] = { -1, -1 };
   int stderr_pipe[2] = { -1, -1 };
   int child_err_report_pipe[2] = { -1, -1 };
+  int helper_sync_pipe[2] = { -1, -1 };
   int helper_report[2];
   static gboolean warned_about_child_setup = FALSE;
   GError *conv_error = NULL;
@@ -534,8 +542,8 @@ do_spawn_with_pipes (gint                 *exit_status,
   gchar *helper_process;
   CONSOLE_CURSOR_INFO cursor_info;
   wchar_t *whelper, **wargv, **wenvp;
-  
-  SETUP_DEBUG();
+  extern gchar *_glib_get_installation_directory (void);
+  gchar *glib_top;
 
   if (child_setup && !warned_about_child_setup)
     {
@@ -574,15 +582,31 @@ do_spawn_with_pipes (gint                 *exit_status,
   if (!make_pipe (child_err_report_pipe, error))
     goto cleanup_and_fail;
   
+  if (!make_pipe (helper_sync_pipe, error))
+    goto cleanup_and_fail;
+  
   new_argv = g_new (char *, argc + 1 + ARG_COUNT);
   if (GetConsoleCursorInfo (GetStdHandle (STD_OUTPUT_HANDLE), &cursor_info))
     helper_process = HELPER_PROCESS "-console.exe";
   else
     helper_process = HELPER_PROCESS ".exe";
+  
+  glib_top = _glib_get_installation_directory ();
+  helper_process = g_build_filename (glib_top, "bin", helper_process, NULL);
+  g_free (glib_top);
+
   new_argv[0] = helper_process;
+
   _g_sprintf (args[ARG_CHILD_ERR_REPORT], "%d", child_err_report_pipe[1]);
   new_argv[ARG_CHILD_ERR_REPORT] = args[ARG_CHILD_ERR_REPORT];
   
+  /* Make the read end of the child error report pipe
+   * noninherited. Otherwise it will needlessly be inherited by the
+   * helper process, and the started actual user process. As such that
+   * shouldn't harm, but it is unnecessary.
+   */
+  child_err_report_pipe[0] = dup_noninherited (child_err_report_pipe[0], _O_RDONLY);
+
   if (flags & G_SPAWN_FILE_AND_ARGV_ZERO)
     {
       /* Overload ARG_CHILD_ERR_REPORT to also encode the
@@ -591,6 +615,17 @@ do_spawn_with_pipes (gint                 *exit_status,
       strcat (args[ARG_CHILD_ERR_REPORT], "#");
     }
   
+  _g_sprintf (args[ARG_HELPER_SYNC], "%d", helper_sync_pipe[0]);
+  new_argv[ARG_HELPER_SYNC] = args[ARG_HELPER_SYNC];
+  
+  /* Make the write end of the sync pipe noninherited. Otherwise the
+   * helper process will inherit it, and thus if this process happens
+   * to crash before writing the sync byte to the pipe, the helper
+   * process won't read but won't get any EOF either, as it has the
+   * write end open itself.
+   */
+  helper_sync_pipe[1] = dup_noninherited (helper_sync_pipe[1], _O_WRONLY);
+
   if (standard_input)
     {
       _g_sprintf (args[ARG_STDIN], "%d", stdin_pipe[0]);
@@ -658,6 +693,8 @@ do_spawn_with_pipes (gint                 *exit_status,
   for (i = 0; i <= argc; i++)
     new_argv[ARG_PROGRAM + i] = protected_argv[i];
 
+  SETUP_DEBUG();
+
   if (debug)
     {
       g_print ("calling %s with argv:\n", helper_process);
@@ -665,8 +702,6 @@ do_spawn_with_pipes (gint                 *exit_status,
        g_print ("argv[%d]: %s\n", i, (new_argv[i] ? new_argv[i] : "NULL"));
     }
 
-  whelper = g_utf8_to_utf16 (helper_process, -1, NULL, NULL, NULL);
-
   if (!utf8_charv_to_wcharv (new_argv, &wargv, &conv_error_index, &conv_error))
     {
       if (conv_error_index == ARG_WORKING_DIRECTORY)
@@ -681,9 +716,9 @@ do_spawn_with_pipes (gint                 *exit_status,
       g_strfreev (protected_argv);
       g_free (new_argv[ARG_WORKING_DIRECTORY]);
       g_free (new_argv);
-      g_free (whelper);
+      g_free (helper_process);
 
-      return FALSE;
+      goto cleanup_and_fail;
     }
 
   if (!utf8_charv_to_wcharv (envp, &wenvp, NULL, &conv_error))
@@ -695,15 +730,18 @@ do_spawn_with_pipes (gint                 *exit_status,
       g_strfreev (protected_argv);
       g_free (new_argv[ARG_WORKING_DIRECTORY]);
       g_free (new_argv);
-      g_free (whelper);
+      g_free (helper_process);
       g_strfreev ((gchar **) wargv);
  
-      return FALSE;
+      goto cleanup_and_fail;
     }
 
   if (child_setup)
     (* child_setup) (user_data);
 
+  whelper = g_utf8_to_utf16 (helper_process, -1, NULL, NULL, NULL);
+  g_free (helper_process);
+
   if (wenvp != NULL)
     /* Let's hope envp hasn't mucked with PATH so that
      * gspawn-win32-helper.exe isn't found.
@@ -722,6 +760,7 @@ do_spawn_with_pipes (gint                 *exit_status,
    * otherwise the reader will never get EOF.
    */
   close_and_invalidate (&child_err_report_pipe[1]);
+  close_and_invalidate (&helper_sync_pipe[0]);
   close_and_invalidate (&stdin_pipe[0]);
   close_and_invalidate (&stdout_pipe[1]);
   close_and_invalidate (&stderr_pipe[1]);
@@ -748,6 +787,8 @@ do_spawn_with_pipes (gint                 *exit_status,
        */
       g_assert (err_report != NULL);
       *err_report = child_err_report_pipe[0];
+      write (helper_sync_pipe[1], " ", 1);
+      close_and_invalidate (&helper_sync_pipe[1]);
     }
   else
     {
@@ -755,7 +796,7 @@ do_spawn_with_pipes (gint                 *exit_status,
       if (!read_helper_report (child_err_report_pipe[0], helper_report, error))
        goto cleanup_and_fail;
         
-      close (child_err_report_pipe[0]);
+      close_and_invalidate (&child_err_report_pipe[0]);
 
       switch (helper_report[0])
        {
@@ -769,13 +810,21 @@ do_spawn_with_pipes (gint                 *exit_status,
              if (!DuplicateHandle ((HANDLE) rc, (HANDLE) helper_report[1],
                                    GetCurrentProcess (), (LPHANDLE) child_handle,
                                    0, TRUE, DUPLICATE_SAME_ACCESS))
-               *child_handle = 0;
+               {
+                 char *emsg = g_win32_error_message (GetLastError ());
+                 g_print("%s\n", emsg);
+                 *child_handle = 0;
+               }
            }
          else if (child_handle)
            *child_handle = 0;
+         write (helper_sync_pipe[1], " ", 1);
+         close_and_invalidate (&helper_sync_pipe[1]);
          break;
          
        default:
+         write (helper_sync_pipe[1], " ", 1);
+         close_and_invalidate (&helper_sync_pipe[1]);
          set_child_error (helper_report, working_directory, error);
          goto cleanup_and_fail;
        }
@@ -801,6 +850,10 @@ do_spawn_with_pipes (gint                 *exit_status,
     close (child_err_report_pipe[0]);
   if (child_err_report_pipe[1] != -1)
     close (child_err_report_pipe[1]);
+  if (helper_sync_pipe[0] != -1)
+    close (helper_sync_pipe[0]);
+  if (helper_sync_pipe[1] != -1)
+    close (helper_sync_pipe[1]);
   if (stdin_pipe[0] != -1)
     close (stdin_pipe[0]);
   if (stdin_pipe[1] != -1)
index 99c65f8..f9f6e07 100644 (file)
@@ -146,6 +146,15 @@ DllMain (HINSTANCE hinstDLL,
   return TRUE;
 }
 
+gchar *
+_glib_get_installation_directory (void)
+{
+  if (glib_dll == NULL)
+    return NULL;
+
+  return g_win32_get_package_installation_directory_of_module (glib_dll);
+}
+
 #endif
 
 /**