This is the mail archive of the
cygwin-developers
mailing list for the Cygwin project.
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: