Commit 17640e78 authored by Tor Lillqvist's avatar Tor Lillqvist Committed by Tor Lillqvist

New internal function.

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
parent 3bd51264
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
......
......@@ -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;
}
......@@ -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
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)
......
......@@ -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
/**
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment