This is the mail archive of the cygwin-developers mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Expect goes crazy... spinning cpu in kill_pgrp


Dave Korn wrote:

>   No, not quite of course: there's still a trival TOCTOU race in, e.g.:

>   Argh.  We could move those data members into the redir stub so it won't
> crash if it touches them.  But there are still likely to be race conditions
> anyway, e.g. if we decide in that if() condition to kill the pid, then it
> might still go to state PID_EXECED between the if() and when kill() gets to do
> its stuff anyway.  Hmm.

  So looking closer, it seems that PID_EXECED is only (if ever) set once at
the start of execution and stays with the process from then on.  That suggests
to me that the problem has to actually be with a half-initialised pinfo being
visible to other processes during creation, which leads me to wonder if
something like /this/ is the solution; I'll try it later on tonight.

    cheers,
      DaveK

Index: winsup/cygwin/pinfo.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/pinfo.cc,v
retrieving revision 1.257
diff -p -u -r1.257 pinfo.cc
--- winsup/cygwin/pinfo.cc	14 Oct 2009 04:17:05 -0000	1.257
+++ winsup/cygwin/pinfo.cc	21 Oct 2009 20:27:03 -0000
@@ -231,12 +231,14 @@ pinfo::init (pid_t n, DWORD flag, HANDLE
   for (int i = 0; i < 20; i++)
     {
       DWORD mapsize;
+      _pinfo *p0;
+
       if (flag & PID_EXECED)
 	mapsize = PINFO_REDIR_SIZE;
       else
 	mapsize = sizeof (_pinfo);
 
-      procinfo = (_pinfo *) open_shared (L"cygpid", n, h0, mapsize, shloc,
+      p0 = (_pinfo *) open_shared (L"cygpid", n, h0, mapsize, shloc,
 					 sec_attribs, access);
       if (!h0)
 	{
@@ -245,7 +247,7 @@ pinfo::init (pid_t n, DWORD flag, HANDLE
 	  return;
 	}
 
-      if (!procinfo)
+      if (!p0)
 	{
 	  if (exit_state)
 	    return;
@@ -264,17 +266,17 @@ pinfo::init (pid_t n, DWORD flag, HANDLE
 
       bool created = shloc != SH_JUSTOPEN;
 
-      if ((procinfo->process_state & PID_INITIALIZING) && (flag & PID_NOREDIR)
-	  && cygwin_pid (procinfo->dwProcessId) != procinfo->pid)
+      if ((p0->process_state & PID_INITIALIZING) && (flag & PID_NOREDIR)
+	  && cygwin_pid (p0->dwProcessId) != p0->pid)
 	{
 	  set_errno (ESRCH);
 	  break;
 	}
 
-      if (procinfo->process_state & PID_EXECED)
+      if (p0->process_state & PID_EXECED)
 	{
 	  assert (i == 0);
-	  pid_t realpid = procinfo->pid;
+	  pid_t realpid = p0->pid;
 	  debug_printf ("execed process windows pid %d, cygwin pid %d", n, realpid);
 	  if (realpid == n)
 	    api_fatal ("retrieval of execed process info for pid %d failed due to recursion.", n);
@@ -289,25 +291,26 @@ pinfo::init (pid_t n, DWORD flag, HANDLE
 	 region to exist for a while after a process has exited.  This should
 	 only be a brief occurrence, so rather than introduce some kind of
 	 locking mechanism, just loop.  */
-      if (!created && createit && (procinfo->process_state & PID_EXITED))
+      if (!created && createit && (p0->process_state & PID_EXITED))
 	{
-	  debug_printf ("looping because pid %d, procinfo->pid %d, "
-			"procinfo->dwProcessid %u has PID_EXITED set",
-			n, procinfo->pid, procinfo->dwProcessId);
+	  debug_printf ("looping because pid %d, p0->pid %d, "
+			"p0->dwProcessid %u has PID_EXITED set",
+			n, p0->pid, p0->dwProcessId);
 	  goto loop;
 	}
 
       if (!created)
 	/* nothing */;
       else if (!(flag & PID_EXECED))
-	procinfo->pid = n;
+	p0->pid = n;
       else
 	{
-	  procinfo->process_state |= PID_IN_USE | PID_EXECED;
-	  procinfo->pid = myself->pid;
+	  p0->process_state |= PID_IN_USE | PID_EXECED;
+	  p0->pid = myself->pid;
 	}
 
       h = h0;	/* Success! */
+      procinfo = p0;
       break;
 
     loop:

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]