Child processes always unwantedly inherit Handles on Windows

[Deewiant]

runInteractiveProcess (the one in runProcess.c, not the Haskell one) calls CreateProcess with bInheritHandles set to TRUE, but the whole thing is not wrapped in a critical section. Microsoft has  a decent explanation of why this is a problem and I found that it's  a two-year old unfixed bug in the Java runtime as well.

In case it's not obvious, the reason why we don't want Handles to be inherited is that then a child process may hang on to it well after it should have been closed. The end result is that trying to do anything to it results in a sharing violation. The case in which I originally ran into this was System.Directory.copyFile intermittently reporting a "permission denied" error for a temp file it was using. I think it was trying to delete it, but failing because a child process somewhere was hanging on to the Handle.

Attached is code demonstrating the problem. It forks a thread which repeatedly tries to open and close the file foo.tmp. The main thread then spawns processes as fast as it can until the other one fails to open the file. This, I think, cannot happen on non-Windows platforms since two processes can have the same file open without trouble. Do not run except on Windows, you risk flooding your system with ping processes.

It may be sensible to make this controllable with the close_fds parameter to createProcess, which is currently just ignored on Windows. I can't see why anybody could ever want this behaviour, though.

I've tested only process-1.0.1 with it, and the openFile fails reliably after spawning up to about 4 processes.

[Deewiant]

Test case

[Deewiant]

Okay then, some info and bugfixes follow. Thanks a lot to dcoutts for pointing me in the right direction and helping me along with this.

There are actually two issues, only one of which is (poorly) tested by createprocess-race-condition.hs:

1. The race condition described in the Microsoft KB article.
2. The fact that all file handles are inheritable by default, meaning that every child process always inherits them. This leads to errors if a child is still alive when the parent wants to e.g. delete a file which was open when the child was born.

The second issue is what is tested by the original test case. It only happens sporadically because the file handle is kept open for a very brief interval. The first issue I haven't managed to reproduce, but I don't see a reason why it couldn't happen given the current code in runProcess.c. Even if the second issue is fixed it should be possible that the anonymous pipes for a child's stdin/out/err streams are inherited by some other process.

I'm attaching:

1. A better test case for issue 2 above.
2. A patch against base which disables inheritance of file handles, solving most cases of issue 2.
3. A tentative patch for issue 1 which I haven't tested properly, against process: it wraps most of runInteractiveProcess in a critical section for atomicity and rewrites the helper function mkAnonPipe to work as described in the  KB article explaining the race condition issue.

Both patches are against the 6.10 darcs HEAD.

According to  MSDN's list of handle types that may be inherited, at least the network library still needs to be changed: sockets shouldn't be inherited either. (I don't think any of the other possible handles apply.) If anybody else wants to do that bit, see  this KB article for info on how it should be done. Note that the inheritance disabling should be done atomically, to avoid the race condition described above.

I'm pointing this bug at "libraries (other)" since it affects at least all of base, process, and network.

[Deewiant]

A better test case than createprocess-race-condition

[Deewiant]

Haven't found a test case for this yet

[Deewiant]

I just realized an error in the patch against process: a critical section object is now initialized and deleted each time runInteractiveProcess is called. This makes it completely useless since each thread gets its own critical section!

It should be made into a global, initializing it only once. Deleting it is probably unnecessary since the OS will free it when the process exits, and you can't be sure it won't be reused until that happens anyway. I defer doing this to someone who knows more about how that single initialization can be performed.

[igloo]

High priority as patches are attached.

[duncan]

So according to the  JVM bug report we can make CreateProcess.close_fds work except when also trying to redirect stdin/out. Currently we do not attempt to make it work at all. And on Vista we can make it work even when redirect stdin/out.

So it seems like we both need to make new handles (eg from opening files and sockets) non-inheritable and implement the above tricks.

This all looks quite doable but will need some careful testing.

[simonmar]

See also #3231

[simonmar]

Fixed:

Tue Jun  2 12:58:07 BST 2009  Simon Marlow <marlowsd@gmail.com>
  * Fix #2650: avoid a race condition on Windows when creating sub-processes

I think this part of the bug is much more unlikely to cause problems than #3231, as it should only affect you if you're creating processes from multiple threads, and even then it might not cause a problem, because the only effect is to unnecessarily inherit a pipe Handle or two. There's also a workaround: do your own mutual exclusion. So I'm not suggesting we merge this into 6.10.4.