* cygthread.cc (cygthread::stub): Detect if thread function wants to release

itself here, to avoid a race.
(cygthread::release): Clear more stuff.  Add a diagnostic for an internal
error.
* cygthread.h (auto_release): New function.
* pinfo.h (pinfo::remember): Add an argument to denote whether child is
detached.
* fork.cc (fork_parent): Reflect change in arguments to pinfo::remember.
* pinfo.cc (_pinfo::exit): Signal exit more forcibly.
(proc_waiter): Use cygthread::auto_release to signify that cygthread::stub
should release the thread.  This should avoid a race.
(pinfo::alert_parent): Don't signify an error when wr_proc_pipe == NULL.
* sigproc.cc (proc_subproc): Add support for PROC_DETACHED_CHILD.
* sigproc.h: Ditto.
* spawn.cc (spawn_guts): Specify whether child is detached or not when calling
pinfo::remember.

9 files changed, 70 insertions, 34 deletions

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index f5760e6..286cab2 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,3 +1,23 @@
+2004-12-23  Christopher Faylor  <cgf@timesys.com>
+
+	* cygthread.cc (cygthread::stub): Detect if thread function wants to
+	release itself here, to avoid a race.
+	(cygthread::release): Clear more stuff.  Add a diagnostic for an
+	internal error.
+	* cygthread.h (auto_release): New function.
+	* pinfo.h (pinfo::remember): Add an argument to denote whether child is
+	detached.
+	* fork.cc (fork_parent): Reflect change in arguments to
+	pinfo::remember.
+	* pinfo.cc (_pinfo::exit): Signal exit more forcibly.
+	(proc_waiter): Use cygthread::auto_release to signify that
+	cygthread::stub should release the thread.  This should avoid a race.
+	(pinfo::alert_parent): Don't signify an error when wr_proc_pipe == NULL.
+	* sigproc.cc (proc_subproc): Add support for PROC_DETACHED_CHILD.
+	* sigproc.h: Ditto.
+	* spawn.cc (spawn_guts): Specify whether child is detached or not when
+	calling pinfo::remember.
+
 2004-12-22  Christopher Faylor  <cgf@timesys.com>
 
 	* cygheap.cc (cygheap_setup_for_child): Add api_fatal to catch failing
diff --git a/winsup/cygwin/cygthread.cc b/winsup/cygwin/cygthread.cc
index 949f471..bca2b28 100644
--- a/winsup/cygwin/cygthread.cc
+++ b/winsup/cygwin/cygthread.cc
@@ -69,13 +69,21 @@ cygthread::stub (VOID *arg)
 	  info->func (info->arg == cygself ? info : info->arg);
 	  /* ...so the above should always return */
 
+	  /* If stack_ptr is NULL, the above function has set that to indicate
+	     that it doesn't want to alert anyone with a SetEvent and should
+	     just be marked as no longer inuse.  Hopefully the function knows
+	     that it is doing.  */
+	  if (!info->func)
+	    info->release (false);
+	  else
+	    {
 #ifdef DEBUGGING
-	  info->func = NULL;	// catch erroneous activation
-	  info->__oldname = info->__name;
+	      info->func = NULL;	// catch erroneous activation
+	      info->__oldname = info->__name;
 #endif
-	  info->__name = NULL;
-	  if (info->inuse)
-	    SetEvent (info->ev);
+	      info->__name = NULL;
+	      SetEvent (info->ev);
+	    }
 	}
       switch (WaitForSingleObject (info->thread_sync, INFINITE))
 	{
@@ -231,7 +239,13 @@ cygthread::release (bool nuke_h)
   __oldname = __name;
   __name = NULL;
   stack_ptr = NULL;
-  (void) InterlockedExchange (&inuse, 0); /* No longer in use */
+  func = NULL;
+  if (!InterlockedExchange (&inuse, 0))
+#ifdef DEBUGGING
+    api_fatal ("released a thread that was not inuse");
+#else
+    system_printf ("released a thread that was not inuse");
+#endif
 }
 
 /* Forcibly terminate a thread. */
