[PATCH v5 0/1] MR8572: Draft: kernelbase: Always wait in GetOverlappedResultEx() if timeout is nonzero.
-- v5: kernelbase: Wait in GetOverlappedResultEx() even if IOSB status is not pending.
From: Paul Gofman pgofman@codeweavers.com
--- dlls/kernel32/tests/file.c | 240 ++++++++++++++++++++++++++++++++++++- dlls/kernelbase/file.c | 50 ++++---- 2 files changed, 268 insertions(+), 22 deletions(-)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index b3ef912f700..7f448810463 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -38,6 +38,7 @@ #include "ddk/ntifs.h"
static HANDLE (WINAPI *pFindFirstFileExA)(LPCSTR,FINDEX_INFO_LEVELS,LPVOID,FINDEX_SEARCH_OPS,LPVOID,DWORD); +static BOOL (WINAPI *pGetOverlappedResultEx)(HANDLE, OVERLAPPED *, DWORD *, DWORD, BOOL); static BOOL (WINAPI *pReplaceFileW)(LPCWSTR, LPCWSTR, LPCWSTR, DWORD, LPVOID, LPVOID); static UINT (WINAPI *pGetSystemWindowsDirectoryA)(LPSTR, UINT); static BOOL (WINAPI *pGetVolumeNameForVolumeMountPointA)(LPCSTR, LPSTR, DWORD); @@ -93,6 +94,7 @@ static void InitFunctionPointers(void) pRtlFreeUnicodeString = (void *)GetProcAddress(hntdll, "RtlFreeUnicodeString");
pFindFirstFileExA=(void*)GetProcAddress(hkernel32, "FindFirstFileExA"); + pGetOverlappedResultEx =(void*)GetProcAddress(hkernel32, "GetOverlappedResultEx"); pReplaceFileW=(void*)GetProcAddress(hkernel32, "ReplaceFileW"); pGetSystemWindowsDirectoryA=(void*)GetProcAddress(hkernel32, "GetSystemWindowsDirectoryA"); pGetVolumeNameForVolumeMountPointA = (void *) GetProcAddress(hkernel32, "GetVolumeNameForVolumeMountPointA"); @@ -3737,8 +3739,93 @@ static void test_OpenFile(void)
static void test_overlapped(void) { + static const struct + { + BOOL ex, alertable, wait, queue_apc; + BOOL pass_file_handle, pass_event_handle, set_event; + } + tests[] = + { + { FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE }, + { TRUE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE }, + { TRUE, TRUE, FALSE, FALSE, FALSE, FALSE, FALSE }, + { FALSE, FALSE, TRUE, FALSE, FALSE, FALSE, FALSE }, + { TRUE, FALSE, TRUE, FALSE, FALSE, FALSE, FALSE }, + { TRUE, TRUE, TRUE, FALSE, FALSE, FALSE, FALSE }, + { TRUE, TRUE, FALSE, TRUE, FALSE, FALSE, FALSE }, + { TRUE, TRUE, TRUE, TRUE, FALSE, FALSE, FALSE }, + { FALSE, FALSE, FALSE, FALSE, TRUE, FALSE, FALSE }, + { TRUE, FALSE, FALSE, FALSE, TRUE, FALSE, FALSE }, + { TRUE, TRUE, FALSE, FALSE, TRUE, FALSE, FALSE }, + { FALSE, FALSE, TRUE, FALSE, TRUE, FALSE, FALSE }, + { TRUE, FALSE, TRUE, FALSE, TRUE, FALSE, FALSE }, + { TRUE, TRUE, TRUE, FALSE, TRUE, FALSE, FALSE }, + { TRUE, TRUE, FALSE, TRUE, TRUE, FALSE, FALSE }, + { TRUE, TRUE, TRUE, TRUE, TRUE, FALSE, FALSE }, + { FALSE, FALSE, FALSE, FALSE, FALSE, TRUE, FALSE }, + { TRUE, FALSE, FALSE, FALSE, FALSE, TRUE, FALSE }, + { TRUE, TRUE, FALSE, FALSE, FALSE, TRUE, FALSE }, + { FALSE, FALSE, TRUE, FALSE, FALSE, TRUE, FALSE }, + { TRUE, FALSE, TRUE, FALSE, FALSE, TRUE, FALSE }, + { TRUE, TRUE, TRUE, FALSE, FALSE, TRUE, FALSE }, + { TRUE, TRUE, FALSE, TRUE, FALSE, TRUE, FALSE }, + { TRUE, TRUE, TRUE, TRUE, FALSE, TRUE, FALSE }, + { FALSE, FALSE, FALSE, FALSE, TRUE, TRUE, FALSE }, + { TRUE, FALSE, FALSE, FALSE, TRUE, TRUE, FALSE }, + { TRUE, TRUE, FALSE, FALSE, TRUE, TRUE, FALSE }, + { FALSE, FALSE, TRUE, FALSE, TRUE, TRUE, FALSE }, + { TRUE, FALSE, TRUE, FALSE, TRUE, TRUE, FALSE }, + { TRUE, TRUE, TRUE, FALSE, TRUE, TRUE, FALSE }, + { TRUE, TRUE, FALSE, TRUE, TRUE, TRUE, FALSE }, + { TRUE, TRUE, TRUE, TRUE, TRUE, TRUE, FALSE }, + { FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, TRUE }, + { TRUE, FALSE, FALSE, FALSE, FALSE, FALSE, TRUE }, + { TRUE, TRUE, FALSE, FALSE, FALSE, FALSE, TRUE }, + { FALSE, FALSE, TRUE, FALSE, FALSE, FALSE, TRUE }, + { TRUE, FALSE, TRUE, FALSE, FALSE, FALSE, TRUE }, + { TRUE, TRUE, TRUE, FALSE, FALSE, FALSE, TRUE }, + { TRUE, TRUE, FALSE, TRUE, FALSE, FALSE, TRUE }, + { TRUE, TRUE, TRUE, TRUE, FALSE, FALSE, TRUE }, + { FALSE, FALSE, FALSE, FALSE, TRUE, FALSE, TRUE }, + { TRUE, FALSE, FALSE, FALSE, TRUE, FALSE, TRUE }, + { TRUE, TRUE, FALSE, FALSE, TRUE, FALSE, TRUE }, + { FALSE, FALSE, TRUE, FALSE, TRUE, FALSE, TRUE }, + { TRUE, FALSE, TRUE, FALSE, TRUE, FALSE, TRUE }, + { TRUE, TRUE, TRUE, FALSE, TRUE, FALSE, TRUE }, + { TRUE, TRUE, FALSE, TRUE, TRUE, FALSE, TRUE }, + { TRUE, TRUE, TRUE, TRUE, TRUE, FALSE, TRUE }, + { FALSE, FALSE, FALSE, FALSE, FALSE, TRUE, TRUE }, + { TRUE, FALSE, FALSE, FALSE, FALSE, TRUE, TRUE }, + { TRUE, TRUE, FALSE, FALSE, FALSE, TRUE, TRUE }, + { FALSE, FALSE, TRUE, FALSE, FALSE, TRUE, TRUE }, + { TRUE, FALSE, TRUE, FALSE, FALSE, TRUE, TRUE }, + { TRUE, TRUE, TRUE, FALSE, FALSE, TRUE, TRUE }, + { TRUE, TRUE, FALSE, TRUE, FALSE, TRUE, TRUE }, + { TRUE, TRUE, TRUE, TRUE, FALSE, TRUE, TRUE }, + { FALSE, FALSE, FALSE, FALSE, TRUE, TRUE, TRUE }, + { TRUE, FALSE, FALSE, FALSE, TRUE, TRUE, TRUE }, + { TRUE, TRUE, FALSE, FALSE, TRUE, TRUE, TRUE }, + { FALSE, FALSE, TRUE, FALSE, TRUE, TRUE, TRUE }, + { TRUE, FALSE, TRUE, FALSE, TRUE, TRUE, TRUE }, + { TRUE, TRUE, TRUE, FALSE, TRUE, TRUE, TRUE }, + { TRUE, TRUE, FALSE, TRUE, TRUE, TRUE, TRUE }, + { TRUE, TRUE, TRUE, TRUE, TRUE, TRUE, TRUE }, + }; + static const NTSTATUS test_status[] = + { + STATUS_SUCCESS, STATUS_PENDING, STATUS_UNEXPECTED_IO_ERROR, + }; + static const ULONG_PTR test_file_bits[] = + { + 0, 1, 2, 3, 0xdeadbeef, + }; + OVERLAPPED ov; - DWORD r, result; + DWORD r, result, err; + HANDLE event; + unsigned int i, status_idx, file_bits_idx; + NTSTATUS iosb_status; + ULONG_PTR file_bits, event_bits;
/* GetOverlappedResult crashes if the 2nd or 3rd param are NULL */ if (0) /* tested: WinXP */ @@ -3793,10 +3880,13 @@ static void test_overlapped(void) "wrong error %lu\n", GetLastError() ); ok( r == FALSE, "should return false\n");
+ SetLastError( 0xdeadbeef ); r = GetOverlappedResult( 0, &ov, &result, TRUE ); ok( r == TRUE, "should return TRUE\n" ); ok( result == 0xabcd, "wrong result %lu\n", result ); ok( ov.Internal == STATUS_PENDING, "expected STATUS_PENDING, got %08Ix\n", ov.Internal ); + err = GetLastError(); + ok( err == ERROR_IO_PENDING || broken( err == 0xdeadbeef ) /* Before Win10 1809 */, "got %lu.\n", GetLastError() );
ResetEvent( ov.hEvent );
@@ -3810,6 +3900,154 @@ static void test_overlapped(void)
r = CloseHandle( ov.hEvent ); ok( r == TRUE, "close handle failed\n"); + + if (!pGetOverlappedResultEx) + { + win_skip( "GetOverlappedResultEx is not available, skipping tests.\n" ); + return; + } + + event = CreateEventW( NULL, FALSE, FALSE, NULL ); + + user_apc_ran = FALSE; + QueueUserAPC( user_apc, GetCurrentThread(), 0 ); + SetEvent( event ); + r = WaitForSingleObjectEx( event, INFINITE, TRUE ); + todo_wine ok( r == WAIT_IO_COMPLETION, "got %lu.\n", r ); + if (!r) SleepEx( 0, TRUE ); + ok( user_apc_ran, "APC was not run.\n" ); + + user_apc_ran = FALSE; + SetEvent( event ); + QueueUserAPC( user_apc, GetCurrentThread(), 0 ); + r = WaitForSingleObjectEx( event, 2, TRUE ); + todo_wine ok( r == WAIT_IO_COMPLETION, "got %lu.\n", r ); + if (!r) SleepEx( 0, TRUE ); + ok( user_apc_ran, "APC was not run.\n" ); + + user_apc_ran = FALSE; + SetEvent( event ); + QueueUserAPC( user_apc, GetCurrentThread(), 0 ); + r = WaitForSingleObjectEx( event, 0, TRUE ); + todo_wine ok( r == WAIT_IO_COMPLETION, "got %lu.\n", r ); + if (!r) SleepEx( 0, TRUE ); + ok( user_apc_ran, "APC was not run.\n" ); + + for (event_bits = 0; event_bits < 2; ++event_bits) + for (file_bits_idx = 0; file_bits_idx < ARRAY_SIZE(test_file_bits); ++file_bits_idx) + { + file_bits = test_file_bits[file_bits_idx]; + for (status_idx = 0; status_idx < ARRAY_SIZE(test_status); ++status_idx) + { + iosb_status = test_status[status_idx]; + for (i = 0; i < ARRAY_SIZE(tests); ++i) + { + BOOL will_wait, wait_fails, wait_alerts, wait_timeouts; + DWORD err; + HANDLE file; + + ov.Internal = iosb_status; + ov.InternalHigh = 0xabcd; + ov.hEvent = (tests[i].pass_event_handle ? event : NULL); + file = (tests[i].pass_file_handle ? event : NULL); + ov.hEvent = (HANDLE)((ULONG_PTR)ov.hEvent | event_bits); + file = (HANDLE)((ULONG_PTR)file | file_bits); + will_wait = tests[i].wait + && (iosb_status == STATUS_PENDING || (tests[i].ex && !(file_bits & 1))); + wait_fails = will_wait && WaitForSingleObject( ov.hEvent ? ov.hEvent : file, 0 ) == WAIT_FAILED; + wait_alerts = will_wait && tests[i].ex && tests[i].alertable && tests[i].queue_apc; + wait_timeouts = will_wait && !wait_fails && !wait_alerts && !tests[i].set_event && !(tests[i].ex && file_bits & 1); + if (will_wait && !tests[i].ex && wait_timeouts) + { + /* This would wait forever. */ + continue; + } + + winetest_push_context( "status %#lx, file_bits %Iu, event_bits %Iu, test %u", + iosb_status, file_bits, event_bits, i ); + + if (tests[i].set_event) + SetEvent( event ); + else + ResetEvent( event ); + + if (tests[i].queue_apc) + QueueUserAPC( user_apc, GetCurrentThread(), 0 ); + + result = 0xdeadbeef; + SetLastError( 0xdeadbeef ); + if (tests[i].ex) + r = pGetOverlappedResultEx( file, &ov, &result, tests[i].wait ? 2 : 0, tests[i].alertable ); + else + r = GetOverlappedResult( file, &ov, &result, tests[i].wait ); + err = GetLastError(); + if (will_wait) + { + if (wait_fails) + { + ok( !r, "got %lu.\n", r ); + ok( err == ERROR_INVALID_HANDLE, "got %lu.\n", err ); + ok( result == 0xdeadbeef, "wrong result %lu\n", result ); + } + else if (wait_alerts) + { + /* todo comes from WaitForSingleObjectEx() with signaled event and queued APC not preferring + * user APC (which is tested above for clarity) */ + todo_wine_if(tests[i].set_event) ok( err == WAIT_IO_COMPLETION, "got %lu.\n", err ); + if (err == WAIT_IO_COMPLETION) + { + ok( !r, "got %lu.\n", r ); + ok( result == 0xdeadbeef, "wrong result %lu\n", result ); + } + } + else if (wait_timeouts) + { + ok( !r, "got %lu.\n", r ); + ok( err == WAIT_TIMEOUT, "got %lu.\n", err ); + ok( result == 0xdeadbeef, "wrong result %lu\n", result ); + } + else + { + ok( r == (iosb_status == STATUS_SUCCESS || iosb_status == STATUS_PENDING), + "got %lu.\n", r ); + ok( err == RtlNtStatusToDosError( iosb_status ) || broken( r && err == 0xdeadbeef ) /* Before Win10 1809 */, + "got %lu.\n", err ); + ok( result == 0xabcd, "wrong result %lu\n", result ); + } + } + else if (iosb_status == STATUS_PENDING) + { + ok( !r, "got %lu.\n", r ); + ok( err == ERROR_IO_INCOMPLETE, "got %lu.\n", err ); + ok( result == 0xdeadbeef, "wrong result %lu\n", result ); + } + else + { + ok( r == (iosb_status == STATUS_SUCCESS || iosb_status == STATUS_PENDING), + "got %lu.\n", r ); + ok( err == RtlNtStatusToDosError( iosb_status ) || broken( r && err == 0xdeadbeef ) /* Before Win10 1809 */, + "got %lu.\n", err ); + ok( result == 0xabcd, "wrong result %lu\n", result ); + } + + r = WaitForSingleObject( event, 0 ); + if (!tests[i].set_event || (will_wait && !wait_fails && !wait_alerts)) + { + ok( r == WAIT_TIMEOUT, "got %#lx.\n", r ); + } + else + { + todo_wine_if(will_wait && !wait_fails && wait_alerts && tests[i].set_event) + ok( r == WAIT_OBJECT_0, "got %#lx.\n", r ); + } + + winetest_pop_context(); + if (tests[i].queue_apc) + SleepEx( 0, TRUE ); + } + } + } + CloseHandle( event ); }
static void test_RemoveDirectory(void) diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index 3e8f470444f..fca611b83f5 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -3212,48 +3212,56 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetFileType( HANDLE file ) BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResult( HANDLE file, LPOVERLAPPED overlapped, LPDWORD result, BOOL wait ) { - return GetOverlappedResultEx( file, overlapped, result, wait ? INFINITE : 0, FALSE ); + TRACE( "(%p %p %p %d)\n", file, overlapped, result, wait ); + + return GetOverlappedResultEx( (HANDLE)((ULONG_PTR)file | 1), overlapped, result, + wait ? INFINITE : 0, FALSE ); }
/*********************************************************************** * GetOverlappedResultEx (kernelbase.@) */ -BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *overlapped, +BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE handle, OVERLAPPED *overlapped, DWORD *result, DWORD timeout, BOOL alertable ) { - NTSTATUS status; + NTSTATUS status = STATUS_PENDING; + BOOL compat_mode; DWORD ret;
- TRACE( "(%p %p %p %lu %d)\n", file, overlapped, result, timeout, alertable ); + TRACE( "(%p %p %p %lu %d)\n", handle, overlapped, result, timeout, alertable );
- /* Paired with the write-release in set_async_iosb() in ntdll; see the - * latter for details. */ - status = ReadAcquire( (LONG *)&overlapped->Internal ); - if (status == STATUS_PENDING) + compat_mode = (ULONG_PTR)handle & 1; + if (compat_mode || !timeout) { - if (!timeout) - { - SetLastError( ERROR_IO_INCOMPLETE ); - return FALSE; - } - ret = WaitForSingleObjectEx( overlapped->hEvent ? overlapped->hEvent : file, timeout, alertable ); - if (ret == WAIT_FAILED) - return FALSE; - else if (ret) + /* Paired with the write-release in set_async_iosb() in ntdll; see the + * latter for details. */ + status = ReadAcquire( (LONG *)&overlapped->Internal ); + } + + if (timeout && status == STATUS_PENDING) + { + if (overlapped->hEvent) handle = overlapped->hEvent; + else handle = (HANDLE)((ULONG_PTR)handle & ~(ULONG_PTR)1); + ret = WaitForSingleObjectEx( handle, timeout, alertable ); + if (ret == WAIT_FAILED) return FALSE; + if (ret && !(compat_mode && ret == WAIT_TIMEOUT)) { SetLastError( ret ); return FALSE; } - /* We don't need to give this load acquire semantics; the wait above * already guarantees that the IOSB and output buffer are filled. */ status = overlapped->Internal; - if (status == STATUS_PENDING) status = STATUS_SUCCESS; } - + else if (status == STATUS_PENDING) + { + SetLastError( ERROR_IO_INCOMPLETE ); + return FALSE; + } *result = overlapped->InternalHigh; - return set_ntstatus( status ); + SetLastError( RtlNtStatusToDosError( status )); + return !status || status == STATUS_PENDING; }
v5: - Add much more tests and organize those with a table and loops; - Handle lower bit set in file handle in GetOverlappedResultEx(); - Still use GetOverlappedResultEx() from GetOverlappedResult() with the "compatibility" flag passed in file handle lower bit.
On the way of testing that I found that our waits, given both signaled state and user APC are available, are preferring user APC. That is the opposite on Windows and the difference is responsible for the TODO tests in the latest patch version (I prepended WaitForSingleObjectEx there which illustrates the difference). I tried to fix that at once in v4 also adding a separate test, but turned out that broke a couple of things which look not that trivial to fix (while the behaviour of Wait itself looks correct with preferring user APC).
One thing is ntdll/tests/pipe.c: test_blocking(FILE_SYNCHRONOUS_IO_ALERT). The call to NtReadFile on pipe with ioapc requested waits alertable for async in server_read_file(). When server finishes async, it both queues io completion user APC and signals the wait object. Signaling async object before queueing APC doesn't help, maybe it is because the wait is not started yet and when it starts both are signaled anyway. I didn't look in all the details yet, not sure how that can be fixed. But broadly this looks like async handling details which don't work quite right, or what if that io completion APC is queued in a bit different way or something is special about it.
Another failure from attempt to fix WaitFor... is in user32/tests/msg.c:test_MsgWaitForMultipleObjects(). The test result directly reads as MsgWaitForMultipleObjectsEx( 0, NULL, 0, QS_POSTMESSAGE,, MWMO_ALERTABLE ) should prefer message queue wake up to the APC (while maybe more things should be tested around to see at least what happens if there are some other wait handles). No idea yet what is the culrpit here, is it maybe message queue is special or some other specifics with MsgWaitForMultipleObjectsEx.
So for now I just made the tests in this patch depending on user APC taking precedence over signaled event state TODO.
Edited comment above, "our waits,... are preferring user APC" -> "our waits,... are preferring signaled state"
Separately, I tried to reproduce a different GetOverlappedResult() (not Ex) behaviour based on app manifest as described here: https://learn.microsoft.com/en-us/windows/win32/w8cookbook/application--exec...). I could not. Tried every supportedOS GUID listed, one by one and together, made sure that those are getting picked up with kernel32.GetVersion result, maxversiontested (while that is Win 10 addition and should not be related). At least on up to date Windows 11 it had no effect on the mentioned difference, so I left this part alone.
-
Paul Gofman -
Paul Gofman (＠gofman)