[PATCH] Fix poll/select signal socket as write ready on connect failure
Marc Hoersken
info@marc-hoersken.de
Fri Jul 17 18:56:29 GMT 2020
Hi Corinna,
Am 16.07.2020 um 11:25 schrieb Corinna Vinschen:
> Thanks for the patch. I pushed it.
thanks for pushing it already. Please excuse my delayed response, family
live kept me busy.
> But then I got second thoughts in terms of how to fix the issue.
Yes, I also got second thoughts yesterday about my initial approach.
> 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
Thanks, I also understood in the meantime, that some flags/events are
not removed from wsock_events->event. FD_CLOSE seems not to be affected
as you described and I was unable to produce an issue in case the
connection was closed from the server side. So only the FD_CONNECT to
FD_WRITE handling remained problematic (before the patch).
> 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?
I think this makes more sense, yes. I am just not sure if the socket
should also be write ready in case of a socket error. Looking at the
description of EINPROGRESS on the man page of connect [1], it seems like
writability is given regardless of the connection being successful or
not, but as soon as the connection attempt is no longer pending. For
successful connections FD_WRITE will be given already, so we will only
need to set it for failed connections regardless of a socket error in
wsock_events->connect_errorcode. Therefore I suggest to move the line
setting FD_WRITE [2] one level up outside of the else branch.
> 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?
I already tested your diff successfully, so this could be an alternative
approach to the issue. I just think the wsock_events->connect_errorcode
should also only be reset if FD_CONNECT is removed, right? So the if
branch would need to be extended to include the second line [3] as well.
Everything together, I think our suggestions together would look like this:
diff --git a/winsup/cygwin/fhandler_socket_inet.cc
b/winsup/cygwin/fhandler_socket_inet.cc
index e5b0d2d14..84cd63698 100644
--- a/winsup/cygwin/fhandler_socket_inet.cc
+++ b/winsup/cygwin/fhandler_socket_inet.cc
@@ -352,10 +352,15 @@ fhandler_socket_wsock::evaluate_events (const long
event_mask, long &events,
WSASetLastError (wsa_err);
ret = SOCKET_ERROR;
}
- else
- wsock_events->events |= FD_WRITE;
- wsock_events->events &= ~FD_CONNECT;
- wsock_events->connect_errorcode = 0;
+ wsock_events->events |= FD_WRITE;
+ /* Since FD_CONNECT is only given once, we have to keep FD_CONNECT
+ for connection failed sockets to have consistent behaviour
+ 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
is called on a socket for which shutdown has been called.
The second
@@ -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;
Best regards,
Marc
[1] https://www.man7.org/linux/man-pages/man2/connect.2.html#ERRORS
[2]
https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/fhandler_socket_inet.cc;h=e5b0d2d1443ecc4430104f6cfb78bf580a8116e5;hb=aa86784937ec7868c358dd90ea5e5324f0be750d#l356
[3]
https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/fhandler_socket_inet.cc;h=e5b0d2d1443ecc4430104f6cfb78bf580a8116e5;hb=aa86784937ec7868c358dd90ea5e5324f0be750d#l358
More information about the Cygwin
mailing list