On Feb 5 09:42, Michael Haubenwallner wrote: > On 2/4/19 3:25 PM, Corinna Vinschen wrote: > > Are you going to test the patched branch? > > Sorry, was indeed unclear: Yes, of course! > Will start testing on Server 2012 while setting up a 2019 VM. > > For now, there's already this one patch I've been using with good success, > please add it to topic/forkables - the suspended thing is something different: > https://cygwin.com/ml/cygwin-patches/2018-q2/msg00039.html
The collision problem shouldn't be as bad anymore with 3.0, given the
new PID handling. However, after spending a bit more time in the fork
code, it looks like not releasing the procinfo in the error case is a
generic problem so I'm inclined to apply it to master.
While at it, there are quite a few spots in the code which end up
jumping to the cleanup code but only one of them calls TerminateProcess.
Wouldn't it make sense to move the TerminateProcess call into the
cleanup code to make sure the child process doesn't stay running
in some limbo state, not doing anything useful but not dying either?
Kind of like this:
diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index 6f00364334c3..5f775249a990 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -400,7 +400,6 @@ frok::parent (volatile char * volatile stack_here)
we can't actually record the pid in the internal table. */
if (!child.remember (false))
{
- TerminateProcess (hchild, 1);
this_errno = EAGAIN;
#ifdef DEBUGGING0
error ("child remember failed");
@@ -508,8 +507,12 @@ cleanup:
__malloc_unlock ();
/* Remember to de-allocate the fd table. */
- if (hchild && !child.hProcess) /* no child.procinfo */
- ForceCloseHandle1 (hchild, childhProc);
+ if (hchild)
+ {
+ TerminateProcess (hchild, 1);
+ if (!child.hProcess) /* no child.procinfo */
+ ForceCloseHandle1 (hchild, childhProc);
+ }
if (forker_finished)
ForceCloseHandle (forker_finished);
debug_printf ("returning -1");
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
