From: Matt Kimball Date: Tue, 24 Oct 2023 02:02:43 +0000 (+0100) Subject: Update Cygwin ICMP service thread for asynchronous pipes X-Git-Tag: v0.96~16^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F495%2Fhead;p=thirdparty%2Fmtr.git Update Cygwin ICMP service thread for asynchronous pipes Recent versions of Cygwin implement pipe() using Windows' named pipes, and put the read end of the pipe in FILE_PIPE_COMPLETE_OPERATION mode, which doesn't allow overlapped I/O operations. For the relevant commit in the Cygwin repository, see 9e4d308cd592fe383dec58ea6523c1b436888ef8 The solution here is to maintain a Windows event object which is set only when any ICMP requests are pending. We can do an alertable wait on that event object, which will allow us to complete ICMP requests. Thanks to Adam Schultz for research into this issue and a first attempt at a fix. --- diff --git a/packet/probe_cygwin.c b/packet/probe_cygwin.c index f41e514..d6d2b7f 100644 --- a/packet/probe_cygwin.c +++ b/packet/probe_cygwin.c @@ -136,8 +136,14 @@ void init_net_state( net_state->platform.thread_out_pipe_read = out_pipe[0]; net_state->platform.thread_out_pipe_write = out_pipe[1]; - net_state->platform.thread_in_pipe_read_handle = - (HANDLE)get_osfhandle(in_pipe[0]); + InitializeCriticalSection(&net_state->platform.pending_request_cs); + net_state->platform.pending_request_count = 0; + net_state->platform.pending_request_event = + CreateEvent(NULL, TRUE, FALSE, NULL); + + if (net_state->platform.pending_request_event == NULL) { + error(EXIT_FAILURE, errno, "Failure creating request event"); + } /* The read on the out pipe needs to be nonblocking because @@ -281,7 +287,7 @@ void WINAPI on_icmp_reply( remote_addr6->sin6_family = AF_INET6; remote_addr6->sin6_port = 0; remote_addr6->sin6_flowinfo = 0; - memcpy(&remote_addr6->sin6_addr, reply6->AddressBits, + memcpy(&remote_addr6->sin6_addr, reply6->Address.sin6_addr, sizeof(struct in6_addr)); remote_addr6->sin6_scope_id = 0; } @@ -468,109 +474,116 @@ void icmp_handle_probe_request(struct icmp_thread_request_t *request) } /* - The main loop of the ICMP service thread. The loop starts - an overlapped read on the incoming request pipe, then waits - in an alertable wait for that read to complete. Because - the wait is alertable, ICMP probes can complete through - APCs in that wait. + Write the next thread request to the request pipe. + Update the count of pending requests and set the event + indicating that requests are present. */ static -DWORD WINAPI icmp_service_thread(LPVOID param) { - struct net_state_t *net_state; - struct icmp_thread_request_t *request; - DWORD wait_status; - OVERLAPPED overlapped; - HANDLE event; - BOOL success; - bool read_pending; - DWORD read_count; - int err; +void send_thread_request( + struct net_state_t *net_state, + struct icmp_thread_request_t *request) +{ + int byte_count; + byte_count = write( + net_state->platform.thread_in_pipe_write, + &request, + sizeof(struct icmp_thread_request_t *)); - /* - We need an event to signal completion of reads from the request - pipe. - */ - event = CreateEvent(NULL, TRUE, FALSE, NULL); - if (event == NULL) { - error_win( - EXIT_FAILURE, GetLastError(), - "failure creating ICMP thread event"); + if (byte_count == -1) { + error( + EXIT_FAILURE, errno, + "failure writing to probe request queue"); } - net_state = (struct net_state_t *)param; - read_pending = false; - while (true) { - /* - Start a new read on the request pipe if none is - currently pending. - */ - if (!read_pending) { - request = NULL; - - ResetEvent(event); - - memset(&overlapped, 0, sizeof(OVERLAPPED)); - overlapped.hEvent = event; - - success = ReadFile( - net_state->platform.thread_in_pipe_read_handle, - &request, - sizeof(struct icmp_thread_request_t *), - NULL, - &overlapped); + EnterCriticalSection(&net_state->platform.pending_request_cs); + { + net_state->platform.pending_request_count++; + SetEvent(net_state->platform.pending_request_event); + } + LeaveCriticalSection(&net_state->platform.pending_request_cs); +} - if (!success) { - err = GetLastError(); +/* + Read the next thread request from the pipe, if any are pending. + If it is the last request in the queue, reset the pending + request event. - if (err != ERROR_IO_PENDING) { - error_win( - EXIT_FAILURE, err, - "failure starting overlapped thread pipe read"); - } + If no requests are pending, return NULL. +*/ +static +struct icmp_thread_request_t *receive_thread_request( + struct net_state_t *net_state) +{ + struct icmp_thread_request_t *request; + int byte_count; + bool pending_request; + + EnterCriticalSection(&net_state->platform.pending_request_cs); + { + if (net_state->platform.pending_request_count > 0) { + pending_request = true; + net_state->platform.pending_request_count--; + if (net_state->platform.pending_request_count == 0) { + ResetEvent(net_state->platform.pending_request_event); } - - read_pending = true; + } else { + pending_request = false; } + } + LeaveCriticalSection(&net_state->platform.pending_request_cs); - /* - Wait for either the request read to complete, or - an APC which completes an ICMP probe. - */ - wait_status = WaitForSingleObjectEx( - event, - INFINITE, - TRUE); + if (!pending_request) { + return NULL; + } - /* - If the event we waited on has been signalled, read - the request from the pipe. - */ - if (wait_status == WAIT_OBJECT_0) { - read_pending = false; - - success = GetOverlappedResult( - net_state->platform.thread_in_pipe_read_handle, - &overlapped, - &read_count, - FALSE); - - if (!success) { - error_win( - EXIT_FAILURE, GetLastError(), - "failure completing overlapped thread pipe read"); - } + byte_count = read( + net_state->platform.thread_in_pipe_read, + &request, + sizeof(struct icmp_thread_request_t *)); - if (read_count == 0) { - continue; - } + if (byte_count == -1) { + error( + EXIT_FAILURE, + errno, + "failure reading probe request queue"); + } + + assert(byte_count == sizeof(struct icmp_thread_request_t *)); + + return request; +} - assert( - read_count == sizeof(struct icmp_thread_request_t *)); +/* + The main loop of the ICMP service thread. The loop starts + an overlapped read on the incoming request pipe, then waits + in an alertable wait for that read to complete. Because + the wait is alertable, ICMP probes can complete through + APCs in that wait. +*/ +static +DWORD WINAPI icmp_service_thread(LPVOID param) { + struct net_state_t *net_state; + struct icmp_thread_request_t *request; + net_state = (struct net_state_t *)param; + while (true) { + request = receive_thread_request(net_state); + if (request != NULL) { /* Start the new probe from the request */ icmp_handle_probe_request(request); + } else { + /* + Wait for either a request to be queued or for + an APC which completes an ICMP probe. + */ + WaitForSingleObjectEx( + net_state->platform.pending_request_event, + INFINITE, + TRUE); } } + + return 0; } /* @@ -587,7 +600,6 @@ void queue_thread_request( struct sockaddr_storage *src_sockaddr) { struct icmp_thread_request_t *request; - int byte_count; request = malloc(sizeof(struct icmp_thread_request_t)); if (request == NULL) { @@ -610,16 +622,7 @@ void queue_thread_request( The ownership of the request is passed to the ICMP thread through the pipe. */ - byte_count = write( - net_state->platform.thread_in_pipe_write, - &request, - sizeof(struct icmp_thread_request_t *)); - - if (byte_count == -1) { - error( - EXIT_FAILURE, errno, - "failure writing to probe request queue"); - } + send_thread_request(net_state, request); } /* Decode the probe parameters and send a probe */ diff --git a/packet/probe_cygwin.h b/packet/probe_cygwin.h index eec001f..1e9fde7 100644 --- a/packet/probe_cygwin.h +++ b/packet/probe_cygwin.h @@ -19,32 +19,14 @@ #ifndef PROBE_CYGWIN_H #define PROBE_CYGWIN_H +#include +typedef struct in6_addr IN6_ADDR, *PIN6_ADDR, *LPIN6_ADDR; + #include #include #include #include -/* - This should be in the Windows headers, but is missing from - Cygwin's Windows headers. -*/ -typedef struct icmpv6_echo_reply_lh { - /* - Although Windows uses an IPV6_ADDRESS_EX here, we are using uint8_t - fields to avoid structure padding differences between gcc and - Visual C++. (gcc wants to align the flow info to a 4 byte boundary, - and Windows uses it unaligned.) - */ - uint8_t PortBits[2]; - uint8_t FlowInfoBits[4]; - uint8_t AddressBits[16]; - uint8_t ScopeIdBits[4]; - - ULONG Status; - unsigned int RoundTripTime; -} ICMPV6_ECHO_REPLY, -*PICMPV6_ECHO_REPLY; - /* Windows requires an echo reply structure for each in-flight ICMP probe. @@ -67,9 +49,16 @@ struct net_state_platform_t { bool ip4_socket_raw; bool ip6_socket_raw; - HANDLE thread_in_pipe_read_handle; int thread_in_pipe_read, thread_in_pipe_write; int thread_out_pipe_read, thread_out_pipe_write; + + CRITICAL_SECTION pending_request_cs; + + /* Guarded by the critical section. */ + unsigned int pending_request_count; + + /* Set when any requests are pending. */ + HANDLE pending_request_event; }; /*