Bug 43329 - apr_proc_create behavior change
Summary: apr_proc_create behavior change
Alias: None
Product: APR
Classification: Unclassified
Component: APR (show other bugs)
Version: HEAD
Hardware: Other Windows 2000
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache Portable Runtime bugs mailinglist
URL: http://people.apache.org/~wrowe/apr-1...
Keywords: PatchAvailable
Depends on:
Reported: 2007-09-07 12:18 UTC by Tom Donovan
Modified: 2007-10-04 19:33 UTC (History)
2 users (show)

patch for threadproc/win32/proc.c bug 43329 (1.93 KB, patch)
2007-09-07 12:20 UTC, Tom Donovan
Details | Diff
2nd patch for threadproc/win32/proc.c bug 43329 (6.32 KB, patch)
2007-09-08 19:12 UTC, Tom Donovan
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Donovan 2007-09-07 12:18:53 UTC
apr_proc_create now passes zero instead of INVALID_HANDLE_VALUE for STDIN,
STDOUT, and STDERR which are zero in the procattr.

This change was caused by Revision 568819. 

Several 2rd-party modules (mod_fcgid, mod_fascgi, mod_perl, etc.) test
explicitly for INVALID_HANDLE_VALUE and are broken by this behavior change.
Comment 1 Tom Donovan 2007-09-07 12:20:15 UTC
Created attachment 20780 [details]
patch for threadproc/win32/proc.c bug 43329
Comment 2 William A. Rowe Jr. 2007-09-07 12:44:46 UTC
The problem is; reverting the behavior is a problem for unix compatibility.

By default, AUIU, unix passes any open handles for FD 0, 1, 2 always.

I'm not quite clear *which* handles are invalid in the running MPM, we should
have a stdin/out/err, no?

More clarification is needed here.
Comment 3 William A. Rowe Jr. 2007-09-07 12:52:54 UTC
Bug one in the proposed patch;

-            si.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
             if (attr->child_in && attr->child_in->filehand)
+                si.hStdInput = GetStdHandle(STD_INPUT_HANDLE);

With this change, you are requesting that an unset child_in be treated as

The trouble is, you still leak that std pipe to the child by not unsetting
the existing STD_INPUT_HANDLE.  For sake of the 'typical case', any file
that has been dup2'ed a valid IN/OUT/ERR file will be left as inherited
(which more closely resembles the way posix works).

Comment 4 William A. Rowe Jr. 2007-09-07 12:54:25 UTC
I ment to say "by not un-inheriting" in my comment above.  Sorry for the confusion. 
Comment 5 Tom Donovan 2007-09-07 13:13:46 UTC
Handles which are NULL in the attr arg (i.e. attr->child_xxx is NULL) are
*expected* to have invalid handles in the created process, not zero handles.

Most (all?) Windows fastcgi programs check stdin, stdout, & stderr to recognize
a fastcgi environment on Windows.

Unfortunately, it is not the Apache module which checks this (like mod_fcgid),
but the target executable.  FastCGI targets are usually linked with the
venerable old FastCGI Development kit (http://www.fastcgi.com/#TheDevKit
libfcgi.dll) or similar.  The code to recognize a fastcgi environment is
unfortunately baked into all the target exe's.

See http://www.fastcgi.com/cvs/fcgi2/libfcgi/os_win32.c
line 387 for a typical test:

      ... do the fastcgi Windows pipe stuff ...

No similar issues on Unix (AFAIK).  stdin/out/err are always 0,1,2 and can be
used the same whether it is a fastcgi environment or not.
Comment 6 William A. Rowe Jr. 2007-09-07 17:08:54 UTC
"No similar issues on Unix (AFAIK)."

Unfortunately not true of APR in general.  We specifically corrected this
behavior since launching cmd.exe in particular with 'crippled file handles' 
definately does not work correctly - particularly from a service.

"stdin/out/err are always 0,1,2 and can be used the same whether it is 
a fastcgi environment or not."


APR is about portability - making things behave the same.

