[newlib-cygwin] Cygwin: serial: use per call OVERLAPPED structs

[newlib-cygwin] Cygwin: serial: use per call OVERLAPPED structs

[Corinna Vinschen]

https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=2a4b1de773e6159ea8197b6fc3f7e4845479b476

commit 2a4b1de773e6159ea8197b6fc3f7e4845479b476
Author: Corinna Vinschen <corinna@vinschen.de>
Date:   Mon Mar 23 21:06:00 2020 +0100

    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 <corinna@vinschen.de>

Diff:
---
 winsup/cygwin/fhandler.h         |  8 -----
 winsup/cygwin/fhandler_serial.cc | 70 +++++++++-------------------------------
 winsup/cygwin/select.cc          | 38 ++++++++++++----------
 winsup/cygwin/select.h           | 21 +++++++++++-
 4 files changed, 56 insertions(+), 81 deletions(-)

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;