* DevNotes: Add entry cgf-000003.

* cygheap.h (init_cygheap::pid_handle): Delete.
* dcrt0.cc (child_info_spawn::handle_spawn): Keep parent open if we have
execed.
* pinfo.cc (pinfo::thisproc): Remove pid_handle manipulations.
(pinfo::init): Don't consider a reaped process to be available.
* spawn.cc (child_info_spawn::worker): Remove pid_handle manipulations.  Make
wr_proc_pipe and parent noninheritable when starting a program which doesn't
use the Cygwin DLL.  Conditionally reset wr_proc_pipe to inheritable if
CreateProcess fails.  Inject wr_proc_pipe handle into non-Cygwin process.
Consider a non-cygwin process to be 'synced'.
This commit is contained in:
Christopher Faylor 2012-05-07 15:05:56 +00:00
parent 8620cb7925
commit 06bd0ef2ab
7 changed files with 110 additions and 24 deletions

View File

@ -1,3 +1,17 @@
2012-05-07 Christopher Faylor <me.cygwin2012@cgf.cx>
* DevNotes: Add entry cgf-000003.
* cygheap.h (init_cygheap::pid_handle): Delete.
* dcrt0.cc (child_info_spawn::handle_spawn): Keep parent open if we
have execed.
* pinfo.cc (pinfo::thisproc): Remove pid_handle manipulations.
(pinfo::init): Don't consider a reaped process to be available.
* spawn.cc (child_info_spawn::worker): Remove pid_handle manipulations.
Make wr_proc_pipe and parent noninheritable when starting a program
which doesn't use the Cygwin DLL. Conditionally reset wr_proc_pipe to
inheritable if CreateProcess fails. Inject wr_proc_pipe handle into
non-Cygwin process. Consider a non-cygwin process to be 'synced'.
2012-05-03 Christopher Faylor <me.cygwin2012@cgf.cx> 2012-05-03 Christopher Faylor <me.cygwin2012@cgf.cx>
* DevNotes: Add entry cgf-000002. * DevNotes: Add entry cgf-000002.

View File

@ -1,3 +1,54 @@
2012-05-03 cgf-000003
<1.7.15>
Don't make Cygwin wait for all children of a non-cygwin child program.
Fixes: http://cygwin.com/ml/cygwin/2012-05/msg00063.html,
http://cygwin.com/ml/cygwin/2012-05/msg00075.html
</1.7.15>
This problem is due to a recent change which added some robustness and
speed to Cygwin's exec/spawn handling by not trying to force inheritance
every time a process is started. See ChangeLog entries starting on
2012-03-20, and multiple on 2012-03-21.
Making the handle inheritable meant that, as usual, there were problems
with non-Cygwin processes. When Cygwin "execs" a non-Cygwin process N,
all of its N + 1, N + 2, ... children will also inherit the handle.
That means that Cygwin will wait until all subprocesses have exited
before it returns.
I was willing to make this a restriction of starting non-Cygwin
processes but the problem with allowing that is that it can cause the
creation of a "limbo" pid when N exits and N + 1 and friends are still
around. In this scenario, Cygwin dutifully notices that process N has
died and sets the exit code to indicate that but N's parent will wait on
rd_proc_pipe and will only return when every N + ... windows process
has exited.
The removal of cygheap::pid_handle was not related to the initial
problem that I set out to fix. The change came from the realization
that we were duping the current process handle into the child twice and
only needed to do it once. The current process handle is used by exec
to keep the Windows pid "alive" so that it will not be reused. So, now
we just close parent in child_info_spawn::handle_spawn iff we're not
execing.
In debugging this it bothered me that 'ps' identified a nonactive pid as
active. Part of the reason for this was the 'parent' handle in
child_info was opened in non-Cygwin processes, keeping the pid alive.
That has been kluged around (more changes after 1.7.15) but that didn't
fix the problem. On further investigation, this seems to be caused by
the fact that the shared memory region pid handles were still being
passed to non-cygwin children, keeping the pid alive in a limbo-like
fashion. This was easily fixed by having pinfo::init() consider a
memory region with PID_REAPED as not available.
This fixed the problem where a pid showed up in the list after a user
does something like: "bash$ cmd /c start notepad" but, for some reason,
it does not fix the problem where "bash$ setsid cmd /c start notepad".
That bears investigation after 1.7.15 is released but it is not a
regression and so is not a blocker for the release.
2012-05-03 cgf-000002 2012-05-03 cgf-000002
<1.7.15> <1.7.15>

