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

6 files changed, 201 insertions, 50 deletions

diff --git a/CHANGES b/CHANGES
index 40b928768..e2f8014f5 100644
--- a/CHANGES
+++ b/CHANGES
@@ -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 */