diff --git a/winsup/cygwin/cygthread.h b/winsup/cygwin/cygthread.h
index 265e51c..81cb681 100644
--- a/winsup/cygwin/cygthread.h
+++ b/winsup/cygwin/cygthread.h
@@ -32,6 +32,7 @@ class cygthread
   static DWORD WINAPI simplestub (VOID *);
   static DWORD main_thread_id;
   static const char * name (DWORD = 0);
+  void auto_release () {func = NULL;}
   void release (bool);
   cygthread (LPTHREAD_START_ROUTINE, LPVOID, const char *);
   cygthread () {};
diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index 770c6c2..b047814 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -413,7 +413,7 @@ fork_parent (HANDLE& hParent, dll *&first_dll,
      it in afterwards.  This requires more bookkeeping than I like, though,
      so we'll just do it the easy way.  So, terminate any child process if
      we can't actually record the pid in the internal table. */
-  if (!child.remember ())
+  if (!child.remember (false))
     {
       TerminateProcess (pi.hProcess, 1);
       set_errno (EAGAIN);
diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
index af0b893..ec51c85 100644
--- a/winsup/cygwin/pinfo.cc
+++ b/winsup/cygwin/pinfo.cc
@@ -135,8 +135,8 @@ _pinfo::exit (UINT n, bool norecord)
 	  /* We could just let this happen automatically when the process
 	     exits but this should gain us a microsecond or so by notifying
 	     the parent early.  */
-	  if (wr_proc_pipe)
-	    CloseHandle (wr_proc_pipe);
+	  myself.alert_parent (0);
+	    
 	}
     }
 
@@ -764,7 +764,7 @@ proc_waiter (void *arg)
 
   sigproc_printf ("exiting wait thread for pid %d", pid);
   vchild.wait_thread = NULL;
-  _my_tls._ctinfo->release (false);	/* return the cygthread to the cygthread pool */
+  _my_tls._ctinfo->auto_release ();	/* automatically return the cygthread to the cygthread pool */
   return 0;
 }
 
@@ -816,10 +816,10 @@ pinfo::alert_parent (char sig)
   DWORD nb = 0;
   /* Send something to our parent.  If the parent has gone away,
      close the pipe. */
-  if (myself->wr_proc_pipe == INVALID_HANDLE_VALUE)
+  if (myself->wr_proc_pipe == INVALID_HANDLE_VALUE
+      || !myself->wr_proc_pipe)
     /* no parent */;
-  else if (myself->wr_proc_pipe
-	   && WriteFile (myself->wr_proc_pipe, &sig, 1, &nb, NULL))
+  else if (WriteFile (myself->wr_proc_pipe, &sig, 1, &nb, NULL))
     /* all is well */;
   else if (GetLastError () != ERROR_BROKEN_PIPE)
     debug_printf ("sending %d notification to parent failed, %E", sig);
diff --git a/winsup/cygwin/pinfo.h b/winsup/cygwin/pinfo.h
index d4034e1..6516832 100644
--- a/winsup/cygwin/pinfo.h
+++ b/winsup/cygwin/pinfo.h
@@ -168,9 +168,10 @@ public:
 #ifndef _SIGPROC_H
   int remember () {system_printf ("remember is not here"); return 0;}
 #else
-  int remember ()
+  int remember (bool detach)
   {
-    int res = proc_subproc (PROC_ADDCHILD, (DWORD) this);
+    int res = proc_subproc (detach ? PROC_DETACHED_CHILD : PROC_ADDCHILD,
+			    (DWORD) this);
     destroy = res ? false : true;
     return res;
   }
diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
index 7f296d2..934988d 100644
--- a/winsup/cygwin/sigproc.cc
+++ b/winsup/cygwin/sigproc.cc
@@ -38,8 +38,6 @@ details. */
 #define WSSC		  60000	// Wait for signal completion
 #define WPSP		  40000	// Wait for proc_subproc mutex
 
