diff --git a/ChangeLog b/ChangeLog index 27a1ae3ea..794ba71f5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,25 @@ +2008-02-24 Tor Lillqvist + + * 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 * glib/gmain.c (g_poll) [Win32]: Use alertable wait functions so diff --git a/glib/gspawn-win32-helper.c b/glib/gspawn-win32-helper.c index c10a59a56..1a717edc2 100644 --- a/glib/gspawn-win32-helper.c +++ b/glib/gspawn-win32-helper.c @@ -21,6 +21,8 @@ #include "config.h" +#include + #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; } diff --git a/glib/gspawn-win32.c b/glib/gspawn-win32.c index 297c9de12..dded80718 100644 --- a/glib/gspawn-win32.c +++ b/glib/gspawn-win32.c @@ -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 #include #include - -#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 #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) diff --git a/glib/gutils.c b/glib/gutils.c index 99c65f8b4..f9f6e072c 100644 --- a/glib/gutils.c +++ b/glib/gutils.c @@ -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 /**