Signed-off-by: Paul Gofman pgofman@codeweavers.com --- The main goal of these patches is to address use after free which happens with TPIO objects when the completion arrives after TpReleaseIoCompletion() was called. Windows seems to keep the TPIO object alive after TpReleaseIoCompletion() until it receives the number of completions equal to what it expects. The following aspects are currently different in Wine: - TpWaitForIoCompletion with cancel_pending set to TRUE in Wine treats that essentially the same way as TpCancelAsyncIoOperatio(). As far as my testing goes, TpCancelAsyncIoOperatio() tells us that the operation is canceled and the completion is not expected. While with TpWaitForIoCompletion(..., TRUE) Windows still expects completions but doesn't call the callback. - The memory for TPIO object should not freed after TpReleaseIoCompletion() until the expected number of completions is received.
dlls/ntdll/threadpool.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c index 9e99398bdee..b82d06e5e42 100644 --- a/dlls/ntdll/threadpool.c +++ b/dlls/ntdll/threadpool.c @@ -1536,6 +1536,8 @@ static void CALLBACK ioqueue_thread_proc( void *param ) { RtlEnterCriticalSection( &io->pool->cs );
+ --io->u.io.pending_count; + if (!array_reserve((void **)&io->u.io.completions, &io->u.io.completion_max, io->u.io.completion_count + 1, sizeof(*io->u.io.completions))) { @@ -2138,7 +2140,6 @@ static void tp_object_execute( struct threadpool_object *object, BOOL wait_threa { assert( object->u.io.completion_count ); completion = object->u.io.completions[--object->u.io.completion_count]; - object->u.io.pending_count--; }
/* Leave critical section and do the actual callback. */
Signed-off-by: Paul Gofman pgofman@codeweavers.com --- dlls/ntdll/tests/threadpool.c | 18 ++++++++++++++++++ dlls/ntdll/threadpool.c | 29 +++++++++++++++++------------ 2 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/dlls/ntdll/tests/threadpool.c b/dlls/ntdll/tests/threadpool.c index 6b301eafcbd..986cbbcf8f1 100644 --- a/dlls/ntdll/tests/threadpool.c +++ b/dlls/ntdll/tests/threadpool.c @@ -2139,6 +2139,24 @@ static void test_tp_io(void) ok(!userdata.length, "got length %lu\n", userdata.length); ok(userdata.io == io, "expected %p, got %p\n", io, userdata.io);
+ userdata.count = 0; + pTpStartAsyncIoOperation(io); + pTpCancelAsyncIoOperation(io); + ret = ReadFile(server, in, sizeof(in), NULL, &ovl); + ok(!ret, "wrong ret %d\n", ret); + ret = WriteFile(client, out, sizeof(out), &ret_size, NULL); + ok(ret, "WriteFile() failed, error %u\n", GetLastError()); + ok(GetLastError() == ERROR_IO_PENDING, "wrong error %u\n", GetLastError()); + + pTpWaitForIoCompletion(io, FALSE); + if (0) + { + /* Add a sleep to check that callback is not called later. Commented out to + * save the test time. */ + Sleep(200); + } + ok(userdata.count == 0, "callback ran %u times\n", userdata.count); + CloseHandle(ovl.hEvent); CloseHandle(client); CloseHandle(server); diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c index b82d06e5e42..c27f6d4a145 100644 --- a/dlls/ntdll/threadpool.c +++ b/dlls/ntdll/threadpool.c @@ -1536,22 +1536,25 @@ static void CALLBACK ioqueue_thread_proc( void *param ) { RtlEnterCriticalSection( &io->pool->cs );
- --io->u.io.pending_count; + TRACE( "pending_count %u.\n", io->u.io.pending_count );
- if (!array_reserve((void **)&io->u.io.completions, &io->u.io.completion_max, - io->u.io.completion_count + 1, sizeof(*io->u.io.completions))) + if (io->u.io.pending_count) { - ERR("Failed to allocate memory.\n"); - RtlLeaveCriticalSection( &io->pool->cs ); - continue; - } - - completion = &io->u.io.completions[io->u.io.completion_count++]; - completion->iosb = iosb; - completion->cvalue = value; + --io->u.io.pending_count; + if (!array_reserve((void **)&io->u.io.completions, &io->u.io.completion_max, + io->u.io.completion_count + 1, sizeof(*io->u.io.completions))) + { + ERR( "Failed to allocate memory.\n" ); + RtlLeaveCriticalSection( &io->pool->cs ); + continue; + }
- tp_object_submit( io, FALSE ); + completion = &io->u.io.completions[io->u.io.completion_count++]; + completion->iosb = iosb; + completion->cvalue = value;
+ tp_object_submit( io, FALSE ); + } RtlLeaveCriticalSection( &io->pool->cs ); }
@@ -2525,6 +2528,8 @@ void WINAPI TpCancelAsyncIoOperation( TP_IO *io )
RtlEnterCriticalSection( &this->pool->cs );
+ TRACE("pending_count %u.\n", this->u.io.pending_count); + this->u.io.pending_count--; if (object_is_finished( this, TRUE )) RtlWakeAllConditionVariable( &this->group_finished_event );
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=94598
Your paranoid android.
=== w8 (32 bit report) ===
ntdll: threadpool.c:1478: Test failed: expected approximately 600 ticks, got 1172
Signed-off-by: Paul Gofman pgofman@codeweavers.com --- dlls/ntdll/threadpool.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c index c27f6d4a145..50433b7c009 100644 --- a/dlls/ntdll/threadpool.c +++ b/dlls/ntdll/threadpool.c @@ -1570,6 +1570,7 @@ static void CALLBACK ioqueue_thread_proc( void *param ) } }
+ ioqueue.thread_running = FALSE; RtlLeaveCriticalSection( &ioqueue.cs );
TRACE( "terminating I/O completion thread\n" );
Signed-off-by: Paul Gofman pgofman@codeweavers.com --- dlls/ntdll/tests/threadpool.c | 56 +++++++++++++++++++++++++++++-- dlls/ntdll/threadpool.c | 63 +++++++++++++++++++++++++++++------ 2 files changed, 107 insertions(+), 12 deletions(-)
diff --git a/dlls/ntdll/tests/threadpool.c b/dlls/ntdll/tests/threadpool.c index 986cbbcf8f1..6c28d0642d7 100644 --- a/dlls/ntdll/tests/threadpool.c +++ b/dlls/ntdll/tests/threadpool.c @@ -2157,10 +2157,62 @@ static void test_tp_io(void) } ok(userdata.count == 0, "callback ran %u times\n", userdata.count);
- CloseHandle(ovl.hEvent); - CloseHandle(client); + pTpReleaseIoCompletion(io); CloseHandle(server); + + /* Test TPIO object destruction. */ + server = CreateNamedPipeA("\\.\pipe\wine_tp_test", + PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED, 0, 1, 1024, 1024, 0, NULL); + ok(server != INVALID_HANDLE_VALUE, "Failed to create server pipe, error %u.\n", GetLastError()); + io = NULL; + status = pTpAllocIoCompletion(&io, server, io_cb, &userdata, &environment); + ok(!status, "got %#x\n", status); + + ret = HeapValidate(GetProcessHeap(), 0, io); + ok(ret, "Got unexpected ret %#x.\n", ret); pTpReleaseIoCompletion(io); + ret = HeapValidate(GetProcessHeap(), 0, io); + ok(!ret, "Got unexpected ret %#x.\n", ret); + CloseHandle(server); + CloseHandle(client); + + server = CreateNamedPipeA("\\.\pipe\wine_tp_test", + PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED, 0, 1, 1024, 1024, 0, NULL); + ok(server != INVALID_HANDLE_VALUE, "Failed to create server pipe, error %u.\n", GetLastError()); + client = CreateFileA("\\.\pipe\wine_tp_test", GENERIC_READ | GENERIC_WRITE, + 0, NULL, OPEN_EXISTING, 0, 0); + ok(client != INVALID_HANDLE_VALUE, "Failed to create client pipe, error %u.\n", GetLastError()); + + io = NULL; + status = pTpAllocIoCompletion(&io, server, io_cb, &userdata, &environment); + ok(!status, "got %#x\n", status); + pTpStartAsyncIoOperation(io); + pTpWaitForIoCompletion(io, TRUE); + ret = HeapValidate(GetProcessHeap(), 0, io); + ok(ret, "Got unexpected ret %#x.\n", ret); + pTpReleaseIoCompletion(io); + ret = HeapValidate(GetProcessHeap(), 0, io); + ok(ret, "Got unexpected ret %#x.\n", ret); + + if (0) + { + /* Object destruction will wait until one completion arrives (which was started but not cancelled). + * Commented out to save test time. */ + Sleep(1000); + ret = HeapValidate(GetProcessHeap(), 0, io); + ok(ret, "Got unexpected ret %#x.\n", ret); + ret = ReadFile(server, in, sizeof(in), NULL, &ovl); + ok(!ret, "wrong ret %d\n", ret); + ret = WriteFile(client, out, sizeof(out), &ret_size, NULL); + ok(ret, "WriteFile() failed, error %u\n", GetLastError()); + Sleep(2000); + ret = HeapValidate(GetProcessHeap(), 0, io); + ok(!ret, "Got unexpected ret %#x.\n", ret); + } + + CloseHandle(server); + CloseHandle(ovl.hEvent); + CloseHandle(client); pTpReleasePool(pool); }
diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c index 50433b7c009..ca323919d05 100644 --- a/dlls/ntdll/threadpool.c +++ b/dlls/ntdll/threadpool.c @@ -201,7 +201,8 @@ struct threadpool_object { PTP_IO_CALLBACK callback; /* locked via .pool->cs */ - unsigned int pending_count, completion_count, completion_max; + unsigned int pending_count, skipped_count, completion_count, completion_max; + BOOL shutting_down; struct io_completion *completions; } io; } u; @@ -1506,6 +1507,7 @@ static void CALLBACK ioqueue_thread_proc( void *param ) struct threadpool_object *io; IO_STATUS_BLOCK iosb; ULONG_PTR key, value; + BOOL destroy, skip; NTSTATUS status;
TRACE( "starting I/O completion thread\n" ); @@ -1519,17 +1521,33 @@ static void CALLBACK ioqueue_thread_proc( void *param ) ERR("NtRemoveIoCompletion failed, status %#x.\n", status); RtlEnterCriticalSection( &ioqueue.cs );
+ destroy = skip = FALSE; io = (struct threadpool_object *)key;
- if (io && io->shutdown) + TRACE( "io %p, iosb.Status %#x.\n", io, iosb.u.Status ); + + if (io && (io->shutdown || io->u.io.shutting_down)) { - if (iosb.u.Status != STATUS_THREADPOOL_RELEASED_DURING_OPERATION) + RtlEnterCriticalSection( &io->pool->cs ); + if (!io->u.io.pending_count) { - /* Skip remaining completions until the final one. */ - continue; + if (io->u.io.skipped_count) + --io->u.io.skipped_count; + + if (io->u.io.skipped_count) + skip = TRUE; + else + destroy = TRUE; } + RtlLeaveCriticalSection( &io->pool->cs ); + if (skip) continue; + } + + if (destroy) + { --ioqueue.objcount; TRACE( "Releasing io %p.\n", io ); + io->shutdown = TRUE; tp_object_release( io ); } else if (io) @@ -2004,7 +2022,10 @@ static void tp_object_cancel( struct threadpool_object *object ) object->u.wait.signaled = 0; } if (object->type == TP_OBJECT_TYPE_IO) + { + object->u.io.skipped_count += object->u.io.pending_count; object->u.io.pending_count = 0; + } RtlLeaveCriticalSection( &pool->cs );
while (pending_callbacks--) @@ -2045,6 +2066,20 @@ static void tp_object_wait( struct threadpool_object *object, BOOL group_wait ) RtlLeaveCriticalSection( &pool->cs ); }
+static void tp_ioqueue_unlock( struct threadpool_object *io ) +{ + assert( io->type == TP_OBJECT_TYPE_IO ); + + RtlEnterCriticalSection( &ioqueue.cs ); + + assert(ioqueue.objcount); + + if (!io->shutdown && !--ioqueue.objcount) + NtSetIoCompletion( ioqueue.port, 0, 0, STATUS_SUCCESS, 0 ); + + RtlLeaveCriticalSection( &ioqueue.cs ); +} + /*********************************************************************** * tp_object_prepare_shutdown (internal) * @@ -2056,6 +2091,8 @@ static void tp_object_prepare_shutdown( struct threadpool_object *object ) tp_timerqueue_unlock( object ); else if (object->type == TP_OBJECT_TYPE_WAIT) tp_waitqueue_unlock( object ); + else if (object->type == TP_OBJECT_TYPE_IO) + tp_ioqueue_unlock( object ); }
/*********************************************************************** @@ -2797,15 +2834,21 @@ VOID WINAPI TpReleaseCleanupGroupMembers( TP_CLEANUP_GROUP *group, BOOL cancel_p void WINAPI TpReleaseIoCompletion( TP_IO *io ) { struct threadpool_object *this = impl_from_TP_IO( io ); + BOOL can_destroy;
TRACE( "%p\n", io );
- RtlEnterCriticalSection( &ioqueue.cs ); + RtlEnterCriticalSection( &this->pool->cs ); + this->u.io.shutting_down = TRUE; + can_destroy = !this->u.io.pending_count && !this->u.io.skipped_count; + RtlLeaveCriticalSection( &this->pool->cs );
- assert( ioqueue.objcount ); - this->shutdown = TRUE; - NtSetIoCompletion( ioqueue.port, (ULONG_PTR)this, 0, STATUS_THREADPOOL_RELEASED_DURING_OPERATION, 1 ); - RtlLeaveCriticalSection( &ioqueue.cs ); + if (can_destroy) + { + tp_object_prepare_shutdown( this ); + this->shutdown = TRUE; + tp_object_release( this ); + } }
/***********************************************************************
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=94600
Your paranoid android.
=== w10pro64_ar (64 bit report) ===
ntdll: threadpool.c:1170: Test failed: WaitForSingleObject returned 0 threadpool.c:1210: Test failed: WaitForSingleObject returned 258
-
Marvin -
Paul Gofman