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>
This commit is contained in:
Corinna Vinschen 2020-03-23 21:06:00 +01:00
parent a1f0585454
commit 2a4b1de773
4 changed files with 56 additions and 81 deletions

View File

@ -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

View File

@ -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);
}

View File

@ -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;
return s;
}
if (WaitCommEvent (get_handle_cyg (), &event, &io_status))
{
else if (WaitCommEvent (get_handle_cyg (), &s->fh_data_serial->event,
&s->fh_data_serial->ov))
s->read_ready = true;
return s;
}
if (GetLastError () == ERROR_IO_PENDING)
{
s->h = io_status.hEvent;
return s;
}
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)
{
CloseHandle (s->fh_data_serial->ov.hEvent);
delete s->fh_data_serial;
}
return s;
}

View File

@ -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;