[PATCH 0/2] MR8873: Iphlpapi, nsiproxy.sys: Fix async icmp echo request completion.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsiproxy.sys/device.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/dlls/nsiproxy.sys/device.c b/dlls/nsiproxy.sys/device.c index b1c674ca5f3..3e806ca9835 100644 --- a/dlls/nsiproxy.sys/device.c +++ b/dlls/nsiproxy.sys/device.c @@ -196,6 +196,8 @@ static inline icmp_handle irp_set_icmp_handle( IRP *irp, icmp_handle handle ) ULongToPtr( handle ) ) ); } +DECLARE_CRITICAL_SECTION( icmp_echo_completion_cs ); + static void WINAPI icmp_echo_cancel( DEVICE_OBJECT *device, IRP *irp ) { struct icmp_cancel_listen_params params; @@ -203,8 +205,7 @@ static void WINAPI icmp_echo_cancel( DEVICE_OBJECT *device, IRP *irp ) TRACE( "device %p, irp %p.\n", device, irp ); IoReleaseCancelSpinLock( irp->CancelIrql ); - - EnterCriticalSection( &nsiproxy_cs ); + EnterCriticalSection( &icmp_echo_completion_cs ); /* If the handle is not set, either the irp is still in the request queue, in which case the request thread will @@ -213,8 +214,7 @@ static void WINAPI icmp_echo_cancel( DEVICE_OBJECT *device, IRP *irp ) will be completed elsewhere. */ params.handle = irp_get_icmp_handle( irp ); if (params.handle) nsiproxy_call( icmp_cancel_listen, ¶ms ); - - LeaveCriticalSection( &nsiproxy_cs ); + LeaveCriticalSection( &icmp_echo_completion_cs ); } static int icmp_echo_reply_struct_len( ULONG family, ULONG bits ) @@ -247,8 +247,7 @@ static DWORD WINAPI listen_thread_proc( void *arg ) status = nsiproxy_call( icmp_listen, ¶ms ); TRACE( "icmp_listen rets %08lx\n", status ); - EnterCriticalSection( &nsiproxy_cs ); - + EnterCriticalSection( &icmp_echo_completion_cs ); close_params.handle = irp_set_icmp_handle( irp, 0 ); nsiproxy_call( icmp_close, &close_params ); @@ -257,10 +256,8 @@ static DWORD WINAPI listen_thread_proc( void *arg ) irp->IoStatus.Information = params.reply_len; else irp->IoStatus.Information = 0; + LeaveCriticalSection( &icmp_echo_completion_cs ); IoCompleteRequest( irp, IO_NO_INCREMENT ); - - LeaveCriticalSection( &nsiproxy_cs ); - return 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8873
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/iphlpapi/iphlpapi_main.c | 107 ++++++++++++++++++++++----------- dlls/iphlpapi/tests/iphlpapi.c | 67 ++++++++++++++++++++- 2 files changed, 138 insertions(+), 36 deletions(-) diff --git a/dlls/iphlpapi/iphlpapi_main.c b/dlls/iphlpapi/iphlpapi_main.c index 6003ba6e485..5f833a0f489 100644 --- a/dlls/iphlpapi/iphlpapi_main.c +++ b/dlls/iphlpapi/iphlpapi_main.c @@ -4774,6 +4774,43 @@ struct icmp_handle_data HANDLE nsi_device; }; +struct icmp_apc_ctxt +{ + HANDLE event; + HANDLE thread; + void *apc_ctxt; + PIO_APC_ROUTINE apc_routine; + IO_STATUS_BLOCK iosb; +}; + +static void CALLBACK icmp_apc_routine( ULONG_PTR context ) +{ + struct icmp_apc_ctxt *ctx = (struct icmp_apc_ctxt *)context; + + ctx->apc_routine( ctx->apc_ctxt, &ctx->iosb, 0 ); + heap_free( ctx ); +} + +static void CALLBACK icmp_iocp_callback( DWORD error, DWORD count, OVERLAPPED *ovr ) +{ + struct icmp_apc_ctxt *ctx = (struct icmp_apc_ctxt *)ovr; + HANDLE thread; + BOOL ret; + + if (!ctx) return; + if (ctx->event) SetEvent( ctx->event ); + else if (ctx->apc_routine) + { + /* Don't access ctx after successful APC queue, it will be freed there. */ + thread = ctx->thread; + ctx->thread = NULL; + ret = QueueUserAPC( icmp_apc_routine, thread, (ULONG_PTR)ctx ); + CloseHandle( thread ); + if (ret) return; + } + heap_free( ctx ); +} + /*********************************************************************** * IcmpCloseHandle (IPHLPAPI.@) */ @@ -4811,7 +4848,13 @@ HANDLE WINAPI IcmpCreateFile( void ) heap_free( data ); return INVALID_HANDLE_VALUE; } - + if (!BindIoCompletionCallback( data->nsi_device, icmp_iocp_callback, 0 )) + { + ERR( "BindIoCompletionCallback failed.\n" ); + CloseHandle( data->nsi_device ); + heap_free( data ); + return INVALID_HANDLE_VALUE; + } return (HANDLE)data; } @@ -4850,33 +4893,17 @@ DWORD WINAPI IcmpSendEcho2( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc_rou opts, reply, reply_size, timeout ); } -struct icmp_apc_ctxt -{ - void *apc_ctxt; - PIO_APC_ROUTINE apc_routine; - IO_STATUS_BLOCK iosb; -}; - -void WINAPI icmp_apc_routine( void *context, IO_STATUS_BLOCK *iosb, ULONG reserved ) -{ - struct icmp_apc_ctxt *ctxt = context; - - ctxt->apc_routine( ctxt->apc_ctxt, iosb, reserved ); - heap_free( ctxt ); -} - static NTSTATUS icmp_send_echo( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc_routine, void *apc_ctxt, SOCKADDR_INET *src_addr, SOCKADDR_INET *dst_addr, void *request, 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 = NULL; - IO_STATUS_BLOCK *iosb; + IO_STATUS_BLOCK *iosb, iosb_local; DWORD opt_size, in_size; struct nsiproxy_icmp_echo *in; - HANDLE request_event; + HANDLE request_event = NULL; NTSTATUS status; if (handle == INVALID_HANDLE_VALUE || !reply) return STATUS_INVALID_PARAMETER; @@ -4905,12 +4932,7 @@ static NTSTATUS icmp_send_echo( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc in->timeout = timeout; memcpy( in->data + opt_size, request, request_size ); - if (event) - { - /* Async completion without calling APC routine, IOSB is not delivered anywhere. */ - iosb = &iosb_placeholder; - } - else + if (event || apc_routine) { if (!(ctxt = heap_alloc( sizeof(*ctxt) ))) { @@ -4920,22 +4942,37 @@ static NTSTATUS icmp_send_echo( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc iosb = &ctxt->iosb; ctxt->apc_routine = apc_routine; ctxt->apc_ctxt = apc_ctxt; + ctxt->event = event; + ctxt->thread = NULL; + if (!event) + { + if (!DuplicateHandle( GetCurrentProcess(), GetCurrentThread(), GetCurrentProcess(), + &ctxt->thread, 0, FALSE, DUPLICATE_SAME_ACCESS )) + { + heap_free( ctxt ); + heap_free( in ); + return STATUS_NO_MEMORY; + } + } + } + else + { + iosb = &iosb_local; + request_event = CreateEventW( NULL, 0, 0, NULL ); } - request_event = event ? event : (apc_routine ? NULL : CreateEventW( NULL, 0, 0, NULL )); - - status = NtDeviceIoControlFile( data->nsi_device, request_event, apc_routine && !event ? icmp_apc_routine : NULL, - ctxt, iosb, IOCTL_NSIPROXY_WINE_ICMP_ECHO, + status = NtDeviceIoControlFile( data->nsi_device, request_event, NULL, ctxt, + iosb, IOCTL_NSIPROXY_WINE_ICMP_ECHO, in, in_size, reply, reply_size ); + if (!ctxt && status == STATUS_PENDING && !WaitForSingleObject( request_event, INFINITE )) + status = iosb->Status; - if (status == STATUS_PENDING) + if (request_event) CloseHandle( request_event ); + if (ctxt && status != STATUS_PENDING) { - if (!event && !apc_routine && !WaitForSingleObject( request_event, INFINITE )) - status = iosb->Status; + if (ctxt->thread) CloseHandle( ctxt->thread ); + heap_free( ctxt ); } - - if (!event && request_event) CloseHandle( request_event ); - if ((!apc_routine && !event) || status != STATUS_PENDING) heap_free( ctxt ); heap_free( in ); return status; } diff --git a/dlls/iphlpapi/tests/iphlpapi.c b/dlls/iphlpapi/tests/iphlpapi.c index 7aa73744813..e8386f52822 100644 --- a/dlls/iphlpapi/tests/iphlpapi.c +++ b/dlls/iphlpapi/tests/iphlpapi.c @@ -951,11 +951,37 @@ static IO_STATUS_BLOCK icmp_send_echo_io; static void WINAPI icmp_send_echo_test_apc(void *context, IO_STATUS_BLOCK *io_status, ULONG reserved) { ok_(__FILE__, icmp_send_echo_test_line)(icmp_send_echo_test_apc_expect, "Unexpected APC execution\n"); + ok_(__FILE__, icmp_send_echo_test_line)(!reserved, "Got reserved %#lx\n", reserved); ok_(__FILE__, icmp_send_echo_test_line)(context == (void*)0xdeadc0de, "Wrong context: %p\n", context); icmp_send_echo_test_apc_expect = FALSE; icmp_send_echo_io = *io_status; } +struct test_echo_thread_params +{ + HANDLE icmp; + HANDLE event; + PIO_APC_ROUTINE apc; + void *apc_context; + IPAddr address; + char *senddata; + DWORD send_data_size; + char *replydata; + DWORD replysz; + DWORD timeout; +}; + +static DWORD CALLBACK test_echo_thread(void *params) +{ + struct test_echo_thread_params *p = params; + BOOL ret; + + ret = IcmpSendEcho2(p->icmp, p->event, p->apc, p->apc_context, p->address, p->senddata, p->send_data_size, + NULL, p->replydata, p->replysz, p->timeout); + ok(!ret && GetLastError() == ERROR_IO_PENDING, "got ret %d, error %ld.\n", ret, GetLastError()); + return 0; +} + static void testIcmpSendEcho(void) { /* The APC's signature is different pre-Vista */ @@ -963,10 +989,11 @@ static void testIcmpSendEcho(void) char senddata[32], replydata[sizeof(senddata) + sizeof(ICMP_ECHO_REPLY)]; char replydata2[sizeof(replydata) + sizeof(IO_STATUS_BLOCK)]; DWORD ret, error, replysz = sizeof(replydata); + struct test_echo_thread_params p; IP_OPTION_INFORMATION opt; IPAddr address; ICMP_ECHO_REPLY *reply; - HANDLE event; + HANDLE event, thread; INT i; memset(senddata, 0, sizeof(senddata)); @@ -1283,6 +1310,37 @@ static void testIcmpSendEcho(void) ok(!memcmp(senddata, reply->Data, min(sizeof(senddata), reply->DataSize)), "Data mismatch\n"); /* asynchronous tests with event */ + + replysz = sizeof(replydata2); + ResetEvent(event); + p.icmp = icmp; + p.event = event; + p.apc = icmp_send_echo_test_apc; + p.apc_context = (void *)0xdeadbeef; + ret = inet_pton(AF_INET, "192.18.1.1", &address); + ok(ret, "got error %u.\n", WSAGetLastError()); + p.address = address; + p.senddata = senddata; + p.send_data_size = sizeof(senddata); + p.replydata = replydata2; + p.replysz = replysz; + p.timeout = 30; + memset(replydata2, 0xcc, sizeof(replydata2)); + reply = (ICMP_ECHO_REPLY*)replydata2; + thread = CreateThread(NULL, 0, test_echo_thread, &p, 0, NULL); + + ret = WaitForSingleObject(thread, INFINITE); + ok(ret == WAIT_OBJECT_0, "got %lu.\n", ret); + IcmpCloseHandle(icmp); /* completion is delivered even if icmp handle is closed */ + icmp = IcmpCreateFile(); + ok (icmp != INVALID_HANDLE_VALUE, "IcmpCreateFile failed unexpectedly with error %ld\n", GetLastError()); + + CloseHandle(thread); + ret = WaitForSingleObject(event, INFINITE); + ok(ret == WAIT_OBJECT_0, "got %lu.\n", ret); + ok(reply->Status == IP_REQ_TIMED_OUT, "Expect status: 0x%08x, got: 0x%08lx\n", IP_REQ_TIMED_OUT, reply->Status); + ok(!reply->DataSize, "got size %d.\n", reply->DataSize); + SetLastError(0xdeadbeef); replysz = sizeof(replydata2); address = htonl(INADDR_LOOPBACK); @@ -1297,6 +1355,9 @@ static void testIcmpSendEcho(void) { ok(!ret, "IcmpSendEcho2 returned success unexpectedly\n"); ok(error == ERROR_IO_PENDING, "Expect last error: 0x%08x, got: 0x%08lx\n", ERROR_IO_PENDING, error); + IcmpCloseHandle(icmp); /* completion is delivered even if icmp handle is closed */ + icmp = IcmpCreateFile(); + ok (icmp != INVALID_HANDLE_VALUE, "IcmpCreateFile failed unexpectedly with error %ld\n", GetLastError()); ret = WaitForSingleObjectEx(event, 2000, TRUE); ok(ret == WAIT_OBJECT_0, "WaitForSingleObjectEx failed unexpectedly with %lu\n", ret); reply = (ICMP_ECHO_REPLY*)replydata2; @@ -1365,6 +1426,10 @@ static void testIcmpSendEcho(void) error = GetLastError(); ok(!ret, "IcmpSendEcho2 returned success unexpectedly\n"); ok(error == ERROR_IO_PENDING, "Expect last error: 0x%08x, got: 0x%08lx\n", ERROR_IO_PENDING, error); + IcmpCloseHandle(icmp); + icmp = IcmpCreateFile(); + ok (icmp != INVALID_HANDLE_VALUE, "IcmpCreateFile failed unexpectedly with error %ld\n", GetLastError()); + SleepEx(200, TRUE); SleepEx(0, TRUE); ok(icmp_send_echo_test_apc_expect == FALSE, "APC was not executed!\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8873
This (hopefully) fixes Farlight 84 randomly hanging during match start. I think both issues concerned here are pre-existing to both https://gitlab.winehq.org/wine/wine/-/merge_requests/8833 and https://gitlab.winehq.org/wine/wine/-/merge_requests/8782. So far the game only calls ipv4 IcmpSendEcho2Ex() with an event and NULL apc routine and context, with a 2sec timeout. Sometimes the thread which called IcmpSendEcho2Ex() will exit before the request is complete. That currently triggers canceling IRP (exiting thread executing async IO will always do so if there is an event provided in async request). There are two problems with it. 1. Canceling echo request IRP may currently hang nsirpoxy.sys (addressed by the first patch). This happens because icmp_echo_cancel() IRP cancel routine takes global nsiproxy_cs. IRP cancel routine is called by ntoskrnl with irp_completion_cs held, so irp_completion_cs. On some other occasions in nsiproxy.sys (in a generic case not limited to ICMP echo operation) whenever irp_completion_cs and nsiproxy_cs end up being taken together nsiproxy_cs is taken first, so this occasionally hangs. I guess most straightforward fix for that is to use a finer grained critical section in that place. icmp_echo_cancel only needs to be synchronized with listen_thread_proc() exiting from listen function. While patch 2 avoids IRP being canceled at all in this specific case I suppose nsiproxy.sys should still support proper IRP cancel which can happen under different conditions. 2. Thread exit of the thread which called IcmpSendEcho() should not cancel echo request (as the test included in patch 2 shows). The necessary conditions for NtDeviceIoControlFile() not cancelling async on IO queueing thread exit are: - a completion port should be associated; - there should be no event provided to NtDeviceIoControlFile call. Using thread pool's BindIoCompletionCallback() (and then never passing app's event to NtDeviceIoControlFile so the async is not cancelled on thread exit) looks like the simplest solution to me. The alternative would probably be to implement a dedicated thread in iphlpapi to deliver the ioctl requests. That is a bit complicated by the fact that synchronous errors should be returned synchronously from icmp_send_echo() and so far looks less than ideal to me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8873#note_114474
Also WRT p. 2., routing completion APC case also through completion callback is there because it is not possible to queue NtDeviceIoControlFile() with completion APC when there is completion port, so doing it that way would require different nsi device files opened with and without BindIoCompletionCallback(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8873#note_114477
This merge request was approved by Huw Davies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8873
participants (3)
-
Huw Davies (＠huw) -
Paul Gofman
-
Paul Gofman (＠gofman)