From: Corinna Vinschen Date: Mon, 23 Mar 2020 20:06:00 +0000 (+0100) Subject: Cygwin: serial: use per call OVERLAPPED structs X-Git-Tag: cygwin-3_1_5-release~104 X-Git-Url: https://sourceware.org/git/?a=commitdiff_plain;h=2a4b1de773e6159ea8197b6fc3f7e4845479b476;p=newlib-cygwin.git Cygwin: serial: use per call OVERLAPPED structs Sharing the OVERLAPPED struct and event object in there between read and select calls in the fhandler might have been a nice optimization way back when, but it is a dangerous, not thread-safe approach. Fix this by creating per-fhandler, per-call OVERLAPPED structs and event objects on demand. Signed-off-by: Corinna Vinschen --- diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 45e256e77..1c7336370 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -1685,19 +1685,13 @@ class fhandler_serial: public fhandler_base pid_t pgrp_; int rts; /* for Windows 9x purposes only */ int dtr; /* for Windows 9x purposes only */ - DWORD event; /* for select */ public: - OVERLAPPED io_status; - /* Constructor */ fhandler_serial (); int open (int flags, mode_t mode); - int close (); int init (HANDLE h, DWORD a, mode_t flags); - void overlapped_setup (); - int dup (fhandler_base *child, int); void __reg3 raw_read (void *ptr, size_t& ulen); ssize_t __reg3 raw_write (const void *ptr, size_t ulen); int tcsendbreak (int); @@ -1714,8 +1708,6 @@ class fhandler_serial: public fhandler_base } int tcflush (int); bool is_tty () const { return true; } - void fixup_after_fork (HANDLE parent); - void fixup_after_exec (); /* We maintain a pgrp so that tcsetpgrp and tcgetpgrp work, but we don't use it for permissions checking. fhandler_pty_slave does diff --git a/winsup/cygwin/fhandler_serial.cc b/winsup/cygwin/fhandler_serial.cc index c7c412e57..b6d1e4f4c 100644 --- a/winsup/cygwin/fhandler_serial.cc +++ b/winsup/cygwin/fhandler_serial.cc @@ -1,5 +1,6 @@ /* fhandler_serial.cc + This file is part of Cygwin. This software is a copyrighted work licensed under the terms of the @@ -29,17 +30,10 @@ fhandler_serial::fhandler_serial () need_fork_fixup (true); } -void -fhandler_serial::overlapped_setup () -{ - memset (&io_status, 0, sizeof (io_status)); - io_status.hEvent = CreateEvent (&sec_none_nih, TRUE, FALSE, NULL); - ProtectHandle (io_status.hEvent); -} - void __reg3 fhandler_serial::raw_read (void *ptr, size_t& ulen) { + OVERLAPPED ov = { 0 }; DWORD io_err; COMSTAT st; DWORD bytes_to_read, read_bytes; @@ -54,8 +48,9 @@ fhandler_serial::raw_read (void *ptr, size_t& ulen) otherwise we're in polling mode and there's no minimum chars. */ ssize_t minchars = (!is_nonblocking () && vmin_) ? MIN (vmin_, ulen) : 0; - debug_printf ("ulen %ld, vmin_ %u, vtime_ %u, hEvent %p", - ulen, vmin_, vtime_, io_status.hEvent); + debug_printf ("ulen %ld, vmin_ %u, vtime_ %u", ulen, vmin_, vtime_); + + ov.hEvent = CreateEvent (&sec_none_nih, TRUE, FALSE, NULL); do { @@ -89,37 +84,36 @@ fhandler_serial::raw_read (void *ptr, size_t& ulen) bytes_to_read = MIN (st.cbInQue, bytes_to_read); } - ResetEvent (io_status.hEvent); - if (!ReadFile (get_handle (), ptr, bytes_to_read, &read_bytes, - &io_status)) + ResetEvent (ov.hEvent); + if (!ReadFile (get_handle (), ptr, bytes_to_read, &read_bytes, &ov)) { if (GetLastError () != ERROR_IO_PENDING) goto err; if (is_nonblocking ()) { CancelIo (get_handle ()); - if (!GetOverlappedResult (get_handle (), &io_status, &read_bytes, + if (!GetOverlappedResult (get_handle (), &ov, &read_bytes, TRUE)) goto err; } else { - switch (cygwait (io_status.hEvent)) + switch (cygwait (ov.hEvent)) { default: /* Handle an error case from cygwait basically like a cancel condition and see if we got "something" */ CancelIo (get_handle ()); /*FALLTHRU*/ case WAIT_OBJECT_0: - if (!GetOverlappedResult (get_handle (), &io_status, - &read_bytes, TRUE)) + if (!GetOverlappedResult (get_handle (), &ov, &read_bytes, + TRUE)) goto err; debug_printf ("got %u bytes from ReadFile", read_bytes); break; case WAIT_SIGNALED: CancelIo (get_handle ()); - if (!GetOverlappedResult (get_handle (), &io_status, - &read_bytes, TRUE)) + if (!GetOverlappedResult (get_handle (), &ov, &read_bytes, + TRUE)) goto err; /* Only if no bytes read, return with EINTR. */ if (!tot && !read_bytes) @@ -133,8 +127,7 @@ fhandler_serial::raw_read (void *ptr, size_t& ulen) goto out; case WAIT_CANCELED: CancelIo (get_handle ()); - GetOverlappedResult (get_handle (), &io_status, &read_bytes, - TRUE); + GetOverlappedResult (get_handle (), &ov, &read_bytes, TRUE); debug_printf ("thread canceled"); pthread::static_cancel_self (); /*NOTREACHED*/ @@ -169,6 +162,7 @@ fhandler_serial::raw_read (void *ptr, size_t& ulen) while (ulen > 0 && minchars > 0 && vtime_ == 0); out: + CloseHandle (ov.hEvent); ulen = (size_t) tot; if (is_nonblocking () && ulen == 0) { @@ -266,8 +260,6 @@ fhandler_serial::open (int flags, mode_t mode) SetCommMask (get_handle (), EV_RXCHAR); - overlapped_setup (); - memset (&to, 0, sizeof (to)); SetCommTimeouts (get_handle (), &to); @@ -318,13 +310,6 @@ fhandler_serial::open (int flags, mode_t mode) return res; } -int -fhandler_serial::close () -{ - ForceCloseHandle (io_status.hEvent); - return fhandler_base::close (); -} - /* tcsendbreak: POSIX 7.2.2.1 */ /* Break for 250-500 milliseconds if duration == 0 */ /* Otherwise, units for duration are undefined */ @@ -1142,28 +1127,3 @@ fhandler_serial::tcgetattr (struct termios *t) return 0; } - -void -fhandler_serial::fixup_after_fork (HANDLE parent) -{ - if (close_on_exec ()) - fhandler_base::fixup_after_fork (parent); - overlapped_setup (); - debug_printf ("io_status.hEvent %p", io_status.hEvent); -} - -void -fhandler_serial::fixup_after_exec () -{ - if (!close_on_exec ()) - overlapped_setup (); - debug_printf ("io_status.hEvent %p, close_on_exec %d", io_status.hEvent, close_on_exec ()); -} - -int -fhandler_serial::dup (fhandler_base *child, int flags) -{ - fhandler_serial *fhc = (fhandler_serial *) child; - fhc->overlapped_setup (); - return fhandler_base::dup (child, flags); -} diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc index b933a8ca9..b5d19cf31 100644 --- a/winsup/cygwin/select.cc +++ b/winsup/cygwin/select.cc @@ -1481,15 +1481,16 @@ serial_read_cleanup (select_record *s, select_stuff *stuff) { if (s->h) { - fhandler_serial *fh = (fhandler_serial *) s->fh; - HANDLE h = fh->get_handle_cyg (); + HANDLE h = ((fhandler_serial *) s->fh)->get_handle_cyg (); DWORD undefined; if (h) { CancelIo (h); - GetOverlappedResult (h, &fh->io_status, &undefined, TRUE); + GetOverlappedResult (h, &s->fh_data_serial->ov, &undefined, TRUE); } + CloseHandle (s->fh_data_serial->ov.hEvent); + delete s->fh_data_serial; } } @@ -1513,27 +1514,30 @@ fhandler_serial::select_read (select_stuff *ss) s->peek = peek_serial; s->read_selected = true; s->read_ready = false; + + s->fh_data_serial = new (fh_select_data_serial); + s->fh_data_serial->ov.hEvent = CreateEvent (&sec_none_nih, TRUE, FALSE, NULL); + /* This is apparently necessary for the com0com driver. See: http://cygwin.com/ml/cygwin/2009-01/msg00667.html */ - ResetEvent (io_status.hEvent); SetCommMask (get_handle_cyg (), 0); SetCommMask (get_handle_cyg (), EV_RXCHAR); if (ClearCommError (get_handle_cyg (), &io_err, &st) && st.cbInQue) + s->read_ready = true; + else if (WaitCommEvent (get_handle_cyg (), &s->fh_data_serial->event, + &s->fh_data_serial->ov)) + s->read_ready = true; + else if (GetLastError () == ERROR_IO_PENDING) + s->h = s->fh_data_serial->ov.hEvent; + else + select_printf ("WaitCommEvent %E"); + + /* No overlapped operation? Destroy the helper struct */ + if (!s->h) { - s->read_ready = true; - return s; - } - if (WaitCommEvent (get_handle_cyg (), &event, &io_status)) - { - s->read_ready = true; - return s; - } - if (GetLastError () == ERROR_IO_PENDING) - { - s->h = io_status.hEvent; - return s; + CloseHandle (s->fh_data_serial->ov.hEvent); + delete s->fh_data_serial; } - select_printf ("WaitCommEvent %E"); return s; } diff --git a/winsup/cygwin/select.h b/winsup/cygwin/select.h index 886810a0b..083c3c4d3 100644 --- a/winsup/cygwin/select.h +++ b/winsup/cygwin/select.h @@ -9,6 +9,14 @@ details. */ #ifndef _SELECT_H_ #define _SELECT_H_ +struct fh_select_data_serial +{ + DWORD event; + OVERLAPPED ov; + + fh_select_data_serial () : event (0) { memset (&ov, 0, sizeof ov); } +}; + struct select_record { int fd; @@ -25,6 +33,13 @@ struct select_record int (*verify) (select_record *, fd_set *, fd_set *, fd_set *); void (*cleanup) (select_record *, class select_stuff *); struct select_record *next; + /* If an fhandler type needs per-fhandler, per-select data, this union + is the place to add it. First candidate: fhandler_serial. */ + union + { + fh_select_data_serial *fh_data_serial; + void *fh_data_union; /* type-agnostic placeholder for constructor */ + }; void set_select_errno () {__seterrno (); thread_errno = errno;} int saw_error () {return thread_errno;} select_record (int): next (NULL) {} @@ -34,7 +49,7 @@ struct select_record except_ready (false), read_selected (false), write_selected (false), except_selected (false), except_on_write (false), startup (NULL), peek (NULL), verify (NULL), cleanup (NULL), - next (NULL) {} + next (NULL), fh_data_union (NULL) {} #ifdef DEBUGGING void dump_select_record (); #endif @@ -83,6 +98,10 @@ public: bool always_ready, windows_used; select_record start; + /* If an fhandler type needs a singleton per-select datastructure for all + its objects in the descriptor lists, here's the place to be. This is + mainly used to maintain a single thread for all fhandlers of a single + type in the descriptor lists. */ select_pipe_info *device_specific_pipe; select_pipe_info *device_specific_ptys; select_fifo_info *device_specific_fifo;