In any case; I still need to ponder this; we are definately looking at some

If we should carefully consider how to achieve the same on all platforms,
even if that means telling APR "I want NO stdout/stderr" and having APR
do that consistently across platforms.

If you want to temporarily patch your own copy of apr until this gets worked
out, as I mention your proposed code has a bug...

this pattern below would do a better job for you of not leaking the unused
handles (and potentially tying up processes forever who's write ends refuse
to terminate until shutdown).

             si.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
+            if (GetHandleInformation(si.hStdInput, &stdin_reset)
+                    && (stdin_reset &= HANDLE_FLAG_INHERIT))
+                SetHandleInformation(si.hStdInput, HANDLE_FLAG_INHERIT, 0);
             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,
+            else
+                si.hStdInput = INVALID_HANDLE_VALUE;

Hmmm k?
Comment 7 Tom Donovan 2007-09-07 20:24:25 UTC
re: leaking handles - I cannot see any file handle leaks checking with the
'handle.exe' utility from:
It seems improbable that INVALID_HANDLE_VALUE handles could leak anyway, since
they're not real handles.

Making the std* handles un-inheritable seems to conflict with the notes about
which says:
"If this flag (STARTF_USESTDHANDLES) is specified when calling one of the
process creation functions, the handles must be inheritable and the function's
bInheritHandles parameter must be set to TRUE."

It's a shame that the practice of checking specifically for INVALID_HANDLE_VALUE
on Windows ever got started in the first place, but it's a practice that appears
to go back many years.

Comment 8 William A. Rowe Jr. 2007-09-08 08:42:08 UTC
"It seems improbable that INVALID_HANDLE_VALUE handles could leak anyway, since
they're not real handles."

Those aren't the handles that are leaking; the issue is that we have the parent's
three handles for STDIN/OUT/ERR which if we don't use them (passing INVALID
handles instead), the child still inherits all inheritable standard handles.

