Summary: | apr_proc_create behavior change | ||
---|---|---|---|
Product: | APR | Reporter: | Tom Donovan <Tom.Donovan> |
Component: | APR | Assignee: | Apache Portable Runtime bugs mailinglist <bugs> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | covener, peter |
Priority: | P2 | Keywords: | PatchAvailable |
Version: | HEAD | ||
Target Milestone: | --- | ||
Hardware: | Other | ||
OS: | Windows 2000 | ||
URL: | http://people.apache.org/~wrowe/apr-1.2-win32-nohandle.patch | ||
Attachments: |
patch for threadproc/win32/proc.c bug 43329
2nd patch for threadproc/win32/proc.c bug 43329 |
Description
Tom Donovan
2007-09-07 12:18:53 UTC
Created attachment 20780 [details] patch for threadproc/win32/proc.c bug 43329 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. 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 INVALID_HANDLE_VALUE. 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). I ment to say "by not un-inheriting" in my comment above. Sorry for the confusion. 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: if((GetStdHandle(STD_OUTPUT_HANDLE) == INVALID_HANDLE_VALUE) && (GetStdHandle(STD_ERROR_HANDLE) == INVALID_HANDLE_VALUE) && (GetStdHandle(STD_INPUT_HANDLE) != INVALID_HANDLE_VALUE) ) { ... 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. "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." Exactly. APR is about portability - making things behave the same. In any case; I still need to ponder this; we are definately looking at some aberration. 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, HANDLE_FLAG_INHERIT); } + else + si.hStdInput = INVALID_HANDLE_VALUE; Hmmm k? re: leaking handles - I cannot see any file handle leaks checking with the 'handle.exe' utility from: http://www.microsoft.com/technet/sysinternals/ProcessesAndThreads/Handle.mspx 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 STARTF_USESTDHANDLES in: http://msdn2.microsoft.com/en-us/library/ms686331.aspx 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. -tom- "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 STARTF_USESTDHANDLES" 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 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). 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) && (stdin_reset &= HANDLE_FLAG_INHERIT)) SetHandleInformation(si.hStdInput, 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, HANDLE_FLAG_INHERIT); } while on Unix, it would look something closer to if (attr->child_in) { apr_file_close(attr->parent_in); dup2(attr->child_in->filedes, STDIN_FILENO); apr_file_close(attr->child_in); } + 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. 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 processing. 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. 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 handles. 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 something. Created attachment 20782 [details] 2nd patch for threadproc/win32/proc.c bug 43329 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. 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) SetHandleInformation(GetStdHandle(STD_INPUT_HANDLE), 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? 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. 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 service. server/log.c has changed for 2.2.6 in a way that is incompatible with fastcgi requirements: 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? "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, before. 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. 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 INVALID_HANDLE_VALUE. Not a worry (IMHO). Applying the proposed patch from http://people.apache.org/~wrowe/apr-1.2-win32-nohandle.patch 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...}; #endif if (apr_procattr_child_in_set(procattr, file, NULL)) {...handle errors...}; {...apr_proc_create...} 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 "inheritable": if (GetHandleInformation(si.hStdOutput, &stdout_reset) && (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 calls. 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)
&& (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
No?
Note patch; thank you Tom and Randy for validating this patch! 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 :-) 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. 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). *** THE FINAL CONCLUSION *** 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. |