[PATCH v43 0/2] MR10291: Fix GetModuleFileName string termination
After Windows XP the string termination behavior of GetModuleFileName was changed to always terminate the path returned, even if the buffer is insufficient to contain the null terminator. -- v43: kernel32: Fix string termination of GetModuleFileName. https://gitlab.winehq.org/wine/wine/-/merge_requests/10291
From: Trent Waddington <trent.waddington@tensorworks.com.au> --- dlls/kernel32/tests/loader.c | 98 ++++++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 529b9de870d..64e8c446a90 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -1552,6 +1552,102 @@ static void test_filenames(void) DeleteFileA( long_path ); } +static void test_getmodulefilenamew_string_termination(void) +{ + WCHAR dll_name[MAX_PATH]; + int dll_name_len, dll_name_term; + DWORD rv, err; + HMODULE mod; + + GetModuleFileNameW(NULL, dll_name, MAX_PATH); + err = GetLastError(); + ok(err == ERROR_SUCCESS, "error getting path for NULL module: %d\n", (int)err); + dll_name_len = wcslen(dll_name); + ok(dll_name_len > 0, "can't get path for NULL module\n"); + ok(dll_name_len < MAX_PATH, "unterminated path for NULL module\n"); + + memset(dll_name, '*', sizeof(dll_name)); + rv = GetModuleFileNameW(NULL, dll_name, dll_name_len); + err = GetLastError(); + ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for NULL module with short buffer: %d\n", (int)err); + dll_name_term = wcsnlen(dll_name, MAX_PATH); + ok(dll_name_term > 0, "can't get path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + todo_wine { + ok(dll_name_term < MAX_PATH, "unterminated path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for NULL module with short buffer. Expected %d got %d. rv=%d\n", dll_name_len - 1, dll_name_term, (int)rv); + } + + mod = LoadLibraryW( L"user32.dll" ); + ok(mod != NULL, "can't load test dll\n"); + GetModuleFileNameW(mod, dll_name, MAX_PATH); + err = GetLastError(); + ok(err == ERROR_SUCCESS, "error getting path for test module: %d\n", (int)err); + dll_name_len = wcslen(dll_name); + ok(dll_name_len > 0, "can't get path for test module\n"); + ok(dll_name_len < MAX_PATH, "unterminated path for test module\n"); + + memset(dll_name, '*', sizeof(dll_name)); + GetModuleFileNameW(mod, dll_name, dll_name_len); + err = GetLastError(); + ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for test module with short buffer: %d\n", (int)err); + dll_name_term = wcsnlen(dll_name, MAX_PATH); + ok(dll_name_term > 0, "can't get path for test module with short buffer, dll_name_term=%d\n", dll_name_term); + todo_wine { + ok(dll_name_term < MAX_PATH, "unterminated path for test module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for test module with short buffer. Expected %d got %d\n", dll_name_len - 1, dll_name_term); + } + + FreeLibrary( mod ); +} + +static void test_getmodulefilenamea_string_termination(void) +{ + char dll_name[MAX_PATH]; + int dll_name_len, dll_name_term; + DWORD rv, err; + HMODULE mod; + + GetModuleFileNameA(NULL, dll_name, MAX_PATH); + err = GetLastError(); + ok(err == ERROR_SUCCESS, "error getting path for NULL module: %d\n", (int)err); + dll_name_len = strlen(dll_name); + ok(dll_name_len > 0, "can't get path for NULL module\n"); + ok(dll_name_len < MAX_PATH, "unterminated path for NULL module\n"); + + memset(dll_name, '*', sizeof(dll_name)); + rv = GetModuleFileNameA(NULL, dll_name, dll_name_len); + err = GetLastError(); + ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for NULL module with short buffer: %d\n", (int)err); + dll_name_term = strnlen(dll_name, MAX_PATH); + ok(dll_name_term > 0, "can't get path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + todo_wine { + ok(dll_name_term < MAX_PATH, "unterminated path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for NULL module with short buffer. Expected %d got %d. rv=%d\n", dll_name_len - 1, dll_name_term, (int)rv); + } + + mod = LoadLibraryA( "user32.dll" ); + ok(mod != NULL, "can't load test dll\n"); + GetModuleFileNameA(mod, dll_name, MAX_PATH); + err = GetLastError(); + ok(err == ERROR_SUCCESS, "error getting path for test module: %d\n", (int)err); + dll_name_len = strlen(dll_name); + ok(dll_name_len > 0, "can't get path for test module\n"); + ok(dll_name_len < MAX_PATH, "unterminated path for test module\n"); + + memset(dll_name, '*', sizeof(dll_name)); + GetModuleFileNameA(mod, dll_name, dll_name_len); + err = GetLastError(); + ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for test module with short buffer: %d\n", (int)err); + dll_name_term = strnlen(dll_name, MAX_PATH); + ok(dll_name_term > 0, "can't get path for test module with short buffer, dll_name_term=%d\n", dll_name_term); + todo_wine { + ok(dll_name_term < MAX_PATH, "unterminated path for test module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for test module with short buffer. Expected %d got %d\n", dll_name_len - 1, dll_name_term); + } + + FreeLibrary( mod ); +} + /* Verify linking style of import descriptors */ static void test_ImportDescriptors(void) { @@ -4859,6 +4955,8 @@ START_TEST(loader) } test_filenames(); + test_getmodulefilenamew_string_termination(); + test_getmodulefilenamea_string_termination(); test_ResolveDelayLoadedAPI(); test_ImportDescriptors(); test_section_access(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10291
From: Trent Waddington <trent.waddington@tensorworks.com.au> --- dlls/kernel32/tests/loader.c | 24 ++++++++---------------- dlls/kernelbase/loader.c | 10 ++++++++-- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 64e8c446a90..8547e79bf26 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -1572,10 +1572,8 @@ static void test_getmodulefilenamew_string_termination(void) ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for NULL module with short buffer: %d\n", (int)err); dll_name_term = wcsnlen(dll_name, MAX_PATH); ok(dll_name_term > 0, "can't get path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); - todo_wine { - ok(dll_name_term < MAX_PATH, "unterminated path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); - ok(dll_name_term == dll_name_len - 1, "incorrect path termination for NULL module with short buffer. Expected %d got %d. rv=%d\n", dll_name_len - 1, dll_name_term, (int)rv); - } + ok(dll_name_term < MAX_PATH, "unterminated path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for NULL module with short buffer. Expected %d got %d. rv=%d\n", dll_name_len - 1, dll_name_term, (int)rv); mod = LoadLibraryW( L"user32.dll" ); ok(mod != NULL, "can't load test dll\n"); @@ -1592,10 +1590,8 @@ static void test_getmodulefilenamew_string_termination(void) ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for test module with short buffer: %d\n", (int)err); dll_name_term = wcsnlen(dll_name, MAX_PATH); ok(dll_name_term > 0, "can't get path for test module with short buffer, dll_name_term=%d\n", dll_name_term); - todo_wine { - ok(dll_name_term < MAX_PATH, "unterminated path for test module with short buffer, dll_name_term=%d\n", dll_name_term); - ok(dll_name_term == dll_name_len - 1, "incorrect path termination for test module with short buffer. Expected %d got %d\n", dll_name_len - 1, dll_name_term); - } + ok(dll_name_term < MAX_PATH, "unterminated path for test module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for test module with short buffer. Expected %d got %d\n", dll_name_len - 1, dll_name_term); FreeLibrary( mod ); } @@ -1620,10 +1616,8 @@ static void test_getmodulefilenamea_string_termination(void) ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for NULL module with short buffer: %d\n", (int)err); dll_name_term = strnlen(dll_name, MAX_PATH); ok(dll_name_term > 0, "can't get path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); - todo_wine { - ok(dll_name_term < MAX_PATH, "unterminated path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); - ok(dll_name_term == dll_name_len - 1, "incorrect path termination for NULL module with short buffer. Expected %d got %d. rv=%d\n", dll_name_len - 1, dll_name_term, (int)rv); - } + ok(dll_name_term < MAX_PATH, "unterminated path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for NULL module with short buffer. Expected %d got %d. rv=%d\n", dll_name_len - 1, dll_name_term, (int)rv); mod = LoadLibraryA( "user32.dll" ); ok(mod != NULL, "can't load test dll\n"); @@ -1640,10 +1634,8 @@ static void test_getmodulefilenamea_string_termination(void) ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for test module with short buffer: %d\n", (int)err); dll_name_term = strnlen(dll_name, MAX_PATH); ok(dll_name_term > 0, "can't get path for test module with short buffer, dll_name_term=%d\n", dll_name_term); - todo_wine { - ok(dll_name_term < MAX_PATH, "unterminated path for test module with short buffer, dll_name_term=%d\n", dll_name_term); - ok(dll_name_term == dll_name_len - 1, "incorrect path termination for test module with short buffer. Expected %d got %d\n", dll_name_len - 1, dll_name_term); - } + ok(dll_name_term < MAX_PATH, "unterminated path for test module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for test module with short buffer. Expected %d got %d\n", dll_name_len - 1, dll_name_term); FreeLibrary( mod ); } diff --git a/dlls/kernelbase/loader.c b/dlls/kernelbase/loader.c index f4e1ca2e23a..e26a08ccca5 100644 --- a/dlls/kernelbase/loader.c +++ b/dlls/kernelbase/loader.c @@ -302,13 +302,12 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetModuleFileNameW( HMODULE module, LPWSTR filena ULONG len = 0; WIN16_SUBSYSTEM_TIB *win16_tib; UNICODE_STRING name; - NTSTATUS status; + NTSTATUS status = 0; if (!module && ((win16_tib = NtCurrentTeb()->Tib.SubSystemTib)) && win16_tib->exe_name) { len = min( size, win16_tib->exe_name->Length / sizeof(WCHAR) ); memcpy( filename, win16_tib->exe_name->Buffer, len * sizeof(WCHAR) ); - if (len < size) filename[len] = 0; goto done; } @@ -319,6 +318,13 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetModuleFileNameW( HMODULE module, LPWSTR filena SetLastError( RtlNtStatusToDosError( status )); done: TRACE( "%s\n", debugstr_wn(filename, len) ); + if (!status || status == STATUS_BUFFER_TOO_SMALL) + { + if (len < size) + filename[len] = 0; + else if (size > 0) + filename[size - 1] = 0; + } return len; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10291
The test-win10-21h2-32 and test-win10-21h2-64 phases are failing in a new and interesting way. Can't resolve gitlab.winehq.org? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10291#note_133815
eric pouech (@epo) commented about dlls/kernel32/tests/loader.c:
DeleteFileA( long_path ); }
+static void test_getmodulefilenamew_string_termination(void) +{ + WCHAR dll_name[MAX_PATH]; + int dll_name_len, dll_name_term; + DWORD rv, err; + HMODULE mod; + + GetModuleFileNameW(NULL, dll_name, MAX_PATH); + err = GetLastError(); + ok(err == ERROR_SUCCESS, "error getting path for NULL module: %d\n", (int)err);
you don't need casts here, just use %lu -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10291#note_133839
eric pouech (@epo) commented about dlls/kernel32/tests/loader.c:
+ dll_name_len = wcslen(dll_name); + ok(dll_name_len > 0, "can't get path for NULL module\n"); + ok(dll_name_len < MAX_PATH, "unterminated path for NULL module\n"); + + memset(dll_name, '*', sizeof(dll_name)); + rv = GetModuleFileNameW(NULL, dll_name, dll_name_len); + err = GetLastError(); + ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for NULL module with short buffer: %d\n", (int)err); + dll_name_term = wcsnlen(dll_name, MAX_PATH); + ok(dll_name_term > 0, "can't get path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + todo_wine { + ok(dll_name_term < MAX_PATH, "unterminated path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for NULL module with short buffer. Expected %d got %d. rv=%d\n", dll_name_len - 1, dll_name_term, (int)rv); + } + + mod = LoadLibraryW( L"user32.dll" ); after sleeping on it, I wonder what do the tests against user32.dll bring us that the one's on app module don't?
(we tend also to minimize tests so that CPU power and computation time doesn't increase too much; so new tests that show errors and ensure we don't break things in the future are good; tests that bring nothing shoudl be avoided) so unless you have a good argument here, I'd opt for removing them -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10291#note_133840
eric pouech (@epo) commented about dlls/kernel32/tests/loader.c:
DeleteFileA( long_path ); }
+static void test_getmodulefilenamew_string_termination(void) +{ + WCHAR dll_name[MAX_PATH]; + int dll_name_len, dll_name_term;
should be size_t (or at least unsigned) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10291#note_133841
eric pouech (@epo) commented about dlls/kernel32/tests/loader.c:
DeleteFileA( long_path ); }
+static void test_getmodulefilenamew_string_termination(void) +{ + WCHAR dll_name[MAX_PATH]; + int dll_name_len, dll_name_term; + DWORD rv, err; + HMODULE mod; + + GetModuleFileNameW(NULL, dll_name, MAX_PATH);
to be pendantic, we already have tests that cover "regular" GetModuleFileNameW behavior, so it doesn't bring much to the whole testsuite most of the tests for non failing access so I tend to simplify this as ``` DWORD dll_name_len; ... dll_name_len = GetModuleFileNameW(NULL, dll_name, MAX_PATH): ok(dll_name_len && dll_name_len < MAX_PATH, ....); ``` and move to the too short buffer tests -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10291#note_133842
eric pouech (@epo) commented about dlls/kernel32/tests/loader.c:
+ GetModuleFileNameW(NULL, dll_name, MAX_PATH); + err = GetLastError(); + ok(err == ERROR_SUCCESS, "error getting path for NULL module: %d\n", (int)err); + dll_name_len = wcslen(dll_name); + ok(dll_name_len > 0, "can't get path for NULL module\n"); + ok(dll_name_len < MAX_PATH, "unterminated path for NULL module\n"); + + memset(dll_name, '*', sizeof(dll_name)); + rv = GetModuleFileNameW(NULL, dll_name, dll_name_len); + err = GetLastError(); + ok(err == ERROR_INSUFFICIENT_BUFFER, "didn't get expected error getting path for NULL module with short buffer: %d\n", (int)err); + dll_name_term = wcsnlen(dll_name, MAX_PATH); + ok(dll_name_term > 0, "can't get path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + todo_wine { + ok(dll_name_term < MAX_PATH, "unterminated path for NULL module with short buffer, dll_name_term=%d\n", dll_name_term); + ok(dll_name_term == dll_name_len - 1, "incorrect path termination for NULL module with short buffer. Expected %d got %d. rv=%d\n", dll_name_len - 1, dll_name_term, (int)rv); please only keep that test and get rid of two previous ones, if the third test succeeds, the two firsts shall also (do they are useless)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10291#note_133843
eric pouech (@epo) commented about dlls/kernelbase/loader.c:
ULONG len = 0; WIN16_SUBSYSTEM_TIB *win16_tib; UNICODE_STRING name; - NTSTATUS status; + NTSTATUS status = 0;
if (!module && ((win16_tib = NtCurrentTeb()->Tib.SubSystemTib)) && win16_tib->exe_name) { len = min( size, win16_tib->exe_name->Length / sizeof(WCHAR) ); memcpy( filename, win16_tib->exe_name->Buffer, len * sizeof(WCHAR) ); - if (len < size) filename[len] = 0;
unfortunately, we don't have support for 16bit apps in the non regression test suite so it's not clear if this change is correct or not; this has to be manually tested with an old compiler and run in Windows so unless you're ready to go that way, I'd leave that code path unchanged -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10291#note_133844
eric pouech (@epo) commented about dlls/kernelbase/loader.c:
SetLastError( RtlNtStatusToDosError( status )); done: TRACE( "%s\n", debugstr_wn(filename, len) ); + if (!status || status == STATUS_BUFFER_TOO_SMALL)
didn't retest it, but the len \< size code path should already be handled in ntdll and i'd move this above the done: label (so you don't to init the status to O) and handle it in ``` if (status == STATUS_BUFFER_TOO_SMALL && size) filename[size - 1] = L'\0'; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10291#note_133845
participants (3)
-
eric pouech (＠epo) -
Trent Waddington
-
Trent Waddington (＠trent.waddington)