glib r6575 - in trunk: . glib

[tml svn gnome org]

Author: tml
Date: Sun Feb 24 21:15:47 2008
New Revision: 6575
URL: http://svn.gnome.org/viewvc/glib?rev=6575&view=rev

Log:
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.



Modified:
   trunk/ChangeLog
   trunk/glib/gspawn-win32-helper.c
   trunk/glib/gspawn-win32.c
   trunk/glib/gutils.c

Modified: trunk/glib/gspawn-win32-helper.c
==============================================================================
--- trunk/glib/gspawn-win32-helper.c	(original)
+++ trunk/glib/gspawn-win32-helper.c	Sun Feb 24 21:15:47 2008
@@ -21,6 +21,8 @@
 
 #include "config.h"
 
+#include <fcntl.h>
+
 #undef G_LOG_DOMAIN
 #include "glib.h"
 #define GSPAWN_HELPER
@@ -151,7 +153,8 @@
 	 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 @@
   int argc;
   wchar_t **wargv, **wenvp;
   _startupinfo si = { 0 };
+  char c;
 
   g_assert (__argc >= ARG_COUNT);
 
@@ -188,6 +192,14 @@
   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 @@
    */
   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 @@
     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;
 }

Modified: trunk/glib/gspawn-win32.c
==============================================================================
--- trunk/glib/gspawn-win32.c	(original)
+++ trunk/glib/gspawn-win32.c	Sun Feb 24 21:15:47 2008
@@ -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
@@ -59,14 +59,7 @@
 #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 {
   ARG_CHILD_ERR_REPORT = 1,
+  ARG_HELPER_SYNC,
   ARG_STDIN,
   ARG_STDOUT,
   ARG_STDERR,
@@ -107,6 +101,19 @@
   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 @@
   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 @@
   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 @@
   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 @@
       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 @@
   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 @@
 	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 @@
       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 @@
       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 @@
    * 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 @@
        */
       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 @@
       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 @@
 	      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 @@
     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)

Modified: trunk/glib/gutils.c
==============================================================================
--- trunk/glib/gutils.c	(original)
+++ trunk/glib/gutils.c	Sun Feb 24 21:15:47 2008
@@ -146,6 +146,15 @@
   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
 
 /**