From: Corinna Vinschen <corinna-cygwin@cygwin.com>
To: Marc Hoersken <info@marc-hoersken.de>
Cc: cygwin@cygwin.com
Subject: Re: [PATCH] Fix poll/select signal socket as write ready on connect failure
Date: Thu, 16 Jul 2020 11:25:53 +0200 [thread overview]
Message-ID: <20200716092553.GA3784@calimero.vinschen.de> (raw)
In-Reply-To: <efcd7b07-ccc3-a8a4-04c1-94f63012f042@marc-hoersken.de>
Hi Marc,
On Jul 15 20:54, Marc Hoersken via Cygwin wrote:
> Hello everyone,
>
> I identified an issue related to the way the events FD_CONNECT and FD_CLOSE
> returned by WSAEnumNetworkEvents are currently handled in
> winsup/cygwin/fhandler_socket_inet.cc.
>
> It seems like the code does not handle the fact that those events are
> returned only once for a socket and if not acted upon by the calling program
> may not be received again. This means poll and select are currently not
> consistend about the socket still being writable after a connect failure.
> The first call to poll or select would signal the socket as writable, but
> not any following call. The first call consumes the FD_CONNECT and FD_CLOSE
> events, regardless of the event mask supplied by the calling program. So
> even if the calling program does not care about writability in the first
> call, the events are consumed and following calls checking for writability
> will not be able to detect a connection failure.
> [...]
> As far as I understand calling poll and/or select should not change/reset
> the socket readyness state, therefore I created a simple fix which could be
> used to solve this issue. Attached you will find a suggested patch to make
> sure poll and select always signal writability of a connection failed
> socket. With this patch applied the above example command failed with a
> "Connection refused" as expected.
>
> This patch only fixes the behaviour regarding connection failure (during
> FD_CONNECT), I am not sure if connection closure (during FD_CLOSE) is also
> affected, but I was not able to find code handling the fact that FD_CLOSE is
> only signalled once.
>
> Please take a look and thanks in advance!
Thanks for the patch. I pushed it. But then I got second thoughts in
terms of how to fix the issue. The reason is that the FD_CLOSE problem
shouldn't exist, simply for the fact that we never remove FD_CLOSE from
the events mask, see
https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/fhandler_socket_inet.cc;hb=HEAD#l377
So, rather than setting FD_WRITE at some later point in the code, what
about handling this where the other FD_CONNECT stuff is handled, by
not erasing the FD_CONNECT bit, just like with FD_CLOSE?
diff --git a/winsup/cygwin/fhandler_socket_inet.cc b/winsup/cygwin/fhandler_socket_inet.cc
index e5b0d2d1443e..b64d96225db1 100644
--- a/winsup/cygwin/fhandler_socket_inet.cc
+++ b/winsup/cygwin/fhandler_socket_inet.cc
@@ -354,7 +354,12 @@ fhandler_socket_wsock::evaluate_events (const long event_mask, long &events,
}
else
wsock_events->events |= FD_WRITE;
- wsock_events->events &= ~FD_CONNECT;
+ /* Since FD_CONNECT is only given once, we have to keep FD_CONNECT
+ for connection failed sockets to have consistent behaviour in
+ programs calling poll/select multiple times. Example test to
+ non-listening port: curl -v 127.0.0.1:47 */
+ if (connect_state () != connect_failed)
+ wsock_events->events &= ~FD_CONNECT;
wsock_events->connect_errorcode = 0;
}
/* This test makes accept/connect behave as on Linux when accept/connect
@@ -376,12 +381,6 @@ fhandler_socket_wsock::evaluate_events (const long event_mask, long &events,
if (erase)
wsock_events->events &= ~(events & ~(FD_WRITE | FD_CLOSE));
}
- /* Since FD_CONNECT is only given once, we manually need to set
- FD_WRITE for connection failed sockets to have consistent
- behaviour in programs calling poll/select multiple times.
- Example test to non-listening port: curl -v 127.0.0.1:47 */
- if ((connect_state () == connect_failed) && (event_mask & FD_WRITE))
- wsock_events->events |= FD_WRITE;
UNLOCK_EVENTS;
return ret;
What do you think?
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
next prev parent reply other threads:[~2020-07-16 9:25 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-15 18:54 Marc Hoersken
2020-07-16 9:25 ` Corinna Vinschen [this message]
2020-07-17 18:56 ` Marc Hoersken
2020-07-17 19:21 ` Corinna Vinschen
2020-07-17 19:33 ` Marc Hoersken
2020-07-20 7:55 ` Corinna Vinschen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200716092553.GA3784@calimero.vinschen.de \
--to=corinna-cygwin@cygwin.com \
--cc=cygwin@cygwin.com \
--cc=info@marc-hoersken.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).