View File

@ -390,7 +390,6 @@ struct init_cygheap: public mini_cygheap
struct _cygtls **threadlist; struct _cygtls **threadlist;
size_t sthreads; size_t sthreads;
pid_t pid; /* my pid */ pid_t pid; /* my pid */
HANDLE pid_handle; /* handle for my pid */
struct { /* Equivalent to using LIST_HEAD. */ struct { /* Equivalent to using LIST_HEAD. */
struct inode_t *lh_first; struct inode_t *lh_first;
} inode_list; /* Global inode pointer for adv. locking. */ } inode_list; /* Global inode pointer for adv. locking. */

View File

@ -665,10 +665,15 @@ child_info_spawn::handle_spawn ()
ready (true); ready (true);
/* Need to do this after debug_fixup_after_fork_exec or DEBUGGING handling of /* Keep pointer to parent open if we've execed so that pid will not be reused.
Otherwise, we no longer need this handle so close it.
Need to do this after debug_fixup_after_fork_exec or DEBUGGING handling of
handles might get confused. */ handles might get confused. */
CloseHandle (child_proc_info->parent); if (type != _CH_EXEC)
child_proc_info->parent = NULL; {
CloseHandle (child_proc_info->parent);
child_proc_info->parent = NULL;
}
signal_fixup_after_exec (); signal_fixup_after_exec ();
fixup_lockf_after_exec (); fixup_lockf_after_exec ();

View File

