Here's my proposal of how to solve leaking pipes. Simply, if we kick off a process without inherited handles, we don't inherit stdin/out/err pipes. Once we have those pipes, inheriting other arbitrary handles is not effective; if you look at the mpm_winnt you'll see I kick that process off (suspended) and then pass a new inherited handle one-by-one from the parent to the child, sending those handle identifiers over the stdin pipe (not unlike a unix domain socket). Ryan's and my early attempts to leverage inherited handles proved futile.
My concern is how others use the apr_proc_create() API, before I go ahead and backport this to 1.2/0.9 for the new releases. If you want to review those backports, I've created them at http://people.apache.org/~wrowe/r569882-r569890-backport-0.9.patch http://people.apache.org/~wrowe/r569882-r569890-backport-1.2.patch If folks would comment, I'd appreciate it. The change definately merits some discussion. So +/-1, or comment, or question away. I hinted at why this doesn't apply to 9x; to 'turn off' the inheritance of a default stdout, we have to dup it inherited, and then dup it back inherited. The resulting handle identifier is different than the original, which means we have broken some apr_file_t objects along the way. With the long-ago demise of 9x, I don't have any qualms about leaving the old behavior as-is, and droping out Win9x entirely by APR 2.0. Bill [EMAIL PROTECTED] wrote: > Author: wrowe > Date: Sun Aug 26 14:19:55 2007 > New Revision: 569882 > > URL: http://svn.apache.org/viewvc?rev=569882&view=rev > Log: > Proposed; > > 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. > > > > > > Modified: > apr/apr/trunk/file_io/win32/pipe.c > apr/apr/trunk/include/arch/win32/apr_arch_inherit.h > apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h > apr/apr/trunk/misc/win32/start.c > apr/apr/trunk/threadproc/win32/proc.c > > Modified: apr/apr/trunk/file_io/win32/pipe.c > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/file_io/win32/pipe.c?rev=569882&r1=569881&r2=569882&view=diff > ============================================================================== > --- apr/apr/trunk/file_io/win32/pipe.c (original) > +++ apr/apr/trunk/file_io/win32/pipe.c Sun Aug 26 14:19:55 2007 > @@ -95,7 +95,15 @@ > 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)); > > Modified: apr/apr/trunk/include/arch/win32/apr_arch_inherit.h > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/include/arch/win32/apr_arch_inherit.h?rev=569882&r1=569881&r2=569882&view=diff > ============================================================================== > --- apr/apr/trunk/include/arch/win32/apr_arch_inherit.h (original) > +++ apr/apr/trunk/include/arch/win32/apr_arch_inherit.h Sun Aug 26 14:19:55 > 2007 > @@ -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 @@ > { \ > 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 @@ > } \ > 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 */ > > Modified: apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h?rev=569882&r1=569881&r2=569882&view=diff > ============================================================================== > --- apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h (original) > +++ apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h Sun Aug 26 > 14:19:55 2007 > @@ -68,5 +68,7 @@ > long value; > }; > > +extern apr_status_t apr_threadproc_init(apr_pool_t *pool); > + > #endif /* ! THREAD_PROC_H */ > > > Modified: apr/apr/trunk/misc/win32/start.c > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/misc/win32/start.c?rev=569882&r1=569881&r2=569882&view=diff > ============================================================================== > --- apr/apr/trunk/misc/win32/start.c (original) > +++ apr/apr/trunk/misc/win32/start.c Sun Aug 26 14:19:55 2007 > @@ -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 "assert.h" > > /* This symbol is _private_, although it must be exported. > @@ -203,6 +204,8 @@ > } > > apr_signal_init(pool); > + > + apr_threadproc_init(pool); > > return APR_SUCCESS; > } > > Modified: apr/apr/trunk/threadproc/win32/proc.c > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/win32/proc.c?rev=569882&r1=569881&r2=569882&view=diff > ============================================================================== > --- apr/apr/trunk/threadproc/win32/proc.c (original) > +++ apr/apr/trunk/threadproc/win32/proc.c Sun Aug 26 14:19:55 2007 > @@ -300,7 +300,7 @@ > 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 @@ > 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 @@ > 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 @@ > } > > #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.hStdError = (attr->child_err) > - ? attr->child_err->filehand > - : GetStdHandle(STD_ERROR_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 = 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 @@ > 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. > + */ > + EnterCriticalSection(&proc_lock); > + > +#else /* defined(_WIN32_WCE) */ > rv = CreateProcessW(wprg, wcmd, /* Executable & Command line > */ > NULL, NULL, /* Proc & thread security > attributes */ > FALSE, /* must be 0 */ Note s/EnterCriticalSection/LeaveCriticalSection just above, which was corrected with r569890.
