diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index ca2768fb0..e9747e750 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,37 @@ +2004-01-23 Christopher Faylor + + * configure.in: Remove NEWVFORK default. + * configure: Regenerate. + * dcrt0.cc: Conditionalize vfork stuff throughout. + * dtable.cc: Ditto. + * perthread.h: Ditto. + * pipe.cc (fhandler_pipe::close): Ditto. + * spawn.cc (spawnve): Ditto. + * syscalls.cc (setsid): Ditto. + * exceptions.cc (sigpacket::process): Use macro to refer to vfork pid. + + * debug.cc (verify_handle): Define new function. + * debug.h (VerifyHandle): Define new macro. + (verify_handle): Declare new function + * fhandler.cc (fhandler_base::dup): Verify that dup'ed handle is not + supposed to be in use. + (fhandler_base::set_inheritance): Ditto. + (fhandler_base::fork_fixup): Ditto. + * fhandler_socket.cc (fhandler_socket::dup): Ditto. + * fhandler_tty.cc (fhandler_tty_slave::open): Ditto. + * net.cc (set_socket_inheritance): Ditto. + * pinfo.cc (pinfo_fixup_after_exec): Ditto. + * sigproc.cc (proc_subproc): Ditto. + (sig_send): Ditto. + * spawn.cc (spawn_guts): Ditto. + * thread.cc (pthread::init_mainthread): Ditto. + * pipe.cc (fhandler_pipe::close): Close read_state with + ForceCloseHandle since it was protected. + (fhandler_pipe::fixup_after_exec): Protect read_state handle. + (fhandler_pipe::dup): Correctly close open handles on error condition. + Verify that dup'ed handle is not supposed to be in use. + (fhandler_pipe::create): Protect read_state. + 2004-01-23 Christopher Faylor * exceptions.cc (sig_handle_tty_stop): Fix boneheaded mistake by using diff --git a/winsup/cygwin/configure b/winsup/cygwin/configure index 418dee19a..9f778b846 100755 --- a/winsup/cygwin/configure +++ b/winsup/cygwin/configure @@ -1924,7 +1924,7 @@ fi case "$vfork" in no) ;; -yes|*) cat >> confdefs.h <> confdefs.h <ctty %p", cygheap->ctty); int usecount = cygheap->ctty->usecount; cygheap->ctty->close (); +#ifndef NEWVFORK + cygheap->ctty = NULL; +#else // FIXME: This code ain't right if (cygheap->ctty_on_hold == cygheap->ctty) cygheap->ctty_on_hold = NULL; if (usecount == 1) @@ -185,6 +188,7 @@ init_cygheap::close_ctty () cygheap->ctty = NULL; debug_printf ("setting cygheap->ctty to NULL"); } +#endif } #define pagetrunc(x) ((void *) (((DWORD) (x)) & ~(4096 - 1))) diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h index be5152771..f1c80e1af 100644 --- a/winsup/cygwin/cygheap.h +++ b/winsup/cygwin/cygheap.h @@ -263,7 +263,9 @@ struct init_cygheap struct sigaction *sigs; fhandler_tty_slave *ctty; /* Current tty */ +#ifdef NEWVFORK fhandler_tty_slave *ctty_on_hold; +#endif struct _threadinfo **threadlist; size_t sthreads; int open_fhs; diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc index 37e52a996..6a08261c1 100644 --- a/winsup/cygwin/dcrt0.cc +++ b/winsup/cygwin/dcrt0.cc @@ -45,10 +45,14 @@ HANDLE NO_COPY hMainProc = (HANDLE) -1; HANDLE NO_COPY hMainThread; per_thread_waitq NO_COPY waitq_storage; +#ifdef NEWVFORK per_thread_vfork NO_COPY vfork_storage; +#endif per_thread NO_COPY *threadstuff[] = {&waitq_storage, +#ifdef NEWVFORK &vfork_storage, +#endif NULL}; bool display_title; @@ -59,7 +63,9 @@ codepage_type current_codepage = ansi_cp; int __argc_safe; int _declspec(dllexport) __argc; char _declspec(dllexport) **__argv; +#ifdef NEWVFORK vfork_save NO_COPY *main_vfork; +#endif static int NO_COPY envc; char NO_COPY **envp; @@ -764,9 +770,10 @@ dll_crt0_1 (char *) Need to do this before any helper threads start. */ debug_init (); +#ifdef NEWVFORK cygheap->fdtab.vfork_child_fixup (); - main_vfork = vfork_storage.create (); +#endif cygbench ("pre-forkee"); if (user_data->forkee) @@ -993,12 +1000,14 @@ do_exit (int status) { syscall_printf ("do_exit (%d), exit_state %d", status, exit_state); +#ifdef NEWVFORK vfork_save *vf = vfork_storage.val (); if (vf != NULL && vf->pid < 0) { exit_state = ES_NOT_EXITING; vf->restore_exit (status); } +#endif EnterCriticalSection (&exit_lock); muto::set_exiting_thread (); diff --git a/winsup/cygwin/debug.cc b/winsup/cygwin/debug.cc index b02f4da5a..2a6b86735 100644 --- a/winsup/cygwin/debug.cc +++ b/winsup/cygwin/debug.cc @@ -78,7 +78,18 @@ out: return hl; } -#ifdef DEBUGGING_AND_FDS_PROTECTED +void +verify_handle (const char *func, int ln, HANDLE h) +{ + handle_list *hl = find_handle (h); + if (!hl) + return; + system_printf ("%s:%d - multiple attempts to add handle %p", func, ln, h); + + system_printf (" previously allocated by %s:%d(%s<%p>) winpid %d", + hl->func, hl->ln, hl->name, hl->h, hl->pid); +} + void setclexec (HANDLE oh, HANDLE nh, bool not_inheriting) { @@ -90,7 +101,6 @@ setclexec (HANDLE oh, HANDLE nh, bool not_inheriting) hl->h = nh; } } -#endif /* Create a new handle record */ static handle_list * __stdcall diff --git a/winsup/cygwin/debug.h b/winsup/cygwin/debug.h index 8941bae75..f4f00c60e 100644 --- a/winsup/cygwin/debug.h +++ b/winsup/cygwin/debug.h @@ -37,6 +37,7 @@ details. */ # define debug_init() do {} while (0) # define setclexec(h, nh, b) do {} while (0) # define debug_fixup_after_fork_exec() do {} while (0) +# define VerifyHandle(h) do {} while (0) #else @@ -59,10 +60,13 @@ details. */ # define ProtectHandleINH(h) add_handle (__PRETTY_FUNCTION__, __LINE__, (h), #h, 1) # define ProtectHandle1INH(h, n) add_handle (__PRETTY_FUNCTION__, __LINE__, (h), #n, 1) # define ProtectHandle2INH(h, n) add_handle (__PRETTY_FUNCTION__, __LINE__, (h), n, 1) +# define VerifyHandle(h) verify_handle (__PRETTY_FUNCTION__, __LINE__, (h)) void debug_init (); void __stdcall add_handle (const char *, int, HANDLE, const char *, bool = false) __attribute__ ((regparm (3))); +void __stdcall verify_handle (const char *, int, HANDLE) + __attribute__ ((regparm (3))); bool __stdcall close_handle (const char *, int, HANDLE, const char *, bool) __attribute__ ((regparm (3))); void __stdcall cygbench (const char *s) __attribute__ ((regparm (1))); diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc index d943e166b..e7e6f1732 100644 --- a/winsup/cygwin/dtable.cc +++ b/winsup/cygwin/dtable.cc @@ -705,6 +705,7 @@ dtable::fixup_after_fork (HANDLE parent) } } +#ifdef NEWVFORK int dtable::vfork_child_dup () { @@ -803,6 +804,7 @@ dtable::vfork_child_fixup () return; } +#endif /*NEWVFORK*/ #define DEVICE_PREFIX "\\device\\" #define DEVICE_PREFIX_LEN sizeof (DEVICE_PREFIX) - 1 diff --git a/winsup/cygwin/dtable.h b/winsup/cygwin/dtable.h index 8f3cbea40..16aefa5d2 100644 --- a/winsup/cygwin/dtable.h +++ b/winsup/cygwin/dtable.h @@ -22,7 +22,9 @@ class dtable { muto *lock_cs; fhandler_base **fds; +#ifdef NEWVFORK fhandler_base **fds_on_hold; +#endif fhandler_base **archetypes; unsigned narchetypes; unsigned farchetype; @@ -74,7 +76,9 @@ public: void stdio_init (); void get_debugger_info (); void set_file_pointers_for_exec (); +#ifdef NEWVFORK bool in_vfork_cleanup () {return fds_on_hold == fds;} +#endif fhandler_fifo *find_fifo (ATOM); fhandler_base *find_archetype (device& dev); fhandler_base **add_archetype (); diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc index e6dc906a0..96e05e247 100644 --- a/winsup/cygwin/exceptions.cc +++ b/winsup/cygwin/exceptions.cc @@ -957,7 +957,7 @@ sigpacket::process () bool special_case; bool insigwait_mask; insigwait_mask = masked = false; - if (special_case = (main_vfork->pid || ISSTATE (myself, PID_STOPPED))) + if (special_case = (VFORKPID || ISSTATE (myself, PID_STOPPED))) /* nothing to do */; else if (tls && sigismember (&tls->sigwait_mask, si.si_signo)) insigwait_mask = true; diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc index 0aad893a3..d5619bd63 100644 --- a/winsup/cygwin/fhandler.cc +++ b/winsup/cygwin/fhandler.cc @@ -1044,6 +1044,7 @@ fhandler_base::dup (fhandler_base *child) return -1; } + VerifyHandle (nh); child->set_io_handle (nh); } return 0; @@ -1209,17 +1210,17 @@ fhandler_dev_null::dump (void) void fhandler_base::set_inheritance (HANDLE &h, int not_inheriting) { -#ifdef DEBUGGING_AND_FDS_PROTECTED HANDLE oh = h; -#endif /* Note that we could use SetHandleInformation here but it is not available on all platforms. Test cases seem to indicate that using DuplicateHandle in this fashion does not actually close the original handle, which is what we want. If this changes in the future, we may be forced to use SetHandleInformation on newer OS's */ - if (!DuplicateHandle (hMainProc, h, hMainProc, &h, 0, !not_inheriting, + if (!DuplicateHandle (hMainProc, oh, hMainProc, &h, 0, !not_inheriting, DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE)) debug_printf ("DuplicateHandle failed, %E"); + if (oh != h) + VerifyHandle (h); #ifdef DEBUGGING_AND_FDS_PROTECTED if (h) setclexec (oh, h, not_inheriting); @@ -1229,17 +1230,14 @@ fhandler_base::set_inheritance (HANDLE &h, int not_inheriting) void fhandler_base::fork_fixup (HANDLE parent, HANDLE &h, const char *name) { + HANDLE oh = h; if (/* !is_socket () && */ !get_close_on_exec ()) debug_printf ("handle %p already opened", h); else if (!DuplicateHandle (parent, h, hMainProc, &h, 0, !get_close_on_exec (), DUPLICATE_SAME_ACCESS)) system_printf ("%s - %E, handle %s<%p>", get_name (), name, h); -#ifdef DEBUGGING_AND_FDS_PROTECTED - else if (get_close_on_exec ()) - ProtectHandle (h); /* would have to be fancier than this */ - else - /* ProtectHandleINH (h) */; /* Should already be protected */ -#endif + else if (oh != h) + VerifyHandle (h); } void diff --git a/winsup/cygwin/fhandler_socket.cc b/winsup/cygwin/fhandler_socket.cc index 8d8f9cbb2..559731065 100644 --- a/winsup/cygwin/fhandler_socket.cc +++ b/winsup/cygwin/fhandler_socket.cc @@ -363,6 +363,7 @@ fhandler_socket::dup (fhandler_base *child) __seterrno (); return -1; } + VerifyHandle (nh); fhs->set_io_handle (nh); return 0; } diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc index 4ca8de269..97252734d 100644 --- a/winsup/cygwin/fhandler_tty.cc +++ b/winsup/cygwin/fhandler_tty.cc @@ -553,6 +553,7 @@ fhandler_tty_slave::open (int flags, mode_t) return 0; } + VerifyHandle (from_master_local); if (!DuplicateHandle (tty_owner, get_ttyp ()->to_master, hMainProc, &to_master_local, 0, TRUE, DUPLICATE_SAME_ACCESS)) @@ -561,6 +562,7 @@ fhandler_tty_slave::open (int flags, mode_t) __seterrno (); return 0; } + VerifyHandle (to_master_local); CloseHandle (tty_owner); } diff --git a/winsup/cygwin/net.cc b/winsup/cygwin/net.cc index 1973d8972..f3c0c8c92 100644 --- a/winsup/cygwin/net.cc +++ b/winsup/cygwin/net.cc @@ -130,6 +130,7 @@ set_socket_inheritance (SOCKET sock) system_printf ("DuplicateHandle failed %E"); else debug_printf ("DuplicateHandle succeeded osock %p, sock %p", osock, sock); + VerifyHandle ((HANDLE) sock); return sock; } diff --git a/winsup/cygwin/perthread.h b/winsup/cygwin/perthread.h index add07e3d5..4436168fd 100644 --- a/winsup/cygwin/perthread.h +++ b/winsup/cygwin/perthread.h @@ -43,8 +43,14 @@ public: size_t size () {return sizeof (waitq);} }; -#if defined (NEED_VFORK) +#ifdef NEED_VFORK #include "cygtls.h" +#endif + +#ifndef NEWVFORK +#define VFORKPID 0 +#else +#if defined (NEED_VFORK) class vfork_save { jmp_buf j; @@ -82,7 +88,9 @@ public: }; extern per_thread_vfork vfork_storage; extern vfork_save *main_vfork; +#define VFORKPID main_vfork->pid #endif +#endif /*NEWVFORK*/ extern per_thread_waitq waitq_storage; diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc index 834fa8574..4a01193ae 100644 --- a/winsup/cygwin/pinfo.cc +++ b/winsup/cygwin/pinfo.cc @@ -49,6 +49,7 @@ pinfo_fixup_after_fork () system_printf ("couldn't save current process handle %p, %E", hMainProc); hexec_proc = NULL; } + VerifyHandle (hexec_proc); } /* Initialize the process table. diff --git a/winsup/cygwin/pipe.cc b/winsup/cygwin/pipe.cc index abc9a81a3..82313c492 100644 --- a/winsup/cygwin/pipe.cc +++ b/winsup/cygwin/pipe.cc @@ -89,11 +89,15 @@ fhandler_pipe::close () CloseHandle (guard); if (writepipe_exists) CloseHandle (writepipe_exists); +#ifndef NEWVFORK + if (read_state) +#else // FIXME is this vfork_cleanup test right? Is it responsible for some of // the strange pipe behavior that has been reported in the cygwin mailing // list? if (read_state && !cygheap->fdtab.in_vfork_cleanup ()) - CloseHandle (read_state); +#endif + ForceCloseHandle (read_state); if (get_handle ()) { CloseHandle (get_handle ()); @@ -122,7 +126,10 @@ void fhandler_pipe::fixup_after_exec (HANDLE parent) { if (read_state) - read_state = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL); + { + read_state = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL); + ProtectHandle (read_state); + } } void @@ -139,15 +146,17 @@ fhandler_pipe::fixup_after_fork (HANDLE parent) int fhandler_pipe::dup (fhandler_base *child) { + int res = -1; + fhandler_pipe *ftp = (fhandler_pipe *) child; + ftp->guard = ftp->writepipe_exists = ftp->read_state = NULL; + if (get_handle ()) { - int res = fhandler_base::dup (child); + res = fhandler_base::dup (child); if (res) - return res; + goto err; } - fhandler_pipe *ftp = (fhandler_pipe *) child; - /* FIXME: This leaks handles in the failing condition */ if (guard == NULL) ftp->guard = NULL; @@ -155,7 +164,7 @@ fhandler_pipe::dup (fhandler_base *child) DUPLICATE_SAME_ACCESS)) { debug_printf ("couldn't duplicate guard %p, %E", guard); - return -1; + goto err; } if (writepipe_exists == NULL) @@ -165,7 +174,7 @@ fhandler_pipe::dup (fhandler_base *child) DUPLICATE_SAME_ACCESS)) { debug_printf ("couldn't duplicate writepipe_exists %p, %E", writepipe_exists); - return -1; + goto err; } if (read_state == NULL) @@ -175,12 +184,31 @@ fhandler_pipe::dup (fhandler_base *child) DUPLICATE_SAME_ACCESS)) { debug_printf ("couldn't duplicate read_state %p, %E", writepipe_exists); - return -1; + goto err; } + res = 0; + goto out; + +err: + if (!ftp->guard) + CloseHandle (ftp->guard); + if (!ftp->writepipe_exists) + CloseHandle (ftp->writepipe_exists); + if (!ftp->read_state) + CloseHandle (ftp->read_state); + goto leave; + +out: ftp->id = id; ftp->orig_pid = orig_pid; - return 0; + VerifyHandle (ftp->guard); + VerifyHandle (ftp->writepipe_exists); + VerifyHandle (ftp->read_state); + +leave: + debug_printf ("res %d", res); + return res; } int @@ -208,6 +236,7 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode, bool fif fhs[0]->read_state = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL); fhs[0]->set_need_fork_fixup (); + ProtectHandle1 (fhs[0]->read_state, read_state); res = 0; fhs[0]->create_guard (sa); diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc index dff2c1bd4..20a44a279 100644 --- a/winsup/cygwin/sigproc.cc +++ b/winsup/cygwin/sigproc.cc @@ -320,6 +320,7 @@ proc_subproc (DWORD what, DWORD val) if (!DuplicateHandle (hMainProc, hMainProc, vchild->hProcess, &vchild->ppid_handle, SYNCHRONIZE | PROCESS_DUP_HANDLE, TRUE, 0)) system_printf ("Couldn't duplicate my handle<%p> for pid %d, %E", hMainProc, vchild->pid); + VerifyHandle (vchild->ppid_handle); vchild->ppid = myself->pid; vchild->uid = myself->uid; vchild->gid = myself->gid; @@ -716,17 +717,19 @@ sig_send (_pinfo *p, siginfo_t& si, _threadinfo *tls) __seterrno (); goto out; } + VerifyHandle (hp); for (int i = 0; !p->sendsig && i < 10000; i++) low_priority_sleep (0); if (!DuplicateHandle (hp, p->sendsig, hMainProc, &sendsig, false, 0, DUPLICATE_SAME_ACCESS) || !sendsig) { + CloseHandle (hp); sigproc_printf ("DuplicateHandle failed, %E"); __seterrno (); goto out; } CloseHandle (hp); - pack.wakeup = NULL; + VerifyHandle (sendsig); } sigproc_printf ("sendsig %p, pid %d, signal %d, its_me %d", sendsig, p->pid, si.si_signo, its_me); diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc index 0e288dfa6..653fdd5e1 100644 --- a/winsup/cygwin/spawn.cc +++ b/winsup/cygwin/spawn.cc @@ -404,6 +404,7 @@ spawn_guts (const char * prog_arg, const char *const *argv, return -1; } + VerifyHandle (ciresrv.parent); ciresrv.moreinfo = (cygheap_exec_info *) ccalloc (HEAP_1_EXEC, 1, sizeof (cygheap_exec_info)); ciresrv.moreinfo->old_title = NULL; @@ -612,6 +613,8 @@ spawn_guts (const char * prog_arg, const char *const *argv, &ciresrv.moreinfo->myself_pinfo, 0, TRUE, DUPLICATE_SAME_ACCESS)) ciresrv.moreinfo->myself_pinfo = NULL; + else + VerifyHandle (ciresrv.moreinfo->myself_pinfo); skip_arg_parsing: PROCESS_INFORMATION pi = {NULL, 0, 0, 0}; @@ -901,6 +904,7 @@ spawn_guts (const char * prog_arg, const char *const *argv, 0, FALSE, DUPLICATE_SAME_ACCESS); sigproc_printf ("%d = DuplicateHandle, oldh %p, newh %p", rc, oldh, myself->hProcess); + VerifyHandle (myself->hProcess); if (!rc && my_parent_is_alive ()) { system_printf ("Reparent failed, parent handle %p, %E", h); @@ -958,12 +962,14 @@ spawnve (int mode, const char *path, const char *const *argv, const char *const *envp) { int ret; +#ifdef NEWVFORK vfork_save *vf = vfork_storage.val (); if (vf != NULL && (vf->pid < 0) && mode == _P_OVERLAY) mode = _P_NOWAIT; else vf = NULL; +#endif syscall_printf ("spawnve (%s, %s, %x)", path, argv[0], envp); @@ -984,6 +990,7 @@ spawnve (int mode, const char *path, const char *const *argv, case _P_SYSTEM: subproc_init (); ret = spawn_guts (path, argv, envp, mode); +#ifdef NEWVFORK if (vf) { if (ret > 0) @@ -992,6 +999,7 @@ spawnve (int mode, const char *path, const char *const *argv, vf->restore_pid (ret); } } +#endif break; default: set_errno (EINVAL); diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc index e704d1451..95eeff4f8 100644 --- a/winsup/cygwin/syscalls.cc +++ b/winsup/cygwin/syscalls.cc @@ -300,6 +300,7 @@ getppid () extern "C" pid_t setsid (void) { +#ifdef NEWVFORK vfork_save *vf = vfork_storage.val (); /* This is a horrible, horrible kludge */ if (vf && vf->pid < 0) @@ -312,6 +313,7 @@ setsid (void) } /* assuming that fork was successful */ } +#endif if (myself->pgid == myself->pid) syscall_printf ("hmm. pgid %d pid %d", myself->pgid, myself->pid); diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc index b04dfa116..c61308d15 100644 --- a/winsup/cygwin/thread.cc +++ b/winsup/cygwin/thread.cc @@ -144,6 +144,7 @@ pthread::init_mainthread () api_fatal ("failed to create mainthread handle"); if (!thread->create_cancel_event ()) api_fatal ("couldn't create cancel event for main thread"); + VerifyHandle (thread->win32_obj_id); thread->postcreate (); }