[PATCH v8 0/2] MR6904: ntdll.dll: Update NtQueryDirectoryFile to align with current Windows behaviour
This PR updates the behaviour of `NtQueryDirectoryFile`, bringing it in line with current Windows behaviour. The need for this update was discovered when attempting to build the Unreal Engine with MSVC under Wine. In certain cases conditional include statements do not behave as expected, due to MSVC depending on undocumented behaviour of `NtQueryDirectoryFile`. We ran tests on multiple versions of Windows, and discovered that the behaviour has changed since the original Wine implementation, but the documentation has not. The source code for our test tool, and a set of results can be found [here](https://github.com/TensorWorks/NtQueryDirectoryFile-Test). As of Windows 8, calling `NtQueryDirectoryFile` with a re-used handle, a new mask, and setting the `RestartScan` flag to True, causes the cached results to be erased and a new scan to be performed with the updated mask. Currently, Wine performs as did earlier versions of Windows, where the changed mask is ignored, and the cache is reused. This can cause `NtQueryDirectoryFile` under Wine to falsely report that files exist, when they do not. This PR corrects this behaviour, invalidating the cache when required. Implementing this exposed further undocumented behaviour of `NtQueryDirectoryFile`, where a search for a non-existent file will return either `STATUS_NO_MORE_FILES` or `STATUS_NO_SUCH_FILE`, depending on whether or not the handle had been previously used regardless of the value of `RestartScan`. This was reflected in a `winetest` which allowed for the response to be either `STATUS_SUCCESS` or `STATUS_NO_MORE_FILES`. This patch also adds unit tests for the new behaviour of `NtQueryDirectoryFile`. These tests pass when running `winetest` under Windows, and under Wine with these changes in place, but they will fail under older versions of Wine. If run under older versions of Windows the test suite will detect that this functionality is not supported, and will not run the updated tests. -- v8: ntdll: Update NtQueryDirectoryFile to purge cache if scan is reset with a new mask ntdll: Test updated NtQueryDirectoryFile mask reset behaviour https://gitlab.winehq.org/wine/wine/-/merge_requests/6904
From: Eugene McArdle <eugene(a)tensorworks.com.au> --- dlls/ntdll/tests/directory.c | 249 +++++++++++++++++++++++++++++++++++ 1 file changed, 249 insertions(+) diff --git a/dlls/ntdll/tests/directory.c b/dlls/ntdll/tests/directory.c index 52ff9936560..6d0ef1d31ef 100644 --- a/dlls/ntdll/tests/directory.c +++ b/dlls/ntdll/tests/directory.c @@ -964,6 +964,254 @@ done: pRtlFreeUnicodeString(&ntdirname); } +static void set_up_mask_test(const char *testdir) +{ + BOOL ret; + char buf[MAX_PATH + 5]; + HANDLE h; + + ret = CreateDirectoryA(testdir, NULL); + ok( ret, "couldn't create dir '%s', error %ld\n", testdir, GetLastError() ); + + sprintf(buf, "%s\\%s", testdir, "a-file.h"); + h = CreateFileA(buf, GENERIC_READ|GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, 0); + ok( h != INVALID_HANDLE_VALUE, "failed to create temp file '%s'\n", buf ); + CloseHandle(h); + + sprintf(buf, "%s\\%s", testdir, "another-file.h"); + h = CreateFileA(buf, GENERIC_READ|GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, 0); + ok( h != INVALID_HANDLE_VALUE, "failed to create temp file '%s'\n", buf ); + CloseHandle(h); +} + +static void tear_down_mask_test(const char *testdir) +{ + int ret; + char buf[MAX_PATH]; + + sprintf(buf, "%s\\%s", testdir, "a-file.h"); + ret = DeleteFileA(buf); + ok( ret || (GetLastError() == ERROR_FILE_NOT_FOUND) || (GetLastError() == ERROR_PATH_NOT_FOUND), + "Failed to rm %s, error %ld\n", buf, GetLastError() ); + + sprintf(buf, "%s\\%s", testdir, "another-file.h"); + ret = DeleteFileA(buf); + ok( ret || (GetLastError() == ERROR_FILE_NOT_FOUND) || (GetLastError() == ERROR_PATH_NOT_FOUND), + "Failed to rm %s, error %ld\n", buf, GetLastError() ); + + RemoveDirectoryA(testdir); +} + + +/* Performs a query with NtQueryDirectoryFile() */ +static BOOL test_NtQueryDirectoryFile_mask(HANDLE handle, BOOL restart_scan, UNICODE_STRING *mask, + NTSTATUS expected_status, UNICODE_STRING *match, BOOL validate_only) +{ + NTSTATUS status; + IO_STATUS_BLOCK io; + UINT data_size = sizeof(FILE_DIRECTORY_INFORMATION) + (MAX_PATH * sizeof(wchar_t)); + BYTE data[8192]; + FILE_DIRECTORY_INFORMATION *dir_info; + WCHAR *name; + ULONG name_len; + WCHAR *mask_value = {0}; + ULONG mask_len = 0; + WCHAR *match_value = {0}; + ULONG match_len = 0; + + if (mask) + { + mask_len = mask->Length / sizeof(WCHAR); + mask_value = mask->Buffer; + } + + /* Perform the query */ + status = pNtQueryDirectoryFile( handle, NULL, NULL, NULL, &io, data, data_size, + FileDirectoryInformation, TRUE, mask, restart_scan ); + + if (validate_only && status != expected_status) return FALSE; + + todo_wine_if(status != expected_status) + ok( status == expected_status, "unexpected status : 0x%lx Test settings: file mask: '%s', restart: %d, expected status: 0x%lx\n", + status, wine_dbgstr_wn(mask_value, mask_len), restart_scan, expected_status ); + + /* Verify the results if required */ + if (status == 0 && match != NULL) + { + dir_info = (FILE_DIRECTORY_INFORMATION *)data; + name = dir_info->FileName; + name_len = dir_info->FileNameLength / sizeof(WCHAR); + match_len = match->Length / sizeof(WCHAR); + match_value = match->Buffer; + + todo_wine_if(name_len != match_len) + ok( name_len == match_len, "unexpected filename length %lu, expected %lu\n", name_len, match_len ); + todo_wine_if(name_len != match_len) + ok( !memcmp(name, match_value, match_len * sizeof(WCHAR)), "unexpected filename %s, expected %s\n", + wine_dbgstr_wn(name, name_len), wine_dbgstr_wn(match_value, match_len) ); + } + return TRUE; +} + +static void test_NtQueryDirectoryFile_change_mask(void) +{ + NTSTATUS status; + HANDLE dirh; + HANDLE dirh_test_multiple_handles; + HANDLE dirh_test_fresh_null; + HANDLE dirh_test_fresh_empty; + char testdir[MAX_PATH]; + OBJECT_ATTRIBUTES attr; + IO_STATUS_BLOCK io; + UNICODE_STRING ntdirname; + WCHAR testdir_w[MAX_PATH]; + + UNICODE_STRING atestfile; + UNICODE_STRING anothertestfile; + UNICODE_STRING notatestfile; + UNICODE_STRING testmask; + UNICODE_STRING emptymask; + + pRtlInitUnicodeString(&atestfile, L"a-file.h"); + pRtlInitUnicodeString(&anothertestfile, L"another-file.h"); + pRtlInitUnicodeString(¬atestfile, L"not-a-file.h"); + pRtlInitUnicodeString(&testmask, L"*.h"); + pRtlInitUnicodeString(&emptymask, L""); + + /* Clean up from prior aborted run, if any, then set up test files */ + ok( GetTempPathA(MAX_PATH, testdir), "couldn't get temp dir\n" ); + strcat(testdir, "mask.tmp"); + tear_down_mask_test(testdir); + set_up_mask_test(testdir); + + pRtlMultiByteToUnicodeN(testdir_w, sizeof(testdir_w), NULL, testdir, strlen(testdir) + 1); + if (!pRtlDosPathNameToNtPathName_U(testdir_w, &ntdirname, NULL, NULL)) + { + ok( 0, "RtlDosPathNametoNtPathName_U failed\n" ); + goto done; + } + InitializeObjectAttributes(&attr, &ntdirname, OBJ_CASE_INSENSITIVE, 0, NULL); + + /* Open a handle for our test directory */ + status = pNtOpenFile(&dirh, SYNCHRONIZE | FILE_LIST_DIRECTORY, &attr, &io, FILE_SHARE_READ, + FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_FOR_BACKUP_INTENT | FILE_DIRECTORY_FILE); + ok( status == STATUS_SUCCESS, "failed to open dir '%s', ret 0x%lx, error %ld\n", testdir, status, GetLastError() ); + if (status != STATUS_SUCCESS) + { + skip("can't test if we can't open the directory\n"); + goto done; + } + + /* Verify that NtQueryDirectoryFile mask reset behaviour introduced in Windows 8 is supported */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, &atestfile, STATUS_SUCCESS, &atestfile, TRUE); + if (!winetest_platform_is_wine && !test_NtQueryDirectoryFile_mask(dirh, TRUE, ¬atestfile, STATUS_NO_MORE_FILES, NULL, TRUE)) + { + skip("Win8+ NtQueryDirectoryFile mask reset behaviour not supported\n"); + goto done; + } + + /* Test that caches for two handles to a single directory do not interfere with each other*/ + /* Open a handle for our test directory to test multiple handles */ + status = pNtOpenFile(&dirh_test_multiple_handles, SYNCHRONIZE | FILE_LIST_DIRECTORY, &attr, &io, FILE_SHARE_READ, + FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_FOR_BACKUP_INTENT | FILE_DIRECTORY_FILE); + ok( status == STATUS_SUCCESS, "failed to open second handle to dir '%s', ret 0x%lx, error %ld\n", testdir, status, GetLastError() ); + if (status != STATUS_SUCCESS) + { + skip("can't test if we can't open a second handle to the directory\n"); + goto done; + } + /* Search for a non-existent file, putting it into a state with an empty cache */ + /* This also confirms that using a non-existent mask with a fresh handle returns a different status*/ + test_NtQueryDirectoryFile_mask(dirh_test_multiple_handles, TRUE, ¬atestfile, STATUS_NO_SUCH_FILE, ¬atestfile, FALSE); + /* Confirm that handle is in a state where it fails to find atestfile */ + test_NtQueryDirectoryFile_mask(dirh_test_multiple_handles, FALSE, &atestfile, STATUS_NO_MORE_FILES, &atestfile, FALSE); + /* Confirm that another handle is able to find atestfile */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, &atestfile, STATUS_SUCCESS, &atestfile, FALSE); + /* Close handle used for testing multiple handles */ + pNtClose(&dirh_test_multiple_handles); + + /* All searches for `notatestfile` are expected to fail */ + /* Tests should also fail if the scan is not reset, and the mask changes to an incompatible one */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, &atestfile, STATUS_SUCCESS, &atestfile, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, &anothertestfile, STATUS_SUCCESS, &anothertestfile, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, ¬atestfile, STATUS_NO_MORE_FILES, ¬atestfile, FALSE); + + test_NtQueryDirectoryFile_mask(dirh, TRUE, &atestfile, STATUS_SUCCESS, &atestfile, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, ¬atestfile, STATUS_NO_MORE_FILES, ¬atestfile, FALSE); + + test_NtQueryDirectoryFile_mask(dirh, TRUE, &atestfile, STATUS_SUCCESS, &atestfile, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, &anothertestfile, STATUS_NO_MORE_FILES, &anothertestfile, FALSE); + + /* Test mask that matches multiple files, do not check results against the mask */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, &testmask, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, ¬atestfile, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, ¬atestfile, STATUS_NO_MORE_FILES, NULL, FALSE); + + /* Test NULL mask with a previously used handle that last returned an error */ + /* Ensure dirh is in a failure state */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, ¬atestfile, STATUS_NO_MORE_FILES, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, NULL, STATUS_NO_MORE_FILES, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, NULL, STATUS_NO_MORE_FILES, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, NULL, STATUS_NO_MORE_FILES, NULL, FALSE); + + /* Test NULL mask with a previously used handle that last returned STATUS_SUCCESS */ + /* Ensure dirh is in a success state */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, &testmask, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, NULL, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, NULL, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, NULL, STATUS_SUCCESS, NULL, FALSE); + + /* Test empty mask with a previously used handle that last returned an error */ + /* Ensure dirh is in a failure state */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, ¬atestfile, STATUS_NO_MORE_FILES, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, &emptymask, STATUS_NO_MORE_FILES, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, &emptymask, STATUS_NO_MORE_FILES, NULL, FALSE); + + /* Test empty mask with a previously used handle that last returned STATUS_SUCCESS */ + /* Ensure dirh is in a success state */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, &testmask, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, &emptymask, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, &emptymask, STATUS_SUCCESS, NULL, FALSE); + + /* Open a handle for our test directory to test null masks with a fresh handle */ + status = pNtOpenFile(&dirh_test_fresh_null, SYNCHRONIZE | FILE_LIST_DIRECTORY, &attr, &io, FILE_SHARE_READ, + FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_FOR_BACKUP_INTENT | FILE_DIRECTORY_FILE); + ok( status == STATUS_SUCCESS, "failed to open handle to dir '%s' for testing NULL masks, ret 0x%lx, error %ld\n", testdir, status, GetLastError() ); + if (status != STATUS_SUCCESS) + { + skip("can't test NULL masks if we can't open an additional handle to the directory\n"); + goto done; + } + /* Test NULL mask with a fresh handle */ + test_NtQueryDirectoryFile_mask(dirh_test_fresh_null, TRUE, NULL, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh_test_fresh_null, FALSE, NULL, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh_test_fresh_null, TRUE, NULL, STATUS_SUCCESS, NULL, FALSE); + /* Release fresh handle for null mask tests */ + pNtClose(&dirh_test_fresh_null); + + /* Open a handle for our test directory to test empty masks with a fresh handle */ + status = pNtOpenFile(&dirh_test_fresh_empty, SYNCHRONIZE | FILE_LIST_DIRECTORY, &attr, &io, FILE_SHARE_READ, + FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_FOR_BACKUP_INTENT | FILE_DIRECTORY_FILE); + ok( status == STATUS_SUCCESS, "failed to open handle to dir '%s' for testing empty masks, ret 0x%lx, error %ld\n", testdir, status, GetLastError() ); + if (status != STATUS_SUCCESS) + { + skip("can't test empty masks if we can't open an additional handle to the directory\n"); + goto done; + } + /* Test empty mask with a fresh handle */ + test_NtQueryDirectoryFile_mask(dirh_test_fresh_empty, TRUE, &emptymask, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh_test_fresh_empty, FALSE, &emptymask, STATUS_SUCCESS, NULL, FALSE); + /* Release fresh handle for empty mask tests */ + pNtClose(&dirh_test_fresh_empty); + + /* Cleanup */ +done: + tear_down_mask_test(testdir); + pNtClose(&dirh); +} + static NTSTATUS get_file_id( FILE_INTERNAL_INFORMATION *info, const WCHAR *root, const WCHAR *name ) { OBJECT_ATTRIBUTES attr; @@ -1157,5 +1405,6 @@ START_TEST(directory) test_directory_sort( sysdir ); test_NtQueryDirectoryFile(); test_NtQueryDirectoryFile_case(); + test_NtQueryDirectoryFile_change_mask(); test_redirection(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6904
From: Eugene McArdle <eugene(a)tensorworks.com.au> --- dlls/ntdll/tests/directory.c | 11 +++--- dlls/ntdll/unix/file.c | 67 ++++++++++++++++++++++++++++++++---- 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/dlls/ntdll/tests/directory.c b/dlls/ntdll/tests/directory.c index 6d0ef1d31ef..0121a089720 100644 --- a/dlls/ntdll/tests/directory.c +++ b/dlls/ntdll/tests/directory.c @@ -1020,7 +1020,7 @@ static BOOL test_NtQueryDirectoryFile_mask(HANDLE handle, BOOL restart_scan, UNI ULONG mask_len = 0; WCHAR *match_value = {0}; ULONG match_len = 0; - + if (mask) { mask_len = mask->Length / sizeof(WCHAR); @@ -1030,10 +1030,9 @@ static BOOL test_NtQueryDirectoryFile_mask(HANDLE handle, BOOL restart_scan, UNI /* Perform the query */ status = pNtQueryDirectoryFile( handle, NULL, NULL, NULL, &io, data, data_size, FileDirectoryInformation, TRUE, mask, restart_scan ); - + if (validate_only && status != expected_status) return FALSE; - todo_wine_if(status != expected_status) ok( status == expected_status, "unexpected status : 0x%lx Test settings: file mask: '%s', restart: %d, expected status: 0x%lx\n", status, wine_dbgstr_wn(mask_value, mask_len), restart_scan, expected_status ); @@ -1045,10 +1044,8 @@ static BOOL test_NtQueryDirectoryFile_mask(HANDLE handle, BOOL restart_scan, UNI name_len = dir_info->FileNameLength / sizeof(WCHAR); match_len = match->Length / sizeof(WCHAR); match_value = match->Buffer; - - todo_wine_if(name_len != match_len) + ok( name_len == match_len, "unexpected filename length %lu, expected %lu\n", name_len, match_len ); - todo_wine_if(name_len != match_len) ok( !memcmp(name, match_value, match_len * sizeof(WCHAR)), "unexpected filename %s, expected %s\n", wine_dbgstr_wn(name, name_len), wine_dbgstr_wn(match_value, match_len) ); } @@ -1137,7 +1134,7 @@ static void test_NtQueryDirectoryFile_change_mask(void) test_NtQueryDirectoryFile_mask(dirh, TRUE, &atestfile, STATUS_SUCCESS, &atestfile, FALSE); test_NtQueryDirectoryFile_mask(dirh, TRUE, &anothertestfile, STATUS_SUCCESS, &anothertestfile, FALSE); test_NtQueryDirectoryFile_mask(dirh, TRUE, ¬atestfile, STATUS_NO_MORE_FILES, ¬atestfile, FALSE); - + test_NtQueryDirectoryFile_mask(dirh, TRUE, &atestfile, STATUS_SUCCESS, &atestfile, FALSE); test_NtQueryDirectoryFile_mask(dirh, FALSE, ¬atestfile, STATUS_NO_MORE_FILES, ¬atestfile, FALSE); diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 13dc36b42ff..9265a2e3b11 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -225,6 +225,7 @@ struct dir_data struct file_identity id; /* directory file identity */ struct dir_data_names *names; /* directory file names */ struct dir_data_buffer *buffer; /* head of data buffers list */ + UNICODE_STRING mask; /* the mask used when creating the cache entry */ }; static const unsigned int dir_data_buffer_initial_size = 4096; @@ -565,6 +566,7 @@ static void free_dir_data( struct dir_data *data ) free( buffer ); } free( data->names ); + if (data->mask.Buffer) free( data->mask.Buffer ); free( data ); } @@ -1595,7 +1597,7 @@ static BOOL append_entry( struct dir_data *data, const char *long_name, TRACE( "long %s short %s mask %s\n", debugstr_w( long_nameW ), debugstr_w( short_nameW ), debugstr_us( mask )); - if (mask && !match_filename( long_nameW, long_len, mask )) + if (mask && mask->Length != 0 && !match_filename( long_nameW, long_len, mask )) { if (!short_len) return TRUE; /* no short name to match */ if (!match_filename( short_nameW, short_len, mask )) return TRUE; @@ -2606,6 +2608,14 @@ static NTSTATUS init_cached_dir_data( struct dir_data **data_ret, int fd, const return status; } + /* if a mask was specified then copy it into the cache entry */ + if (mask && mask->Length) + { + data->mask.Length = data->mask.MaximumLength = mask->Length; + if (!(data->mask.Buffer = calloc( 1, mask->Length ))) return STATUS_NO_MEMORY; + memcpy(data->mask.Buffer, mask->Buffer, mask->Length); + } + /* sort filenames, but not "." and ".." */ i = 0; if (i < data->count && !strcmp( data->names[i].unix_name, "." )) i++; @@ -2628,17 +2638,34 @@ static NTSTATUS init_cached_dir_data( struct dir_data **data_ret, int fd, const } +/*********************************************************************** + * ustring_equal + * + * Simplified version of RtlEqualUnicodeString that performs only case-sensitive comparisons. + */ +static BOOLEAN ustring_equal( const UNICODE_STRING *a, const UNICODE_STRING *b ) +{ + USHORT length_a = (a ? a->Length : 0); + USHORT length_b = (b ? b->Length : 0); + if (length_a != length_b) return FALSE; + + if (length_a == 0 && length_b == 0) return TRUE; + return (!memcmp(a->Buffer, b->Buffer, a->Length)); +} + + /*********************************************************************** * get_cached_dir_data * * Retrieve the cached directory data, or initialize it if necessary. */ static unsigned int get_cached_dir_data( HANDLE handle, struct dir_data **data_ret, int fd, - const UNICODE_STRING *mask ) + const UNICODE_STRING *mask, BOOLEAN restart_scan ) { unsigned int i; int entry = -1, free_entries[16]; unsigned int status; + BOOLEAN fresh_handle = TRUE; SERVER_START_REQ( get_directory_cache_entry ) { @@ -2675,7 +2702,30 @@ static unsigned int get_cached_dir_data( HANDLE handle, struct dir_data **data_r dir_data_cache_size = size; } - if (!dir_data_cache[entry]) status = init_cached_dir_data( &dir_data_cache[entry], fd, mask ); + /* If we have an existing cache entry then set the flag to indicate that the handle is not fresh */ + if (dir_data_cache[entry]) fresh_handle = FALSE; + + /* If we have an existing cache entry, but restart_scan is true and a new non-empty mask + was specified then we need to invalidate the existing cache entry and create a new one + */ + if (dir_data_cache[entry] && restart_scan && mask && mask->Length != 0 && + !ustring_equal(&dir_data_cache[entry]->mask, mask)) + { + TRACE( "invalidating existing cache entry for handle %p, old mask: \"%s\", new mask: \"%s\"\n", + handle, debugstr_us(&(dir_data_cache[entry]->mask)), debugstr_us(mask)); + free_dir_data( dir_data_cache[entry] ); + dir_data_cache[entry] = NULL; + } + + if (!dir_data_cache[entry]) + { + status = init_cached_dir_data( &dir_data_cache[entry], fd, mask ); + /* Return STATUS_NO_MORE_FILES if a mask did not match a file and the handle had previously been used */ + if (status == STATUS_NO_SUCH_FILE && !fresh_handle) + { + status = STATUS_NO_MORE_FILES; + } + } *data_ret = dir_data_cache[entry]; return status; @@ -2746,7 +2796,8 @@ NTSTATUS WINAPI NtQueryDirectoryFile( HANDLE handle, HANDLE event, PIO_APC_ROUTI cwd = open( ".", O_RDONLY ); if (fchdir( fd ) != -1) { - if (!(status = get_cached_dir_data( handle, &data, fd, mask ))) + status = get_cached_dir_data( handle, &data, fd, mask, restart_scan ); + if (!status) { union file_directory_info *last_info = NULL; @@ -2761,13 +2812,17 @@ NTSTATUS WINAPI NtQueryDirectoryFile( HANDLE handle, HANDLE event, PIO_APC_ROUTI if (!last_info) status = STATUS_NO_MORE_FILES; else if (status == STATUS_MORE_ENTRIES) status = STATUS_SUCCESS; - - io->Status = status; } if (cwd == -1 || fchdir( cwd ) == -1) chdir( "/" ); } else status = errno_to_status( errno ); + /* io->Status should only update if status isn't STATUS_NO_SUCH_FILE */ + if (status != STATUS_NO_SUCH_FILE) + { + io->Status = status; + } + mutex_unlock( &dir_mutex ); if (needs_close) close( fd ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6904
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=151117 Your paranoid android. === debian11b (64 bit WoW report) === mf: mf.c:6272: Test failed: Unexpected hr 0. mf.c:6276: Test failed: Unexpected hr 0. user32: input.c:4306: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000005C00F6, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
On Wed Jan 29 05:12:53 2025 +0000, eric pouech wrote:
nitpick: for readibility of the tests, you'd rather move that close to the part of the tests where you use the created handle I've moved all the additional handle creation and release to the came code area where they are used
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93155
On Wed Jan 29 05:11:07 2025 +0000, Eugene McArdle wrote:
changed this line in [version 8 of the diff](/wine/wine/-/merge_requests/6904/diffs?diff_id=154680&start_sha=71a97a7ac32cc5aec09fac83588038348c80ca20#7fa7ee56f28b04717b114bfb52e12634ff21b2e2_1045_1048) No reason at all, I neglected to account for the size of the WCHAR's
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93156
On Wed Jan 29 05:14:34 2025 +0000, eric pouech wrote:
this generates tons of failures... as you're fixing it in next patch, it's fine to use todo_wine_if(status != expected_status) same remark applies to the other todo_wine calls in this function This is now used in place of `todo_wine`
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93157
On Wed Jan 29 05:15:54 2025 +0000, eric pouech wrote:
it looks to me that the test would be more complete if instead of check_name_and_length argument you'd pass a unicode string (assuming all STATUS_SUCCESS should have a matching UNICODE_STRING), while the status returning an error should have a NULL unicode string. and the test below for the returned string should be tested agains that string and not the input mask Agreed, it makes more sense to have a comparison string and not just compare to the input mask. There are a few tests which use wildcards, which was why this flag was initially added, but now I simply pass a NULL unicode string and the comparison is not done.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93158
On Wed Jan 29 05:11:08 2025 +0000, Eugene McArdle wrote:
changed this line in [version 8 of the diff](/wine/wine/-/merge_requests/6904/diffs?diff_id=154680&start_sha=71a97a7ac32cc5aec09fac83588038348c80ca20#7fa7ee56f28b04717b114bfb52e12634ff21b2e2_1170_1200) Fair enough. I had these as TRUE since their purpose was to ensure the handle was in the expected state, and so they weren't testing anything new. However, there is no reason not to check that this has behaved as expected.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93159
On Wed Jan 29 05:11:09 2025 +0000, Eugene McArdle wrote:
changed this line in [version 8 of the diff](/wine/wine/-/merge_requests/6904/diffs?diff_id=154680&start_sha=71a97a7ac32cc5aec09fac83588038348c80ca20#51bc191c8a1fc172f9ad299dd009ef2d24dfa043_1600_1600) This was actually an edge case I discovered when adding tests using fresh handles with NULL and empty masks. Under Windows, using a fresh handle with an empty mask will produce STATUS_SUCCESS, but under Wine it fails. The tests added on lines 1201 and 1202 will both fail under Wine if this condition is removed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93160
On Wed Jan 29 05:24:31 2025 +0000, eric pouech wrote:
@eugenemcardle hi... sorry for the late reply, been busy on some other stuffs a bunch of comments to improve the merge request (the overall logic looks ok, the tests are covering most of the interesting aspects, mostly code cleanup, simplification & readbility) @epo Hi, not a problem, I'm sure the Wine 10 release was a hectic time (and congrats, by the way)
I've resolved most of your comments above, please let me know if I haven't resolved anything to your satisfaction, or if you have any further questions or comments. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93161
On Wed Jan 29 04:57:53 2025 +0000, Adam Rehn wrote:
Removing this field and its associated checks would result in flushing the cache every time the scan is restarted, regardless of whether the mask has changed. This would represent something of a departure from the existing approach to caching, which is to reuse the cache wherever possible. Given that flushing the cache involves making an RPC call to the Wineserver, there is the potential that a minor difference in behaviour here could have meaningful performance implications for applications that restart scans frequently (particularly if applications do so concurrently from multiple processes or threads, resulting in contention over the Wineserver RPC call). We'll conduct some performance analysis to determine the impact on relevant workloads and post the results here to inform further discussion. Adam, agreeing on these remarks. Real life impact would however vary from usage of rescan (between restart keeping current mask vs use a different mask). Out of curiosity, do you have some figures to share here from your current test cases?
My thoughts behind the question deals in fact with the data structures used... I wondered if the mask should be part of the cache, or part of an iterator on top of the directory view (meaning that the cached directory view shouldn't be filtered by the mask). That would cleanly separate iterator operation (restart, set a different mask) from flushing the cache entry. But that would have other impacts. So I'm not considering it for this MR. So ok to keep the mask inside the cache entry. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93174
eric pouech (@epo) commented about dlls/ntdll/tests/directory.c:
+ /* Ensure dirh is in a failure state */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, ¬atestfile, STATUS_NO_MORE_FILES, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, &emptymask, STATUS_NO_MORE_FILES, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, &emptymask, STATUS_NO_MORE_FILES, NULL, FALSE); + + /* Test empty mask with a previously used handle that last returned STATUS_SUCCESS */ + /* Ensure dirh is in a success state */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, &testmask, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, &emptymask, STATUS_SUCCESS, NULL, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, &emptymask, STATUS_SUCCESS, NULL, FALSE); + + /* Open a handle for our test directory to test null masks with a fresh handle */ + status = pNtOpenFile(&dirh_test_fresh_null, SYNCHRONIZE | FILE_LIST_DIRECTORY, &attr, &io, FILE_SHARE_READ, + FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_FOR_BACKUP_INTENT | FILE_DIRECTORY_FILE); + ok( status == STATUS_SUCCESS, "failed to open handle to dir '%s' for testing NULL masks, ret 0x%lx, error %ld\n", testdir, status, GetLastError() ); + if (status != STATUS_SUCCESS) there's no need to skip the rest of the test here. from what I've seen the first test for checking that the feature is present should be sufficient.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93177
eric pouech (@epo) commented about dlls/ntdll/tests/directory.c:
+ } + /* Search for a non-existent file, putting it into a state with an empty cache */ + /* This also confirms that using a non-existent mask with a fresh handle returns a different status*/ + test_NtQueryDirectoryFile_mask(dirh_test_multiple_handles, TRUE, ¬atestfile, STATUS_NO_SUCH_FILE, ¬atestfile, FALSE); + /* Confirm that handle is in a state where it fails to find atestfile */ + test_NtQueryDirectoryFile_mask(dirh_test_multiple_handles, FALSE, &atestfile, STATUS_NO_MORE_FILES, &atestfile, FALSE); + /* Confirm that another handle is able to find atestfile */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, &atestfile, STATUS_SUCCESS, &atestfile, FALSE); + /* Close handle used for testing multiple handles */ + pNtClose(&dirh_test_multiple_handles); + + /* All searches for `notatestfile` are expected to fail */ + /* Tests should also fail if the scan is not reset, and the mask changes to an incompatible one */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, &atestfile, STATUS_SUCCESS, &atestfile, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, &anothertestfile, STATUS_SUCCESS, &anothertestfile, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, ¬atestfile, STATUS_NO_MORE_FILES, ¬atestfile, FALSE); it doesn't make sense to pass ¬atestfile when expected status isn't STATUS_SUCCESS
IOW, all tests with expected_status != STATUS_SUCCESS should have NULL as expected filename (actually, we could even think that for the expected_status == STATUS_SUCCESS case, the expected filename isn't NULL, but that would requires some more work as a NULL mask generates other filenames starting with "." and "..") -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93176
eric pouech (@epo) commented about dlls/ntdll/tests/directory.c:
ULONG mask_len = 0; WCHAR *match_value = {0}; ULONG match_len = 0; - +
that should be part of previous patch -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93178
eric pouech (@epo) commented about dlls/ntdll/unix/file.c:
dir_data_cache_size = size; }
- if (!dir_data_cache[entry]) status = init_cached_dir_data( &dir_data_cache[entry], fd, mask ); + /* If we have an existing cache entry then set the flag to indicate that the handle is not fresh */ + if (dir_data_cache[entry]) fresh_handle = FALSE;
fresh_handle = dir_data_cache\[entry\] == NULL; should be simpler (and removing init in fresh_handle declaration) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93181
eric pouech (@epo) commented about dlls/ntdll/unix/file.c:
TRACE( "long %s short %s mask %s\n", debugstr_w( long_nameW ), debugstr_w( short_nameW ), debugstr_us( mask ));
- if (mask && !match_filename( long_nameW, long_len, mask )) + if (mask && mask->Length != 0 && !match_filename( long_nameW, long_len, mask ))
wouldn't it be simpler for factorize that test in caller (and pass NULL for mask when its length is 0) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93180
eric pouech (@epo) commented about dlls/ntdll/tests/directory.c:
/* Perform the query */ status = pNtQueryDirectoryFile( handle, NULL, NULL, NULL, &io, data, data_size, FileDirectoryInformation, TRUE, mask, restart_scan ); - +
ditto -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93179
eric pouech (@epo) commented about dlls/ntdll/unix/file.c:
cwd = open( ".", O_RDONLY ); if (fchdir( fd ) != -1) { - if (!(status = get_cached_dir_data( handle, &data, fd, mask ))) + status = get_cached_dir_data( handle, &data, fd, mask, restart_scan ); + if (!status) { union file_directory_info *last_info = NULL;
if (restart_scan) data->pos = 0;
nitpick: that would make more sense to move this line to get_cached_dir_data -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93182
eric pouech (@epo) commented about dlls/ntdll/unix/file.c:
if (!last_info) status = STATUS_NO_MORE_FILES; else if (status == STATUS_MORE_ENTRIES) status = STATUS_SUCCESS; - - io->Status = status; } if (cwd == -1 || fchdir( cwd ) == -1) chdir( "/" ); } else status = errno_to_status( errno );
+ /* io->Status should only update if status isn't STATUS_NO_SUCH_FILE */ + if (status != STATUS_NO_SUCH_FILE)
for the record, my local win10 VM sets io-\>Status even for STATUS_NO_SUCH_FILE, but checked on Wine's testbot, and none of the listed Win10&11 sets it. so it's likely been updated in recent VM (Wine's testbot windows versions haven't been updated for a while. Fine to keep it as it is. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93183
looks almost good. a couple of stylistic and code optimization. Note you still have lines with only white spaces in the MR that could be cleaned up -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93184
On Wed Jan 29 22:46:11 2025 +0000, eric pouech wrote:
it doesn't make sense to pass ¬atestfile when expected status isn't STATUS_SUCCESS IOW, all tests with expected_status != STATUS_SUCCESS should have NULL as expected filename (actually, we could even think that for the expected_status == STATUS_SUCCESS case, the expected filename isn't NULL, but that would requires some more work as a NULL mask generates other filenames starting with "." and "..") You're absolutely right, in the case where status isn't STATUS_SUCCESS the name check is never done, so there is no point in passing it in at all. I'll correct this and replace it with NULL.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93240
On Wed Jan 29 22:58:10 2025 +0000, eric pouech wrote:
for the record, my local win10 VM sets io-\>Status even for STATUS_NO_SUCH_FILE, but checked on Wine's testbot, and none of the listed Win10&11 sets it. so it's likely been updated in recent VM (Wine's testbot windows versions haven't been updated for a while. Fine to keep it as it is. Interesting, I wonder what the logic behind not setting it was originally...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_93244
participants (4)
-
eric pouech (＠epo) -
Eugene McArdle
-
Eugene McArdle (＠eugenemcardle)
-
Marvin