diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index 0253d982d..83316d5f3 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,22 @@ +2004-06-07 Christopher Faylor + + * dtable.cc (dtable::find_fifo): Release lock after fifo found (still + racy). + * fhandler.h (fhandler_fifo::get_io_handle): New fifo-specific method. + * fhandler_fifo.cc (fhandler_fifo::close): Close output_handle only if + it is open. + (fhandler_fifo::open_not_mine): Reorganize slightly. Don't call _pinfo + methods when the fifo is owned by me or suffer dtable lock_cs deadlock. + (fhandler_fifo::open): Call open_not_mine first, otherwise open myself + (racy). + * pinfo.cc (_pinfo::commune_recv): Duplicate fifo handles here in + requesting processes arena to avoid one potential race (of many). + (_pinfo::commune_send): Move all PICOM_FIFO code under one case + statement. + + * thread.cc (pthread::init_mainthread) Use existing hMainProc handle + rather than calling GetCurrentProcess. + 2004-06-04 Christopher Faylor * winbase.h (ilockincr): Add more neverending changes from the diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc index 532910410..c9f0cde7e 100644 --- a/winsup/cygwin/dtable.cc +++ b/winsup/cygwin/dtable.cc @@ -557,13 +557,18 @@ fhandler_fifo * dtable::find_fifo (const char *path) { lock (); + fhandler_fifo *fh_res = NULL; for (unsigned i = 0; i < size; i++) { fhandler_base *fh = fds[i]; if (fh && fh->isfifo () && strcmp (path, fh->get_win32_name ()) == 0) - return (fhandler_fifo *) fh; + { + fh_res = (fhandler_fifo *) fh; + break; + } } - return NULL; + unlock (); + return fh_res; } select_record * diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 1e8129842..c5f99f07a 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -324,6 +324,7 @@ class fhandler_base bool is_fs_special () {return pc.is_fs_special ();} bool device_access_denied (int) __attribute__ ((regparm (2))); int fhaccess (int flags) __attribute__ ((regparm (2))); + friend class fhandler_fifo; }; class fhandler_socket: public fhandler_base @@ -454,6 +455,7 @@ class fhandler_fifo: public fhandler_pipe HANDLE owner; // You can't have too many mutexes, now, can you? long read_use; long write_use; + virtual HANDLE& get_io_handle () { return io_handle ?: output_handle; } public: fhandler_fifo (); int open (int flags, mode_t mode = 0); diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc index 8fd064b39..6026deff8 100644 --- a/winsup/cygwin/fhandler_fifo.cc +++ b/winsup/cygwin/fhandler_fifo.cc @@ -60,7 +60,8 @@ int fhandler_fifo::close () { fhandler_pipe::close (); - CloseHandle (get_output_handle ()); + if (get_output_handle ()) + CloseHandle (get_output_handle ()); set_use (-1); return 0; } @@ -72,44 +73,53 @@ fhandler_fifo::open_not_mine (int flags) winpids pids; static int flagtypes[] = {DUMMY_O_RDONLY | O_RDWR, O_WRONLY | O_APPEND | O_RDWR}; HANDLE *usehandles[2] = {&(get_handle ()), &(get_output_handle ())}; - int res; + int res = 0; + int testflags = (flags & (O_RDWR | O_WRONLY | O_APPEND)) ?: DUMMY_O_RDONLY; for (unsigned i = 0; i < pids.npids; i++) { _pinfo *p = pids[i]; - HANDLE hp = OpenProcess (PROCESS_DUP_HANDLE, false, p->dwProcessId); - if (!hp) - { - __seterrno (); - goto err; - } - - HANDLE handles[2]; commune_result r; - r = p->commune_send (PICOM_FIFO, get_win32_name ()); - if (r.handles[0] == NULL) - continue; // process doesn't own fifo - - flags = (flags & (O_RDWR | O_WRONLY | O_APPEND)) ?: DUMMY_O_RDONLY; - for (int i = 0; i < 2; i++) + if (p->pid != myself->pid) { - if (!(flags & flagtypes[i])) + r = p->commune_send (PICOM_FIFO, get_win32_name ()); + if (r.handles[0] == NULL) + continue; // process doesn't own fifo + } + else + { + /* FIXME: racy? */ + fhandler_fifo *fh = cygheap->fdtab.find_fifo (get_win32_name ()); + if (!fh) continue; - if (!DuplicateHandle (hp, r.handles[i], hMainProc, usehandles[i], 0, - false, DUPLICATE_SAME_ACCESS)) + if (!DuplicateHandle (hMainProc, fh->get_handle (), hMainProc, + &r.handles[0], 0, false, DUPLICATE_SAME_ACCESS)) { - debug_printf ("couldn't duplicate handle %d/%p, %E", i, handles[i]); __seterrno (); - goto err; + goto out; } - - if (i == 0) + if (!DuplicateHandle (hMainProc, fh->get_handle (), hMainProc, + &r.handles[1], 0, false, DUPLICATE_SAME_ACCESS)) { - read_state = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL); - need_fork_fixup (true); + CloseHandle (r.handles[0]); + __seterrno (); + goto out; } } - CloseHandle (hp); + + for (int i = 0; i < 2; i++) + if (!(testflags & flagtypes[i])) + CloseHandle (r.handles[i]); + else + { + *usehandles[i] = r.handles[i]; + + if (i == 0) + { + read_state = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL); + need_fork_fixup (true); + } + } res = 1; set_flags (flags); @@ -118,10 +128,6 @@ fhandler_fifo::open_not_mine (int flags) set_errno (EAGAIN); -err: - res = 0; - debug_printf ("failed"); - out: debug_printf ("res %d", res); return res; @@ -132,26 +138,31 @@ fhandler_fifo::open (int flags, mode_t) { int res = 1; + set_io_handle (NULL); + set_output_handle (NULL); + if (open_not_mine (flags)) + goto out; + fhandler_pipe *fhs[2]; if (create (fhs, 0, flags, true)) - goto errnout; - - set_flags (fhs[0]->get_flags ()); - set_io_handle (fhs[0]->get_handle ()); - set_output_handle (fhs[1]->get_handle ()); - guard = fhs[0]->guard; - read_state = fhs[0]->read_state; - writepipe_exists = fhs[1]->writepipe_exists; - orig_pid = fhs[0]->orig_pid; - id = fhs[0]->id; - delete (fhs[0]); - delete (fhs[1]); - set_use (1); - goto out; - -errnout: - __seterrno (); - res = 0; + { + __seterrno (); + res = 0; + } + else + { + set_flags (fhs[0]->get_flags ()); + set_io_handle (fhs[0]->get_handle ()); + set_output_handle (fhs[1]->get_handle ()); + guard = fhs[0]->guard; + read_state = fhs[0]->read_state; + writepipe_exists = fhs[1]->writepipe_exists; + orig_pid = fhs[0]->orig_pid; + id = fhs[0]->id; + delete (fhs[0]); + delete (fhs[1]); + set_use (1); + } out: debug_printf ("returning %d, errno %d", res, get_errno ()); diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc index 0ed74009f..79d4c60d4 100644 --- a/winsup/cygwin/pinfo.cc +++ b/winsup/cygwin/pinfo.cc @@ -351,11 +351,11 @@ _pinfo::commune_recv () return; } - CloseHandle (hp); hello_pid = 0; if (!ReadFile (__fromthem, &code, sizeof code, &nr, NULL) || nr != sizeof code) { + CloseHandle (hp); /* __seterrno ();*/ // this is run from the signal thread, so don't set errno goto out; } @@ -368,6 +368,8 @@ _pinfo::commune_recv () CloseHandle (__fromthem); __fromthem = NULL; extern int __argc_safe; const char *argv[__argc_safe + 1]; + + CloseHandle (hp); for (int i = 0; i < __argc_safe; i++) { if (IsBadStringPtr (__argv[i], INT32_MAX)) @@ -403,6 +405,7 @@ _pinfo::commune_recv () if (!ReadFile (__fromthem, &len, sizeof len, &nr, NULL) || nr != sizeof len) { + CloseHandle (hp); /* __seterrno ();*/ // this is run from the signal thread, so don't set errno goto out; } @@ -410,6 +413,7 @@ _pinfo::commune_recv () if (!ReadFile (__fromthem, path, len, &nr, NULL) || nr != len) { + CloseHandle (hp); /* __seterrno ();*/ // this is run from the signal thread, so don't set errno goto out; } @@ -422,8 +426,16 @@ _pinfo::commune_recv () { it[0] = fh->get_handle (); it[1] = fh->get_output_handle (); + for (int i = 0; i < 2; i++) + if (!DuplicateHandle (hMainProc, it[i], hp, &it[i], 0, false, + DUPLICATE_SAME_ACCESS)) + { + it[0] = it[1] = NULL; /* FIXME: possibly left a handle open in child? */ + break; + } } + CloseHandle (hp); if (!WriteFile (__tothem, it, sizeof (it), &nr, NULL)) { /*__seterrno ();*/ // this is run from the signal thread, so don't set errno @@ -442,7 +454,7 @@ out: CloseHandle (__tothem); } -#define PIPEBUFSIZE (16 * sizeof (DWORD)) +#define PIPEBUFSIZE (4096 * sizeof (DWORD)) commune_result _pinfo::commune_send (DWORD code, ...) @@ -485,21 +497,6 @@ _pinfo::commune_send (DWORD code, ...) goto err; } - switch (code) - { - case PICOM_FIFO: - { - char *path = va_arg (args, char *); - size_t len = strlen (path) + 1; - if (!WriteFile (tothem, path, len, &nr, NULL) || nr != len) - { - __seterrno (); - goto err; - } - break; - } - } - if (sig_send (this, __SIGCOMMUNE)) goto err; @@ -550,10 +547,25 @@ _pinfo::commune_send (DWORD code, ...) break; case PICOM_FIFO: { + char *path = va_arg (args, char *); + size_t len = strlen (path) + 1; + if (!WriteFile (tothem, &len, sizeof (len), &nr, NULL) + || nr != sizeof (len)) + { + __seterrno (); + goto err; + } + if (!WriteFile (tothem, path, len, &nr, NULL) || nr != len) + { + __seterrno (); + goto err; + } + DWORD x = ReadFile (fromthem, res.handles, sizeof (res.handles), &nr, NULL); - WriteFile (tothem, &x, sizeof (x), &x, NULL); + (void) WriteFile (tothem, &x, sizeof (x), &x, NULL); if (!x) goto err; + if (nr != sizeof (res.handles)) { set_errno (EPIPE); diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc index 4cac624b8..a248b2604 100644 --- a/winsup/cygwin/thread.cc +++ b/winsup/cygwin/thread.cc @@ -178,9 +178,8 @@ pthread::init_mainthread () set_tls_self_pointer (thread); thread->thread_id = GetCurrentThreadId (); - if (!DuplicateHandle (GetCurrentProcess (), GetCurrentThread (), - GetCurrentProcess (), &thread->win32_obj_id, - 0, FALSE, DUPLICATE_SAME_ACCESS)) + if (!DuplicateHandle (hMainProc, GetCurrentThread (), hMainProc, + &thread->win32_obj_id, 0, FALSE, DUPLICATE_SAME_ACCESS)) api_fatal ("failed to create mainthread handle"); if (!thread->create_cancel_event ()) api_fatal ("couldn't create cancel event for main thread");