My favorite tool for exploring these happens to be sysinternals.com's 
ProcessExplorer, which illustrates these nicely with file pipe entries of
apr.{pid}.{#} when you use the logic in the old server or your suggested patch.

"Making the std* handles un-inheritable seems to conflict with the notes about

Only if you pass them.  Your patch proposes to remove them from the list of
handles passed to the child, so those parent handles are camped in the child
process without any reference that uses them.  Correct?
Comment 9 William A. Rowe Jr. 2007-09-08 15:37:00 UTC
Comment on attachment 20780 [details]
patch for threadproc/win32/proc.c bug 43329

Obsoleting the proposed patch, unfortunately the
suggested logic will lead an unindended handle for
any unused STD handle (e.g. where that attr_child_xxx 
entry is null).
Comment 10 William A. Rowe Jr. 2007-09-08 16:14:48 UTC
Here's one proposal;

if we added a new ***platform independent*** flag to apr_procattr_XXX(?) that
would close-on-exec any undesired (null) child handles on unix, while for win32
would set the StdHandle value to INVALID_HANDLE_VALUE, I could accept that this
would be very useful.

It's a new-flag because the side effects of changing the unix assumption result
in a app that opens a file, lets say it lands in fd2, and then the programmer
discovers there is additional garbage in their file emitted from some library
which logged some error condition to stderr.  On windows, some process such as
cmd.exe would mysteriously fail to start if they didn't exist in a console,
e.g. when cmd.exe is run in a service context.  Both bugs can be hard to track
down for the typical user.

So it appears that APR 1.2.11 behavior is correct, but confusing to libfcgid
and a handful of other apps that have exploited the existing error.  We can
work around those limited cases.

The patch on Win32 might look like this;

             si.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
-            if (attr->child_in && attr->child_in->filehand)
+            if ((attr->child_in && attr->child_in->filehand)
+                   || (attr->flags & APR_NO_DEFAULT_CHILD_STDFILES))
                 if (GetHandleInformation(si.hStdInput,
                         && (stdin_reset &= HANDLE_FLAG_INHERIT))
                                          HANDLE_FLAG_INHERIT, 0);
                 si.hStdInput = INVALID_HANDLE_VALUE;
+            if ((attr->child_in && attr->child_in->filehand)) {
                 si.hStdInput = attr->child_in->filehand;
                 SetHandleInformation(si.hStdInput, HANDLE_FLAG_INHERIT,

while on Unix, it would look something closer to

         if (attr->child_in) {
             dup2(attr->child_in->filedes, STDIN_FILENO);
+        else if (attr->flags & APR_NO_DEFAULT_CHILD_STDFILES) 
+            close(STDIN_FILENO);

Does this sound like a solution that would restore mod_fcgid without requiring
extra apr or libfcgid patches, while also permitting cross-platform behavior?

If you agree it's a solution, we can propose this to the dev@apr list and
pick up the discussion there.
Comment 11 William A. Rowe Jr. 2007-09-08 16:39:59 UTC
FYI, to actually implement that patch completely

         if ((attr->child_in && attr->child_in->filehand)
             || (attr->child_out && attr->child_out->filehand)
-            || (attr->child_err && attr->child_err->filehand))
+            || (attr->child_err && attr->child_err->filehand)
+            || (attr->flags & APR_NO_DEFAULT_CHILD_STDFILES))

is also required in a number of places to perform all of the appropriate

Another @bug, it would not work on Win9x, but we aren't particularly supporting
those anymore as they are entirely abandoned by MS.   Also would not work on
WinCE which has no 'stdio' construct or inherited std handles.

It should not be hard to implement on os2, beos or netware.
Comment 12 Tom Donovan 2007-09-08 19:11:47 UTC
It might be possible to confine the fix to Windows proc.c with enough
SetHandleInformation calls.  Since it's entirely a Windows problem, it seems
desirable to keep it out of the non-Windows code.

I've discovered a few things:

1. msvcr80 differs from msvcrt if si.hStdError contains INVALID_HANDLE_VALUE
but there is another valid stderr handle.  My orig patch didn't work with VS8
anyway because the new log.c cannot tolerate a missing stderr.  Previously VS8
builds had lots of failed apr calls in log.c because stderr was missing, but
they weren't visible (obviously...)  None were fatal.  The new httpd log.c has a
dup2 call (line 411) which breaks if stderr_log is null.

2. handle inheritance wasn't being reset correctly.  The 2nd arg to
SetHandleInformation is a mask, so the following doesn't reset the handle to
un-inheritable if stdin_reset is zero:

  SetHandleInformation(GetStdHandle(STD_INPUT_HANDLE),stdin_reset, stdin_reset);

SetHandleInformation is effectively a no-op when the second arg is zero.
That had me confused for a bit when the inheritance changed from call-to-call!  

3. There really is no good way to accommodate both the mpm_winnt child-creation
call (all 3 are inheritable but only stdin is passed in attr) and the various
combinations of inheritable (stderr can be changed by logging) handles and
supplied arguments in attr.  I came up with this rule:

* If 1-or-more attr handles are supplied and all 3 original handles are still
inheritable - keep them inheritable.  Otherwise, use INVALID_HANDLE_VALUE for
any missing handles in attr and suppress inheriting the original std* handles.

This does (I think) what you describe re: not allowing inherited handles in most
cases.  It does still allow the httpd child proc to inherit all 3 from its
parent, but once stdout becomes un-inheritable, no unwanted handles should ever
get to any later created processes.

A cmd.exe process would either need no handles in attr, or else three useable

I'll attach a patch which does all this.  I believe it works as expected for VC6
and VS8 builds on Win2k and WinXP with mod_perl and mod_fcgid (running
PHP-FastCGI or a stand-alone test FastCGI program).   I really should get a
clean copy of the 2.2.6-r2 source, apply the patch, and build & test it all
again - but I'll attach the patch now and add a comment if I find I overlooked
Comment 13 Tom Donovan 2007-09-08 19:12:49 UTC
Created attachment 20782 [details]
2nd patch for threadproc/win32/proc.c bug 43329
Comment 14 William A. Rowe Jr. 2007-09-09 02:06:15 UTC
Going the initial comment and postfix of comment #12 above, please don't worry 
about totally functional code, as right now we have to pound out some 
theoreticals.  In fact I *had* prompted the dev@apr list (who's readers include 
subversion, log4cxx and other apr applications beyond httpd) for any insight 
if the proposed 'fix' in APR 1.2.11 would break things.  This application case 
was not forseen.

and lastly... "Since it's entirely a Windows problem, it seems desirable 
to keep it out of the non-Windows code." ---

from the inception to first release of APR, it was never designed to solve
a per-platform issue, rather address the same computing problem for ALL of
our platforms.  In fact, it's old behavior definately qualifies as a /bug/
although your program was able to exploit that as a feature :)  So in short,
I'll definately require a bigger-picture solution to be able to back the
patch with a +1.  As I mention, the old behavior was a bug, conveinent to
a limited number of cases.

Now, I need to address the specific bullets and your proposed patch and I'll
do that in a later comment.
Comment 15 William A. Rowe Jr. 2007-09-09 02:15:12 UTC
I'll address your second bullet first in comment #12 above;

> 2. handle inheritance wasn't being reset correctly.  The 2nd arg to
> SetHandleInformation is a mask, so the following doesn't reset the handle to
> un-inheritable if stdin_reset is zero:
>  SetHandleInformation(GetStdHandle(STD_INPUT_HANDLE),stdin_reset, stdin_reset);

I missed your point as applies to either the current 1.2.11 code, or my
proposed code fragement above - just above the reset you should find;

            if (stdin_reset)
                                     stdin_reset, stdin_reset);

and you are right, stdin_reset will *not* toggle an uninheritable
handle to inheritable; we determined stdin_reset from the parent
processes' STD_INPUT_HANDLE current value.  We mask it & INHERIT so
that we examine and toggle none of the other possible handle flags.

If stdin_reset is 0 - there is nothing to set back to INHERITED - then
the SetHandleInformation is not called, and the current STD_INPUT_HANDLE
enters and leaves apr_proc_create unmolested.  Right?
Comment 16 Tom Donovan 2007-09-09 07:26:26 UTC
I should have been clearer about #2.  A case is where the parent's
(non-inheritable) stdin handle is also passed in attr as child_in.  It is set to
inheritable, but never reset to not-inheritable before leaving apr_proc_create.
Probably an edge case anyway.

re: "the larger context" - Agreed, APR should be as platform-independent and
consistent as possible. Alas, FastCGI programs are the real-world issue.  If it
was just a matter of updating a few modules like mod_fcgid, mod_fastcgi et. al.
this would probably be OK.  The problem is not in these modules, but in all the
target executables which these modules run in external processes.  These target
exes include common ones like php-cgi.exe (which probably *could* also be
changed if necessary) and many one-off or in-house exe's which have been ported
from Unix fastcgi over the years since APR change 63707 in 2002.  These target
executables acquired a built-in dependency on INVALID_HANDLE_VALUE for stdout &
stderr on Windows.  Finding and re-building all these targets is the painful
part of accommodating this APR change.

re: "the old behavior was a bug, conveinent to a limited number of cases"
I think some (particularly the fastcgi library developers) interpreted the
change to pass INVALID_HANDLE_VALUE as an intentional change, albeit a
Windows-specific one.

re: "your program" - It's ironic!  Many Win/Apache sites depend on some variant
of fastcgi.  I'm one of the rare ones who don't.  My only affected programs were
constructed just for testing this release. I'm just sympathetic to the majority
who do need it fixed.

The comments in my proposed patch are inappropriately httpd-specific for an APR
function and should probably be changed.
Comment 17 Tom Donovan 2007-09-09 12:55:52 UTC
I've built the 2nd patch with VC6 and VS8 and tested on Win2k and WinXP.

While it does as expected, it will not allow httpd 2.2.6 to run as a Windows

server/log.c has changed for 2.2.6 in a way that is incompatible with fastcgi

  the mpm_winnt child process is created with child_in as the only handle 
  supplied in attr.
  log.c now *requires* a valid stderr, so one must be created or inherited.
  Prior to 2.2.6 it could survive until the parent pipe got set up.

  fastcgi processes are also created with child_in as the only handle 
  supplied in attr.  
  fastcgi requires that both out & err be INVALID_HANDLE_VALUE

When httpd is started as a service, all three handles are zero (and invalid), so
inheritance can't help.  When started as a normal process, stderr gets inherited
in APR 1.2.11 (either with or without the patch).  When started as a service, if
the patch sets stderr=INVALID_HANDLE_VALUE, this breaks log.c.

In other words, the rule I proposed (... all 3 original handles are inheritable
...) won't work for Windows services.

I don't see a way to solve this for both log.c and fastcgi, short of changes to
both APR's win proc.c and changes to httpd.   Any ideas?
Comment 18 William A. Rowe Jr. 2007-09-12 01:10:47 UTC
"I don't see a way to solve this for both log.c and fastcgi, short of changes to
both APR's win proc.c and changes to httpd.   Any ideas?"

I guess I have to step back and ask a few things.  Typical cgi looks like

  httpd.exe [parent] (happens to set up stdout \device\null, stderr to errorlog)
   \-- httpd.exe [child] (gets a stdin from parent for passing handles)
        \-- prog.exe (stdin/out set up to pipe cgi, errors to errorlog)

That's simple enough so far.  Now mod_fcgid spawns a long-lived app serving
process, and wants to pass that app no stdout/stderr.

What would be wrong with mod_fcgid invoking

apr_procattr_io_set(fcgid_procattr, APR_FULL_BLOCK, APR_NO_FILE, APR_NO_FILE);

for it's procattr structure, and then invoking the fcgi app with those flags?

What happens when we add a new flag to win32, at least, is that previous apr
versions would treat those as APR_FULL_BLOCK, creating pipes that are never
used.  With the next apr release forward, they would be INVALID_FILE_HANDLE
notations to avoid passing the parent's current stdhandles, or any other handle.

To accomplish the same but using apr_procattr_stdin_set() instead, just first
call this;

apr_procattr_io_set(fcgid_procattr,  APR_NO_FILE, APR_NO_FILE, APR_NO_FILE);

and it will mark those as not-inherited, although stdin_set would override.

Wouldn't this solve the case of using apr file_io?  The compiled modules aren't
using apr themselves, are they?  Or if they are, it doesn't matter, since they
aren't being recompiled and they dealt with INVALID_FILE_HANDLES just fine,

I agree we don't want to rebuild every fcgi application out there.  Obviously
mod_fcgid needs a recompile.  I'm trying to find the shortest path.
Comment 19 Tom Donovan 2007-09-12 17:13:38 UTC
Yes, a new flag APR_NO_FILE to trigger INVALID_HANDLE_VALUE in the created
process seems like it would work OK for both mod_fcgid and mod_fastcgi on
Windows.  These are the two I have tested with, and I suspect they are the two
most commonly used fastcgi-launching Apache modules on Windows.

Presumably APR_NO_FILE would be equivalent to APR_NO_PIPE for Unix platforms. 
This would avoid platform-dependent calls to apr_proc_create (both mod_fcgid and
mod_fastcgi also run on Unix).

This is certainly much better than requiring changes to all the target executables.

re: "The compiled modules aren't using apr themselves, are they?"
Typically not, although it's hard to generalize about what all the target
executables do. It seems very unlikely that any would create additional
processes using apr_proc_create and also depend on stderr/stdout being
Comment 20 Tom Donovan 2007-09-28 09:18:38 UTC
Applying the proposed patch from
to Apache 2.2.6-r2 source on Windows works correctly with mod_fcgid and
mod_fastcgi when the modules' process creation code is changed like this:

#ifdef APR_NO_FILE
    /* required for APR 1.2.11+ to pass INVALID_HANDLE_VALUE */
    if (apr_procattr_io_set(attr,  APR_NO_FILE, APR_NO_FILE, APR_NO_FILE))
            {...handle errors...};
    if (apr_procattr_child_in_set(procattr, file, NULL))
            {...handle errors...}; 

Tested with VC6 and VC8 on Win2k.

The mod_fcgid developers have indicated willingness to make the change. I don't
know about the mod_fastcgi developers, but I expect they will too eventually.

One minor nit - in proc.c (lines 802-832) the _reset vars are always set to

    if (GetHandleInformation(si.hStdOutput,
            && (stdout_reset &= HANDLE_FLAG_INHERIT))

You probably meant: (stdout_reset & HANDLE_FLAG_INHERIT) 
instead of:         (stdout_reset &= HANDLE_FLAG_INHERIT).  
Not relevant to this bug, but it was the reason the heritability changed between
Comment 21 William A. Rowe Jr. 2007-09-28 10:07:36 UTC
On your comment above;
> You probably meant: (stdout_reset & HANDLE_FLAG_INHERIT) 
> instead of:         (stdout_reset &= HANDLE_FLAG_INHERIT).  

It does mean exactly what it states already, an and-mask-assignment;

    if (GetHandleInformation(si.hStdOutput,
            && (stdout_reset &= HANDLE_FLAG_INHERIT))

should presently GetHandleInformation(si.hStdOutput, &stdout_reset)
  assigning the value of the current handle flags to stdout_reset
and if not 0 (success), 
  stdout_reset contains the handle bits from GetHandleInformation,
  (stdout_reset &= HANDLE_FLAG_INHERIT)
    causes stdout_reset to be masked with only the previous inherit bit,
    resulting in a value of 0 or HANDLE_FLAG_INHERIT based on it's prior value.
but if GetHandleInformation returns 0 (failure)
  stdout_reset is not touched
  the stdout_reset value is not &= and-mask assigned

Comment 22 William A. Rowe Jr. 2007-09-28 10:14:16 UTC
Note patch; thank you Tom and Randy for validating this patch!
Comment 23 Tom Donovan 2007-09-28 10:22:30 UTC
re: "It does mean exactly what it states already ... No?"

Yep, you're right!
I guess I should stop pondering the set/reset STD inheritance flags any more.  
I'll stop now :-)
Comment 24 Tom Donovan 2007-10-02 04:14:05 UTC
Proposed an Apache patch instead of an APR patch in bug 43534 to accomplish this
without requiring module changes, and also re-enable mod_perl.
Comment 25 William A. Rowe Jr. 2007-10-02 11:11:18 UTC
FYI The patch to introduce APR_NO_FILE as an apr_procattr_io_set() will be
present in apr 0.9.17 + 1.2.12 to reintroduce this desired behavior.

That flag becomes portable with APR 1.3, and was introduced prematurely
('violating' our versioning philosophy) for the one platform solution to
simultaneously correct portability (new behavior) - and feature set (old
behavior/new flag).

Comment 26 William A. Rowe Jr. 2007-10-04 19:33:41 UTC

Here's the upshot, folks.  The fix of APR_NO_FILE is applied for the eventual
release of APR 1.3.0, probably with httpd 2.4.  Your change to test for this
flag (#ifdef) and use this API for apps which insist on INVALID_HANDLE_VALUE 
will be a good change, but is not needed for 2.0/2.2.

The patch introducing APR_NO_FILE as an apr_procattr_io_set() option is reverted,
the behavior itself, to inherit the parent's stdio handles if one has been set
through apr_procattr_io_set() or apr_procattr_child_XXX_set(), is also reverted.
So APR 1.2.12 will revert in *that* respect to the behavior of 1.2.8, and the
APR 0.9.17 behavior will revert in that respect to 0.9.14.

Sorry for the hassles introduced by attempting compatibility with the unix
implementation, and we look forward to your module running as expected for any
users on apr 1.2.x, and encourage you to patch already in order to accommodate
your users on apr 1.3.x and later.