apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: svn commit: r569882 - in /apr/apr/trunk: file_io/win32/pipe.c include/arch/win32/apr_arch_inherit.h include/arch/win32/apr_arch_threadproc.h misc/win32/start.c threadproc/win32/proc.c
Date Mon, 27 Aug 2007 00:00:49 GMT
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


wrowe@apache.org 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.

Mime
View raw message