@ -77,11 +77,6 @@ pinfo::thisproc (HANDLE h)
else if (!child_proc_info) /* child_proc_info is only set when this process else if (!child_proc_info) /* child_proc_info is only set when this process
was started by another cygwin process */ was started by another cygwin process */
procinfo->start_time = time (NULL); /* Register our starting time. */ procinfo->start_time = time (NULL); /* Register our starting time. */
else if (::cygheap->pid_handle)
{
ForceCloseHandle (::cygheap->pid_handle);
::cygheap->pid_handle = NULL;
}
} }
/* Initialize the process table entry for the current task. /* Initialize the process table entry for the current task.
@ -305,8 +300,9 @@ pinfo::init (pid_t n, DWORD flag, HANDLE h0)
bool created = shloc != SH_JUSTOPEN; bool created = shloc != SH_JUSTOPEN;
if ((procinfo->process_state & PID_INITIALIZING) && (flag & PID_NOREDIR) if ((procinfo->process_state & PID_REAPED)
&& cygwin_pid (procinfo->dwProcessId) != procinfo->pid) || ((procinfo->process_state & PID_INITIALIZING) && (flag & PID_NOREDIR)
&& cygwin_pid (procinfo->dwProcessId) != procinfo->pid))
{ {
set_errno (ESRCH); set_errno (ESRCH);
break; break;

View File

@ -22,3 +22,8 @@ Bug fixes:
- Avoid "WARNING: Couldn't compute FAST_CWD pointer." message on - Avoid "WARNING: Couldn't compute FAST_CWD pointer." message on
Windows 8 Customer Preview 32 bit. Windows 8 Customer Preview 32 bit.
Fixes: http://cygwin.com/ml/cygwin/2012-04/msg00616.html Fixes: http://cygwin.com/ml/cygwin/2012-04/msg00616.html
- Don't make Cygwin wait for all children of a non-cygwin child program.
Fixes: http://cygwin.com/ml/cygwin/2012-05/msg00063.html,
http://cygwin.com/ml/cygwin/2012-05/msg00075.html

View File

@ -517,17 +517,6 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
myself->sendsig = NULL; myself->sendsig = NULL;
reset_sendsig = true; reset_sendsig = true;
} }
/* Save a copy of a handle to the current process around the first time we
exec so that the pid will not be reused. Why did I stop cygwin from
generating its own pids again? */
if (::cygheap->pid_handle)
/* already done previously */;
else if (DuplicateHandle (GetCurrentProcess (), GetCurrentProcess (),
GetCurrentProcess (), &::cygheap->pid_handle,
PROCESS_QUERY_INFORMATION, TRUE, 0))
ProtectHandleINH (::cygheap->pid_handle);
else
system_printf ("duplicate to pid_handle failed, %E");
} }
if (null_app_name) if (null_app_name)
@ -613,6 +602,19 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
else else
wr_proc_pipe = my_wr_proc_pipe; wr_proc_pipe = my_wr_proc_pipe;
/* Don't allow child to inherit these handles if it's not a Cygwin program.
wr_proc_pipe will be injected later. parent won't be used by the child
so there is no reason for the child to have it open as it can confuse
ps into thinking that children of windows processes are all part of
the same "execed" process.
FIXME: Someday, make it so that parent is never created when starting
non-Cygwin processes. */
if (!iscygwin ())
{
SetHandleInformation (wr_proc_pipe, HANDLE_FLAG_INHERIT, 0);
SetHandleInformation (parent, HANDLE_FLAG_INHERIT, 0);
}
/* When ruid != euid we create the new process under the current original /* When ruid != euid we create the new process under the current original
account and impersonate in child, this way maintaining the different account and impersonate in child, this way maintaining the different
effective vs. real ids. effective vs. real ids.
@ -734,6 +736,12 @@ loop:
myself->exec_sendsig = NULL; myself->exec_sendsig = NULL;
} }
myself->process_state &= ~PID_NOTCYGWIN; myself->process_state &= ~PID_NOTCYGWIN;
/* Reset handle inheritance to default when the execution of a non-Cygwin
process fails. Only need to do this for _P_OVERLAY since the handle will
be closed otherwise. Don't need to do this for 'parent' since it will
be closed in every case. See FIXME above. */
if (!real_path.iscygexec () && mode == _P_OVERLAY)
SetHandleInformation (wr_proc_pipe, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT);
if (wr_proc_pipe == my_wr_proc_pipe) if (wr_proc_pipe == my_wr_proc_pipe)
wr_proc_pipe = NULL; /* We still own it: don't nuke in destructor */ wr_proc_pipe = NULL; /* We still own it: don't nuke in destructor */
res = -1; res = -1;
@ -755,7 +763,7 @@ loop:
cygpid = myself->pid; cygpid = myself->pid;
/* We print the original program name here so the user can see that too. */ /* We print the original program name here so the user can see that too. */
syscall_printf ("%d = child_info_spawn::worker(%s, %.9500s)", syscall_printf ("pid %d, prog_arg %s, cmd line %.9500s)",
rc ? cygpid : (unsigned int) -1, prog_arg, one_line.buf); rc ? cygpid : (unsigned int) -1, prog_arg, one_line.buf);
/* Name the handle similarly to proc_subproc. */ /* Name the handle similarly to proc_subproc. */
@ -815,6 +823,12 @@ loop:
/* Start the child running */ /* Start the child running */
if (c_flags & CREATE_SUSPENDED) if (c_flags & CREATE_SUSPENDED)
{ {
/* Inject a non-inheritable wr_proc_pipe handle into child so that we
can accurately track when the child exits without keeping this
process waiting around for it to exit. */
if (!iscygwin ())
DuplicateHandle (GetCurrentProcess (), wr_proc_pipe, pi.hProcess, NULL,
0, false, DUPLICATE_SAME_ACCESS);
ResumeThread (pi.hThread); ResumeThread (pi.hThread);
if (iscygwin ()) if (iscygwin ())
strace.write_childpid (pi.dwProcessId); strace.write_childpid (pi.dwProcessId);
@ -827,7 +841,9 @@ loop:
if ((mode == _P_DETACH || mode == _P_NOWAIT) && !iscygwin ()) if ((mode == _P_DETACH || mode == _P_NOWAIT) && !iscygwin ())
synced = false; synced = false;
else else
synced = sync (pi.dwProcessId, pi.hProcess, INFINITE); /* Just mark a non-cygwin process as 'synced'. We will still eventually
wait for it to exit in maybe_set_exit_code_from_windows(). */
synced = iscygwin () ? sync (pi.dwProcessId, pi.hProcess, INFINITE) : true;
switch (mode) switch (mode)
{ {