diff options
author | wrowe <wrowe@13f79535-47bb-0310-9956-ffa450edef68> | 2007-08-28 04:43:26 +0000 |
---|---|---|
committer | wrowe <wrowe@13f79535-47bb-0310-9956-ffa450edef68> | 2007-08-28 04:43:26 +0000 |
commit | 25c97ff81cc2d5cdc3bd0dfa9ebc6c0119c10bc3 (patch) | |
tree | a6d48a53722d22ca5611f19e8e22bc69d41c346f | |
parent | 59a3afb78d7094cafce9715c2dd5473adfbb1046 (diff) | |
download | libapr-25c97ff81cc2d5cdc3bd0dfa9ebc6c0119c10bc3.tar.gz |
Solve win32 inherited pipe leaks by leveraging OS2 port's solution.
Mutex the pipe manipulation on WinNT+++ alone (not WinCE, nor 9x)
so that we toggle the inherited state of the stdin/out/err pipes.
This is only possible on NT, because in CE/9x it would involve
replacing the pipe handles all over the place as there is no toggle.
This CRITICAL_SECTION pipe is incredibly fast in the mainline case,
and only introduces contention in the threaded server after startup
(for cgi, etc). Not unlike an in-process cgid.
So, leave WinCE alone for now, since it doesn't follow the stdio model,
and leave Win9x alone for good, as nearly abandoned.
Backport: r569882 (+r569890)
git-svn-id: http://svn.apache.org/repos/asf/apr/apr/branches/1.2.x@570303 13f79535-47bb-0310-9956-ffa450edef68
-rw-r--r-- | CHANGES | 6 | ||||
-rw-r--r-- | file_io/win32/pipe.c | 10 | ||||
-rw-r--r-- | include/arch/win32/apr_arch_inherit.h | 98 | ||||
-rw-r--r-- | include/arch/win32/apr_arch_threadproc.h | 2 | ||||
-rw-r--r-- | misc/win32/start.c | 5 | ||||
-rw-r--r-- | threadproc/win32/proc.c | 130 |
6 files changed, 201 insertions, 50 deletions
@@ -1,6 +1,12 @@ -*- coding: utf-8 -*- Changes for APR 1.2.10 + *) Solve winNT inherited pipe leaks by mutexing apr_proc_create calls, + on WinNT (not WinCE, nor 9x) so that we toggle the inherited state + of the stdin/out/err pipes. All other file handles are treated as + not-inherited until apr_file_dup2'ed a std handle of this process, + or while they are used by apr_proc_create. [William Rowe] + *) Define the Mac OS/X filesystem_encoding as utf-8 (in previous releases the interpretation would vary). [Branko Čibej] diff --git a/file_io/win32/pipe.c b/file_io/win32/pipe.c index cda79e3af..dd00f06cb 100644 --- a/file_io/win32/pipe.c +++ b/file_io/win32/pipe.c @@ -95,7 +95,15 @@ apr_status_t apr_create_nt_pipe(apr_file_t **in, apr_file_t **out, char name[50]; sa.nLength = sizeof(sa); - sa.bInheritHandle = TRUE; + +#if APR_HAS_UNICODE_FS + IF_WIN_OS_IS_UNICODE + sa.bInheritHandle = FALSE; +#endif +#if APR_HAS_ANSI_FS + ELSE_WIN_OS_IS_ANSI + sa.bInheritHandle = TRUE; +#endif sa.lpSecurityDescriptor = NULL; (*in) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t)); diff --git a/include/arch/win32/apr_arch_inherit.h b/include/arch/win32/apr_arch_inherit.h index d70d5972c..8969af664 100644 --- a/include/arch/win32/apr_arch_inherit.h +++ b/include/arch/win32/apr_arch_inherit.h @@ -21,43 +21,19 @@ #define APR_INHERIT (1 << 24) /* Must not conflict with other bits */ -#if defined(_WIN32_WCE) -#define APR_IMPLEMENT_INHERIT_SET(name, flag, pool, cleanup) \ -APR_DECLARE(apr_status_t) apr_##name##_inherit_set(apr_##name##_t *the##name) \ -{ \ - HANDLE temp, hproc = GetCurrentProcess(); \ - if (!DuplicateHandle(hproc, the##name->filehand, \ - hproc, &temp, 0, TRUE, \ - DUPLICATE_SAME_ACCESS)) \ - return apr_get_os_error(); \ - CloseHandle(the##name->filehand); \ - the##name->filehand = temp; \ - return APR_SUCCESS; \ -} +#if APR_HAS_UNICODE_FS && APR_HAS_ANSI_FS +/* !defined(_WIN32_WCE) is implicit here */ -#define APR_IMPLEMENT_INHERIT_UNSET(name, flag, pool, cleanup) \ -APR_DECLARE(apr_status_t) apr_##name##_inherit_unset(apr_##name##_t *the##name)\ -{ \ - HANDLE temp, hproc = GetCurrentProcess(); \ - if (!DuplicateHandle(hproc, the##name->filehand, \ - hproc, &temp, 0, FALSE, \ - DUPLICATE_SAME_ACCESS)) \ - return apr_get_os_error(); \ - CloseHandle(the##name->filehand); \ - the##name->filehand = temp; \ - return APR_SUCCESS; \ -} -#else #define APR_IMPLEMENT_INHERIT_SET(name, flag, pool, cleanup) \ APR_DECLARE(apr_status_t) apr_##name##_inherit_set(apr_##name##_t *the##name) \ { \ IF_WIN_OS_IS_UNICODE \ { \ - if (!SetHandleInformation(the##name->filehand, \ - HANDLE_FLAG_INHERIT, \ - HANDLE_FLAG_INHERIT)) \ - return apr_get_os_error(); \ - } \ +/* if (!SetHandleInformation(the##name->filehand, \ + * HANDLE_FLAG_INHERIT, \ + * HANDLE_FLAG_INHERIT)) \ + * return apr_get_os_error(); \ + */ } \ ELSE_WIN_OS_IS_ANSI \ { \ HANDLE temp, hproc = GetCurrentProcess(); \ @@ -76,10 +52,10 @@ APR_DECLARE(apr_status_t) apr_##name##_inherit_unset(apr_##name##_t *the##name)\ { \ IF_WIN_OS_IS_UNICODE \ { \ - if (!SetHandleInformation(the##name->filehand, \ - HANDLE_FLAG_INHERIT, 0)) \ - return apr_get_os_error(); \ - } \ +/* if (!SetHandleInformation(the##name->filehand, \ + * HANDLE_FLAG_INHERIT, 0)) \ + * return apr_get_os_error(); \ + */ } \ ELSE_WIN_OS_IS_ANSI \ { \ HANDLE temp, hproc = GetCurrentProcess(); \ @@ -92,6 +68,56 @@ APR_DECLARE(apr_status_t) apr_##name##_inherit_unset(apr_##name##_t *the##name)\ } \ return APR_SUCCESS; \ } -#endif + +#elif APR_HAS_ANSI_FS || defined(_WIN32_WCE) + +#define APR_IMPLEMENT_INHERIT_SET(name, flag, pool, cleanup) \ +APR_DECLARE(apr_status_t) apr_##name##_inherit_set(apr_##name##_t *the##name) \ +{ \ + HANDLE temp, hproc = GetCurrentProcess(); \ + if (!DuplicateHandle(hproc, the##name->filehand, \ + hproc, &temp, 0, TRUE, \ + DUPLICATE_SAME_ACCESS)) \ + return apr_get_os_error(); \ + CloseHandle(the##name->filehand); \ + the##name->filehand = temp; \ + return APR_SUCCESS; \ +} + +#define APR_IMPLEMENT_INHERIT_UNSET(name, flag, pool, cleanup) \ +APR_DECLARE(apr_status_t) apr_##name##_inherit_unset(apr_##name##_t *the##name)\ +{ \ + HANDLE temp, hproc = GetCurrentProcess(); \ + if (!DuplicateHandle(hproc, the##name->filehand, \ + hproc, &temp, 0, FALSE, \ + DUPLICATE_SAME_ACCESS)) \ + return apr_get_os_error(); \ + CloseHandle(the##name->filehand); \ + the##name->filehand = temp; \ + return APR_SUCCESS; \ +} + +#else /* APR_HAS_UNICODE_FS && !APR_HAS_ANSI_FS && !defined(_WIN32_WCE) */ + +#define APR_IMPLEMENT_INHERIT_SET(name, flag, pool, cleanup) \ +APR_DECLARE(apr_status_t) apr_##name##_inherit_set(apr_##name##_t *the##name) \ +{ \ +/* if (!SetHandleInformation(the##name->filehand, \ + * HANDLE_FLAG_INHERIT, \ + * HANDLE_FLAG_INHERIT)) \ + * return apr_get_os_error(); \ + */ return APR_SUCCESS; \ +} + +#define APR_IMPLEMENT_INHERIT_UNSET(name, flag, pool, cleanup) \ +APR_DECLARE(apr_status_t) apr_##name##_inherit_unset(apr_##name##_t *the##name)\ +{ \ +/* if (!SetHandleInformation(the##name->filehand, \ + * HANDLE_FLAG_INHERIT, 0)) \ + * return apr_get_os_error(); \ + */ return APR_SUCCESS; \ +} + +#endif /* defined(APR_HAS_UNICODE_FS) */ #endif /* ! INHERIT_H */ diff --git a/include/arch/win32/apr_arch_threadproc.h b/include/arch/win32/apr_arch_threadproc.h index 7af8ab686..d3ce9c518 100644 --- a/include/arch/win32/apr_arch_threadproc.h +++ b/include/arch/win32/apr_arch_threadproc.h @@ -68,5 +68,7 @@ struct apr_thread_once_t { long value; }; +extern apr_status_t apr_threadproc_init(apr_pool_t *pool); + #endif /* ! THREAD_PROC_H */ diff --git a/misc/win32/start.c b/misc/win32/start.c index b40b49e30..98b5ddcab 100644 --- a/misc/win32/start.c +++ b/misc/win32/start.c @@ -22,7 +22,8 @@ #include "apr_arch_misc.h" /* for WSAHighByte / WSALowByte */ #include "wchar.h" -#include "apr_arch_file_io.h" +#include "apr_arch_file_io.h" /* bring in unicode-ness */ +#include "apr_arch_threadproc.h" /* bring in apr_threadproc_init */ #include "crtdbg.h" #include "assert.h" @@ -205,6 +206,8 @@ APR_DECLARE(apr_status_t) apr_initialize(void) apr_signal_init(pool); + apr_threadproc_init(pool); + return APR_SUCCESS; } diff --git a/threadproc/win32/proc.c b/threadproc/win32/proc.c index 5e70c70cd..c1784d606 100644 --- a/threadproc/win32/proc.c +++ b/threadproc/win32/proc.c @@ -300,7 +300,7 @@ APR_DECLARE(apr_status_t) apr_procattr_user_set(apr_procattr_t *attr, attr->sa = apr_palloc(attr->pool, sizeof(SECURITY_ATTRIBUTES)); attr->sa->nLength = sizeof (SECURITY_ATTRIBUTES); attr->sa->lpSecurityDescriptor = attr->sd; - attr->sa->bInheritHandle = TRUE; + attr->sa->bInheritHandle = FALSE; /* register the cleanup */ apr_pool_cleanup_register(attr->pool, (void *)attr, @@ -383,6 +383,47 @@ APR_DECLARE(apr_status_t) apr_procattr_addrspace_set(apr_procattr_t *attr, return APR_SUCCESS; } +#if APR_HAS_UNICODE_FS && !defined(_WIN32_WCE) + +/* Used only for the NT code path, a critical section is the fastest + * implementation available. + */ +static CRITICAL_SECTION proc_lock; + +static apr_status_t threadproc_global_cleanup(void *ignored) +{ + DeleteCriticalSection(&proc_lock); + return APR_SUCCESS; +} + +/* Called from apr_initialize, we need a critical section to handle + * the pipe inheritance on win32. This will mutex any process create + * so as we change our inherited pipes, we prevent another process from + * also inheriting those alternate handles, and prevent the other process + * from failing to inherit our standard handles. + */ +apr_status_t apr_threadproc_init(apr_pool_t *pool) +{ + IF_WIN_OS_IS_UNICODE + { + InitializeCriticalSection(&proc_lock); + /* register the cleanup */ + apr_pool_cleanup_register(pool, &proc_lock, + threadproc_global_cleanup, + apr_pool_cleanup_null); + } + return APR_SUCCESS; +} + +#else /* !APR_HAS_UNICODE_FS || defined(_WIN32_WCE) */ + +apr_status_t apr_threadproc_init(apr_pool_t *pool) +{ + return APR_SUCCESS; +} + +#endif + APR_DECLARE(apr_status_t) apr_proc_create(apr_proc_t *new, const char *progname, const char * const *args, @@ -657,6 +698,9 @@ APR_DECLARE(apr_status_t) apr_proc_create(apr_proc_t *new, IF_WIN_OS_IS_UNICODE { STARTUPINFOW si; + DWORD stdin_reset = 0; + DWORD stdout_reset = 0; + DWORD stderr_reset = 0; apr_wchar_t *wprg = NULL; apr_wchar_t *wcmd = NULL; apr_wchar_t *wcwd = NULL; @@ -720,25 +764,65 @@ APR_DECLARE(apr_status_t) apr_proc_create(apr_proc_t *new, } #ifndef _WIN32_WCE + /* LOCK CRITICAL SECTION + * before we begin to manipulate the inherited handles + */ + EnterCriticalSection(&proc_lock); + if ((attr->child_in && attr->child_in->filehand) || (attr->child_out && attr->child_out->filehand) || (attr->child_err && attr->child_err->filehand)) { si.dwFlags |= STARTF_USESTDHANDLES; - si.hStdInput = (attr->child_in) - ? attr->child_in->filehand - : GetStdHandle(STD_INPUT_HANDLE); - - si.hStdOutput = (attr->child_out) - ? attr->child_out->filehand - : GetStdHandle(STD_OUTPUT_HANDLE); + si.hStdInput = GetStdHandle(STD_INPUT_HANDLE); + if (attr->child_in && attr->child_in->filehand) + { + if (GetHandleInformation(si.hStdInput, + &stdin_reset) + && (stdin_reset &= HANDLE_FLAG_INHERIT)) + SetHandleInformation(si.hStdInput, + HANDLE_FLAG_INHERIT, 0); + + si.hStdInput = attr->child_in->filehand; + SetHandleInformation(si.hStdInput, HANDLE_FLAG_INHERIT, + HANDLE_FLAG_INHERIT); + } + + si.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE); + if (attr->child_out && attr->child_out->filehand) + { + if (GetHandleInformation(si.hStdOutput, + &stdout_reset) + && (stdout_reset &= HANDLE_FLAG_INHERIT)) + SetHandleInformation(si.hStdOutput, + HANDLE_FLAG_INHERIT, 0); + + si.hStdOutput = attr->child_out->filehand; + SetHandleInformation(si.hStdInput, HANDLE_FLAG_INHERIT, + HANDLE_FLAG_INHERIT); + } - si.hStdError = (attr->child_err) - ? attr->child_err->filehand - : GetStdHandle(STD_ERROR_HANDLE); + si.hStdError = GetStdHandle(STD_ERROR_HANDLE); + if (attr->child_err && attr->child_err->filehand) + { + if (GetHandleInformation(si.hStdError, + &stderr_reset) + && (stderr_reset &= HANDLE_FLAG_INHERIT)) + SetHandleInformation(si.hStdError, + HANDLE_FLAG_INHERIT, 0); + + si.hStdError = attr->child_err->filehand; + SetHandleInformation(si.hStdError, HANDLE_FLAG_INHERIT, + HANDLE_FLAG_INHERIT); + } } if (attr->user_token) { + /* XXX: for terminal services, handles can't be cannot be + * inherited across sessions. This process must be created + * in our existing session. lpDesktop assignment appears + * to be wrong according to these rules. + */ si.lpDesktop = L"Winsta0\\Default"; if (!ImpersonateLoggedOnUser(attr->user_token)) { /* failed to impersonate the logged user */ @@ -768,7 +852,29 @@ APR_DECLARE(apr_status_t) apr_proc_create(apr_proc_t *new, wcwd, /* Current directory name */ &si, &pi); } -#else + + if ((attr->child_in && attr->child_in->filehand) + || (attr->child_out && attr->child_out->filehand) + || (attr->child_err && attr->child_err->filehand)) + { + if (stdin_reset) + SetHandleInformation(GetStdHandle(STD_INPUT_HANDLE), + stdin_reset, stdin_reset); + + if (stdout_reset) + SetHandleInformation(GetStdHandle(STD_OUTPUT_HANDLE), + stdout_reset, stdout_reset); + + if (stderr_reset) + SetHandleInformation(GetStdHandle(STD_ERROR_HANDLE), + stderr_reset, stderr_reset); + } + /* RELEASE CRITICAL SECTION + * The state of the inherited handles has been restored. + */ + LeaveCriticalSection(&proc_lock); + +#else /* defined(_WIN32_WCE) */ rv = CreateProcessW(wprg, wcmd, /* Executable & Command line */ NULL, NULL, /* Proc & thread security attributes */ FALSE, /* must be 0 */ |