diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index 02e6931bf..39247a970 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,23 @@ +2012-06-03 Christopher Faylor + + * DevNotes: Add entry cgf-000011. + * fhandler.h (fhandler_base::refcnt): Delete. + (fhandler_base::inc_refcnt): New function. + (fhandler_base::dec_refcnt): New function. + * cygheap.h (cygheap_fdnew::~cygheap_fdnew): Accommodate split of + refcnt to inc_refcnt/dec_refcnt. + (cygheap_fdget::cygheap_fdget): Ditto. + (cygheap_fdget::~cygheap_fdget::cygheap_fdget): Ditto. + * dtable.cc (dtable::release): Ditto. + (cygwin_attach_handle_to_fd): Ditto. + (dtable::init_std_file_from_handle): Ditto. + (dtable::dup3): On success, return with fdtab locked. + * dtable.h (dtable): Add dup_finish as a friend. + * syscalls.cc (dup_finish): Define new function. Increment refcnt + while fdtab is locked. + (dup2): Use common dup_finish() to perform dup operation. + (dup3): Ditto. + 2012-06-03 Corinna Vinschen * globals.cc (ro_u_refs): New R/O unicode string. diff --git a/winsup/cygwin/DevNotes b/winsup/cygwin/DevNotes index 778a5d059..a311256e2 100644 --- a/winsup/cygwin/DevNotes +++ b/winsup/cygwin/DevNotes @@ -1,3 +1,28 @@ +2012-06-02 cgf-000011 + +The refcnt handling was tricky to get right but I had convinced myself +that the refcnt's were always incremented/decremented under a lock. +Corinna's 2012-05-23 change to refcnt exposed a potential problem with +dup handling where the fdtab could be updated while not locked. + +That should be fixed by this change but, on closer examination, it seems +ilke there are many places where it is possible for the refcnt to be +updated while the fdtab is not locked since the default for +cygheap_fdget is to not lock the fdtab (and that should be the default - +you can't have read holding a lock). + +Since refcnt was only ever called with 1 or -1, I broke it up into two +functions but kept the Interlocked* operation. Incrementing a variable +should not be as racy as adding an arbitrary number to it but we have +InterlockedIncrement/InterlockedDecrement for a reason so I kept the +Interlocked operation here. + +In the meantime, I'll be mulling over whether the refcnt operations are +actually safe as they are. Maybe just ensuring that they are atomically +updated is enough since they control the destruction of an fh. If I got +the ordering right with incrementing and decrementing then that should +be adequate. + 2012-06-02 cgf-000010 <1.7.16> diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h index 4895019aa..f2b0cd9ec 100644 --- a/winsup/cygwin/cygheap.h +++ b/winsup/cygwin/cygheap.h @@ -458,7 +458,7 @@ class cygheap_fdnew : public cygheap_fdmanip ~cygheap_fdnew () { if (cygheap->fdtab[fd]) - cygheap->fdtab[fd]->refcnt (1); + cygheap->fdtab[fd]->inc_refcnt (); } void operator = (fhandler_base *fh) {cygheap->fdtab[fd] = fh;} }; @@ -476,7 +476,7 @@ public: this->fd = fd; locked = lockit; fh = cygheap->fdtab[fd]; - fh->refcnt (1); + fh->inc_refcnt (); } else { @@ -491,7 +491,7 @@ public: } ~cygheap_fdget () { - if (fh && fh->refcnt (-1) <= 0) + if (fh && fh->dec_refcnt () <= 0) { debug_only_printf ("deleting fh %p", fh); delete fh; diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc index fc3a502f9..059d782d0 100644 --- a/winsup/cygwin/dtable.cc +++ b/winsup/cygwin/dtable.cc @@ -243,7 +243,7 @@ dtable::release (int fd) { if (fds[fd]->need_fixup_before ()) dec_need_fixup_before (); - fds[fd]->refcnt (-1); + fds[fd]->dec_refcnt (); fds[fd] = NULL; if (fd <= 2) set_std_handle (fd); @@ -259,7 +259,7 @@ cygwin_attach_handle_to_fd (char *name, int fd, HANDLE handle, mode_t bin, if (!fh) return -1; cygheap->fdtab[fd] = fh; - cygheap->fdtab[fd]->refcnt (1); + cygheap->fdtab[fd]->inc_refcnt (); fh->init (handle, myaccess, bin ?: fh->pc_binmode ()); return fd; } @@ -398,7 +398,7 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle) fh->open_setup (openflags); fh->usecount = 0; cygheap->fdtab[fd] = fh; - cygheap->fdtab[fd]->refcnt (1); + cygheap->fdtab[fd]->inc_refcnt (); set_std_handle (fd); paranoid_printf ("fd %d, handle %p", fd, handle); } @@ -713,6 +713,7 @@ dtable::dup3 (int oldfd, int newfd, int flags) MALLOC_CHECK; debug_printf ("dup3 (%d, %d, %p)", oldfd, newfd, flags); lock (); + bool do_unlock = true; if (not_open (oldfd)) { @@ -760,6 +761,7 @@ dtable::dup3 (int oldfd, int newfd, int flags) goto done; } + do_unlock = false; fds[newfd] = newfh; if ((res = newfd) <= 2) @@ -767,7 +769,8 @@ dtable::dup3 (int oldfd, int newfd, int flags) done: MALLOC_CHECK; - unlock (); + if (do_unlock) + unlock (); syscall_printf ("%R = dup3(%d, %d, %p)", res, oldfd, newfd, flags); return res; diff --git a/winsup/cygwin/dtable.h b/winsup/cygwin/dtable.h index 288b4767f..7b4cd5d62 100644 --- a/winsup/cygwin/dtable.h +++ b/winsup/cygwin/dtable.h @@ -89,6 +89,7 @@ public: void fixup_before_fork (DWORD win_proc_id); friend void dtable_init (); friend void __stdcall close_all_files (bool); + friend int dup_finish (int, int, int); friend class fhandler_disk_file; friend class cygheap_fdmanip; friend class cygheap_fdget; diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 84f6e4c3f..72f8d8429 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -182,15 +182,8 @@ class fhandler_base HANDLE read_state; public: - long refcnt(long i = 0) - { - debug_only_printf ("%p, %s, i %d, refcnt %ld", this, get_name (), i, _refcnt + i); - /* This MUST be an interlocked operation. If multiple threads access the - same descriptor in quick succession, a context switch can interrupt - the += operation and we wrongly end up with a refcnt of 0 in the - cygheap_fdget destructor. */ - return i ? InterlockedAdd (&_refcnt, i) : _refcnt; - } + long inc_refcnt () {return InterlockedIncrement (&_refcnt);} + long dec_refcnt () {return InterlockedDecrement (&_refcnt);} class fhandler_base *archetype; int usecount; diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc index 657616ad9..3c0a02671 100644 --- a/winsup/cygwin/syscalls.cc +++ b/winsup/cygwin/syscalls.cc @@ -126,6 +126,18 @@ dup (int fd) return res; } +inline int +dup_finish (int oldfd, int newfd, int flags) +{ + int res; + if ((res = cygheap->fdtab.dup3 (oldfd, newfd, flags)) == newfd) + { + cygheap_fdget (newfd)->inc_refcnt (); + cygheap->fdtab.unlock (); /* dup3 exits with lock set on success */ + } + return res; +} + extern "C" int dup2 (int oldfd, int newfd) { @@ -140,8 +152,8 @@ dup2 (int oldfd, int newfd) cygheap_fdget cfd (oldfd); res = (cfd >= 0) ? oldfd : -1; } - else if ((res = cygheap->fdtab.dup3 (oldfd, newfd, 0)) == newfd) - cygheap->fdtab[newfd]->refcnt (1); + else + res = dup_finish (oldfd, newfd, 0); syscall_printf ("%R = dup2(%d, %d)", res, oldfd, newfd); return res; @@ -162,8 +174,8 @@ dup3 (int oldfd, int newfd, int flags) set_errno (cfd < 0 ? EBADF : EINVAL); res = -1; } - else if ((res = cygheap->fdtab.dup3 (oldfd, newfd, flags)) == newfd) - cygheap->fdtab[newfd]->refcnt (1); + else + res = dup_finish (oldfd, newfd, flags); syscall_printf ("%R = dup3(%d, %d, %p)", res, oldfd, newfd, flags); return res;