diff options
| -rw-r--r-- | winsup/cygwin/ChangeLog | 14 | ||||
| -rw-r--r-- | winsup/cygwin/DevNotes | 51 | ||||
| -rw-r--r-- | winsup/cygwin/cygheap.h | 1 | ||||
| -rw-r--r-- | winsup/cygwin/dcrt0.cc | 11 | ||||
| -rw-r--r-- | winsup/cygwin/pinfo.cc | 10 | ||||
| -rw-r--r-- | winsup/cygwin/release/1.7.15 | 5 | ||||
| -rw-r--r-- | winsup/cygwin/spawn.cc | 42 |
7 files changed, 110 insertions, 24 deletions
diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index b962d7f91..0e3056e73 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -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> * DevNotes: Add entry cgf-000002. diff --git a/winsup/cygwin/DevNotes b/winsup/cygwin/DevNotes index a40161140..59cda286f 100644 --- a/winsup/cygwin/DevNotes +++ b/winsup/cygwin/DevNotes @@ -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 <1.7.15> diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h index 154ad6a84..4895019aa 100644 --- a/winsup/cygwin/cygheap.h +++ b/winsup/cygwin/cygheap.h @@ -390,7 +390,6 @@ struct init_cygheap: public mini_cygheap struct _cygtls **threadlist; size_t sthreads; pid_t pid; /* my pid */ - HANDLE pid_handle; /* handle for my pid */ struct { /* Equivalent to using LIST_HEAD. */ struct inode_t *lh_first; } inode_list; /* Global inode pointer for adv. locking. */ diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc index 63eaa70dd..aea2a564b 100644 --- a/winsup/cygwin/dcrt0.cc +++ b/winsup/cygwin/dcrt0.cc @@ -665,10 +665,15 @@ child_info_spawn::handle_spawn () 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. */ - CloseHandle (child_proc_info->parent); - child_proc_info->parent = NULL; + if (type != _CH_EXEC) + { + CloseHandle (child_proc_info->parent); + child_proc_info->parent = NULL; + } signal_fixup_after_exec (); fixup_lockf_after_exec (); diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc index a13100cfc..daed1cc7b 100644 --- a/winsup/cygwin/pinfo.cc +++ b/winsup/cygwin/pinfo.cc @@ -77,11 +77,6 @@ pinfo::thisproc (HANDLE h) else if (!child_proc_info) /* child_proc_info is only set when this process was started by another cygwin process */ 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. @@ -305,8 +300,9 @@ pinfo::init (pid_t n, DWORD flag, HANDLE h0) bool created = shloc != SH_JUSTOPEN; - if ((procinfo->process_state & PID_INITIALIZING) && (flag & PID_NOREDIR) - && cygwin_pid (procinfo->dwProcessId) != procinfo->pid) + if ((procinfo->process_state & PID_REAPED) + || ((procinfo->process_state & PID_INITIALIZING) && (flag & PID_NOREDIR) + && cygwin_pid (procinfo->dwProcessId) != procinfo->pid)) { set_errno (ESRCH); break; diff --git a/winsup/cygwin/release/1.7.15 b/winsup/cygwin/release/1.7.15 index 43eb9b56b..4e9e64090 100644 --- a/winsup/cygwin/release/1.7.15 +++ b/winsup/cygwin/release/1.7.15 @@ -22,3 +22,8 @@ Bug fixes: - Avoid "WARNING: Couldn't compute FAST_CWD pointer." message on Windows 8 Customer Preview 32 bit. 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 + diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc index d296597d1..8b5e382c5 100644 --- a/winsup/cygwin/spawn.cc +++ b/winsup/cygwin/spawn.cc @@ -517,17 +517,6 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv, myself->sendsig = NULL; 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) @@ -613,6 +602,19 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv, else 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 account and impersonate in child, this way maintaining the different effective vs. real ids. @@ -734,6 +736,12 @@ loop: myself->exec_sendsig = NULL; } 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) wr_proc_pipe = NULL; /* We still own it: don't nuke in destructor */ res = -1; @@ -755,7 +763,7 @@ loop: cygpid = myself->pid; /* 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); /* Name the handle similarly to proc_subproc. */ @@ -815,6 +823,12 @@ loop: /* Start the child running */ 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); if (iscygwin ()) strace.write_childpid (pi.dwProcessId); @@ -827,7 +841,9 @@ loop: if ((mode == _P_DETACH || mode == _P_NOWAIT) && !iscygwin ()) synced = false; 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) { |