[PATCH 0/1] MR8833: iphlpapi: Avoid leaking APC context in icmp_send_echo().

[Paul Gofman (＠gofman)]

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8833

[PATCH 1/1] iphlpapi: Avoid leaking APC context in icmp_send_echo().

[Paul Gofman]

From: Paul Gofman <pgofman(a)codeweavers.com>

---
 dlls/iphlpapi/iphlpapi_main.c | 38 ++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/dlls/iphlpapi/iphlpapi_main.c b/dlls/iphlpapi/iphlpapi_main.c
index 595f499c36e..135e1dc4cdd 100644
--- a/dlls/iphlpapi/iphlpapi_main.c
+++ b/dlls/iphlpapi/iphlpapi_main.c
@@ -4870,32 +4870,22 @@ static NTSTATUS icmp_send_echo( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc
                                 WORD request_size, IP_OPTION_INFORMATION *opts, void *reply, DWORD reply_size,
                                 DWORD timeout )
 {
+    static IO_STATUS_BLOCK iosb_placeholder;
     struct icmp_handle_data *data = (struct icmp_handle_data *)handle;
-    struct icmp_apc_ctxt *ctxt = heap_alloc( sizeof(*ctxt) );
-    IO_STATUS_BLOCK *iosb = &ctxt->iosb;
+    struct icmp_apc_ctxt *ctxt = NULL;
+    IO_STATUS_BLOCK *iosb;
     DWORD opt_size, in_size;
     struct nsiproxy_icmp_echo *in;
     HANDLE request_event;
     NTSTATUS status;
 
-    if (handle == INVALID_HANDLE_VALUE || !reply)
-    {
-        heap_free( ctxt );
-        return STATUS_INVALID_PARAMETER;
-    }
-
-    ctxt->apc_routine = apc_routine;
-    ctxt->apc_ctxt = apc_ctxt;
+    if (handle == INVALID_HANDLE_VALUE || !reply) return STATUS_INVALID_PARAMETER;
 
     opt_size = opts ? (opts->OptionsSize + 3) & ~3 : 0;
     in_size = FIELD_OFFSET(struct nsiproxy_icmp_echo, data[opt_size + request_size]);
     in = heap_alloc_zero( in_size );
 
-    if (!in)
-    {
-        heap_free( ctxt );
-        return STATUS_NO_MEMORY;
-    }
+    if (!in) return STATUS_NO_MEMORY;
 
     in->user_reply_ptr = (ULONG_PTR)reply;
     in->bits = sizeof(void*) * 8;
@@ -4917,8 +4907,24 @@ static NTSTATUS icmp_send_echo( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc
 
     request_event = event ? event : (apc_routine ? NULL : CreateEventW( NULL, 0, 0, NULL ));
 
+    if (event)
+    {
+        /* Async completion without calling APC routine, IOSB is not delivered anywhere. */
+        iosb = &iosb_placeholder;
+    }
+    else
+    {
+        if (!(ctxt = heap_alloc( sizeof(*ctxt) )))
+        {
+            heap_free( in );
+            return STATUS_NO_MEMORY;
+        }
+        iosb = &ctxt->iosb;
+        ctxt->apc_routine = apc_routine;
+        ctxt->apc_ctxt = apc_ctxt;
+    }
     status = NtDeviceIoControlFile( data->nsi_device, request_event, apc_routine && !event ? icmp_apc_routine : NULL,
-                                    apc_routine ? ctxt : apc_ctxt, iosb, IOCTL_NSIPROXY_WINE_ICMP_ECHO,
+                                    ctxt, iosb, IOCTL_NSIPROXY_WINE_ICMP_ECHO,
                                     in, in_size, reply, reply_size );
 
     if (status == STATUS_PENDING)
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8833

[Paul Gofman (＠gofman)]

The problem is pre-existing to https://gitlab.winehq.org/wine/wine/-/merge_requests/8782 . When there is just an event and no completion routine iosb pointer should still be passed to NtDeviceIoControlFile and the storage should persist until the completion, currently ctxt is leaked in such a case. Yet there is no way to get the IOSB data in this case by the app or us in iphlpapi, so in this case we can just use static IOSB and let writes to it freely race.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8833#note_113780