]> git.ipfire.org Git - thirdparty/mtr.git/commitdiff
Update Cygwin ICMP service thread for asynchronous pipes 495/head
authorMatt Kimball <matt.kimball@gmail.com>
Tue, 24 Oct 2023 02:02:43 +0000 (03:02 +0100)
committerMatt Kimball <matt.kimball@gmail.com>
Tue, 24 Oct 2023 02:02:43 +0000 (03:02 +0100)
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.

packet/probe_cygwin.c
packet/probe_cygwin.h

index f41e5144b461e3b902b4bf7bb32ed62769deca59..d6d2b7f24ea0b8c9f28702daa5afea78dc1f0b68 100644 (file)
@@ -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  */
index eec001f56dd3815370b3de56d50d4847596d8883..1e9fde7b1a70caff57ba90c840affec0b50f1c49 100644 (file)
 #ifndef PROBE_CYGWIN_H
 #define PROBE_CYGWIN_H
 
+#include <cygwin/in6.h>
+typedef struct in6_addr IN6_ADDR, *PIN6_ADDR, *LPIN6_ADDR;
+
 #include <arpa/inet.h>
 #include <windows.h>
 #include <iphlpapi.h>
 #include <icmpapi.h>
 
-/*
-    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;
 };
 
 /*