-#define PSIZE 63		// Number of processes
-
 #define no_signals_available() (!hwait_sig || (myself->sendsig == INVALID_HANDLE_VALUE) || exit_state)
 
 #define NPROCS	256
@@ -246,7 +244,9 @@ proc_subproc (DWORD what, DWORD val)
 	  set_errno (EAGAIN);
 	  break;
 	}
+      /* fall through intentionally */
 
+    case PROC_DETACHED_CHILD:
       if (vchild != myself)
 	{
 	  vchild->ppid = myself->pid;
@@ -258,6 +258,8 @@ proc_subproc (DWORD what, DWORD val)
 	  vchild->cygstarted = true;
 	  vchild->process_state |= PID_INITIALIZING | (myself->process_state & PID_USETTY);
 	}
+      if (what == PROC_DETACHED_CHILD)
+	break;
       procs[nprocs] = vchild;
       rc = procs[nprocs].wait ();
       if (rc)
diff --git a/winsup/cygwin/sigproc.h b/winsup/cygwin/sigproc.h
index 9a432d3..020d438 100644
--- a/winsup/cygwin/sigproc.h
+++ b/winsup/cygwin/sigproc.h
@@ -30,11 +30,11 @@ enum
 
 enum procstuff
 {
-  PROC_ADDCHILD		= 1,	// add a new subprocess to list
-  PROC_CHILDTERMINATED	= 2,	// a child died
-  PROC_CLEARWAIT	= 3,	// clear all waits - signal arrived
-  PROC_WAIT		= 4,	// setup for wait() for subproc
-  PROC_NOTHING		= 5	// nothing, really
+  PROC_ADDCHILD		  = 1,	// add a new subprocess to list
+  PROC_DETACHED_CHILD	  = 2,	// set up a detached child
+  PROC_CLEARWAIT	  = 3,	// clear all waits - signal arrived
+  PROC_WAIT		  = 4,	// setup for wait() for subproc
+  PROC_NOTHING		  = 5	// nothing, really
 };
 
 struct sigpacket
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 587c963..1c84fad 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -802,7 +802,7 @@ spawn_guts (const char * prog_arg, const char *const *argv,
 	this). */
       if (!myself->wr_proc_pipe)
        {
-	 myself.remember ();
+	 myself.remember (true);
 	 wait_for_myself = true;
 	 myself->wr_proc_pipe = INVALID_HANDLE_VALUE;
        }
@@ -822,14 +822,6 @@ spawn_guts (const char * prog_arg, const char *const *argv,
 	}
       child->dwProcessId = pi.dwProcessId;
       child.hProcess = pi.hProcess;
-      if (!child.remember ())
-	{
-	  /* FIXME: Child in strange state now. */
-	  CloseHandle (pi.hProcess);
-	  CloseHandle (pi.hThread);
-	  res = -1;
-	  goto out;
-	}
 
       strcpy (child->progname, real_path);
       /* FIXME: This introduces an unreferenced, open handle into the child.
@@ -839,9 +831,15 @@ spawn_guts (const char * prog_arg, const char *const *argv,
 	 However, we should try to find another way to do this eventually. */
       (void) DuplicateHandle (hMainProc, child.shared_handle (), pi.hProcess,
 			      NULL, 0, 0, DUPLICATE_SAME_ACCESS);
-      if (mode == _P_DETACH)
-	myself.alert_parent (0);
       child->start_time = time (NULL); /* Register child's starting time. */
+      if (!child.remember (mode == _P_DETACH))
+	{
+	  /* FIXME: Child in strange state now */
+	  CloseHandle (pi.hProcess);
+	  ForceCloseHandle (pi.hThread);
+	  res = -1;
+	  goto out;
+	}
     }
 
 /* Start the child running */
@@ -867,7 +865,7 @@ switch (mode)
       res = -1;
     break;
   case _P_DETACH:
-    res = 0;	/* Lose all memory of this child. */
+    res = 0;	/* Lost all memory of this child. */
     break;
   case _P_NOWAIT:
   case _P_NOWAITO: