[PATCH v2 0/4] MR4587: kernelbase: Implement flag validation in `LoadLibraryEx()` to work like in Win10
While debugging League of Legends I noticed that `LoadLibraryEx()` is invoked with binary filename (non-text) and non-NULL `hFile`. In Windows such call would return `NULL` with LastError set as `ERROR_INVALID_PARAMETER` but current Wine implementation would try to proceed ahead. I don't know if this is intentional trick to mess up debugging/reversing tools or if we have some memory corruption before that overwrites filename buffer. But then I can't imagine how `hFile` could have gotten corrupted since shouldn't that be simple static value in register... Having this MR causes very noticeable difference in LoL: * without it - process deadlocks * with it - no deadlock Also while looking at this, I tried a lot of different flag combinations on Windows 10 and here I implemented so it works exactly like that. I also implemented test case for non-NULL `hFile` but I didn't bother for other flags since I don't know if we even care about those. -- v2: kernelbase: LoadLibraryEx validate flags like Win10 kernelbase: LoadLibraryEx support LOAD_PACKAGED_LIBRARY kernelbase: LoadLibraryEx enforce valid flag combinations kernelbase: Add test case for LoadLibraryEx() when hFile is non-NULL https://gitlab.winehq.org/wine/wine/-/merge_requests/4587
From: Dāvis Mosāns <davispuh(a)gmail.com> When hFile is non-NULL then LoadLibraryEx() should return NULL with LastError set as ERROR_INVALID_PARAMETER --- dlls/kernelbase/tests/file.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/dlls/kernelbase/tests/file.c b/dlls/kernelbase/tests/file.c index 18dff0a32ab..b19360c8998 100644 --- a/dlls/kernelbase/tests/file.c +++ b/dlls/kernelbase/tests/file.c @@ -46,6 +46,23 @@ static void test_ioring_caps(void) todo_wine ok(hr == S_OK, "got %#lx.\n", hr); } +static void test_load_library_ex_flags(void) +{ + HMODULE hmod; + DWORD last_error; + + SetLastError(0x33); + + /* Test if LoadLibraryEx can load a dll */ + hmod = LoadLibraryExW(L"kernelbase.dll", NULL /* hFile */, LOAD_LIBRARY_SEARCH_SYSTEM32 /* dwFlags */); + ok(hmod != NULL, "LoadLibraryEx returned %p with LastError: %#lx.\n", hmod, GetLastError()); + + /* When hFile is non-NULL LoadLibraryEx should return NULL and set LastError to ERROR_INVALID_PARAMETER */ + hmod = LoadLibraryExW(L"kernelbase.dll", (HANDLE)0x000003A0 /* random hFile */, 0 /* dwFlags */); + last_error = GetLastError(); + ok(hmod == NULL && last_error == ERROR_INVALID_PARAMETER, "LoadLibraryEx returned %p with LastError: %#lx.\n", hmod, last_error); +} + START_TEST(file) { HMODULE hmod; @@ -54,4 +71,5 @@ START_TEST(file) pQueryIoRingCapabilities = (void *)GetProcAddress(hmod, "QueryIoRingCapabilities"); test_ioring_caps(); + test_load_library_ex_flags(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4587
From: Dāvis Mosāns <davispuh(a)gmail.com> --- dlls/kernelbase/loader.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dlls/kernelbase/loader.c b/dlls/kernelbase/loader.c index 59b91596f13..8f40286b880 100644 --- a/dlls/kernelbase/loader.c +++ b/dlls/kernelbase/loader.c @@ -530,12 +530,19 @@ HMODULE WINAPI DECLSPEC_HOTPATCH LoadLibraryExW( LPCWSTR name, HANDLE file, DWOR { UNICODE_STRING str; HMODULE module; + BOOL invalid_parameter; - if (!name) + invalid_parameter = !name || + file || + ((flags & LOAD_LIBRARY_AS_DATAFILE) && (flags & LOAD_LIBRARY_AS_DATAFILE_EXCLUSIVE)) || + ((flags & LOAD_LIBRARY_SEARCH_DEFAULT_DIRS) && (flags & LOAD_WITH_ALTERED_SEARCH_PATH)); + + if (invalid_parameter) { SetLastError( ERROR_INVALID_PARAMETER ); return 0; } + RtlInitUnicodeString( &str, name ); if (str.Buffer[str.Length/sizeof(WCHAR) - 1] != ' ') return load_library( &str, flags ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4587
From: Dāvis Mosāns <davispuh(a)gmail.com> --- dlls/kernelbase/loader.c | 7 +++++++ include/winbase.h | 1 + 2 files changed, 8 insertions(+) diff --git a/dlls/kernelbase/loader.c b/dlls/kernelbase/loader.c index 8f40286b880..af61428487b 100644 --- a/dlls/kernelbase/loader.c +++ b/dlls/kernelbase/loader.c @@ -543,6 +543,13 @@ HMODULE WINAPI DECLSPEC_HOTPATCH LoadLibraryExW( LPCWSTR name, HANDLE file, DWOR return 0; } + if (flags & LOAD_PACKAGED_LIBRARY) + { + /* FIXME: Don't know if it's implemented like this on Windows + but this does accomplish same result in error case */ + return LoadPackagedLibrary(name, flags); + } + RtlInitUnicodeString( &str, name ); if (str.Buffer[str.Length/sizeof(WCHAR) - 1] != ' ') return load_library( &str, flags ); diff --git a/include/winbase.h b/include/winbase.h index 16e449f7c2d..93b592c3791 100644 --- a/include/winbase.h +++ b/include/winbase.h @@ -995,6 +995,7 @@ DECL_WINELIB_TYPE_AW(ENUMRESLANGPROC) /* flags that can be passed to LoadLibraryEx */ #define DONT_RESOLVE_DLL_REFERENCES 0x00000001 #define LOAD_LIBRARY_AS_DATAFILE 0x00000002 +#define LOAD_PACKAGED_LIBRARY 0x00000004 /* Undocumented Windows internal */ #define LOAD_WITH_ALTERED_SEARCH_PATH 0x00000008 #define LOAD_IGNORE_CODE_AUTHZ_LEVEL 0x00000010 #define LOAD_LIBRARY_AS_IMAGE_RESOURCE 0x00000020 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4587
From: Dāvis Mosāns <davispuh(a)gmail.com> --- dlls/kernelbase/loader.c | 12 ++++++++++++ include/winbase.h | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/dlls/kernelbase/loader.c b/dlls/kernelbase/loader.c index af61428487b..316b1d180cb 100644 --- a/dlls/kernelbase/loader.c +++ b/dlls/kernelbase/loader.c @@ -537,6 +537,18 @@ HMODULE WINAPI DECLSPEC_HOTPATCH LoadLibraryExW( LPCWSTR name, HANDLE file, DWOR ((flags & LOAD_LIBRARY_AS_DATAFILE) && (flags & LOAD_LIBRARY_AS_DATAFILE_EXCLUSIVE)) || ((flags & LOAD_LIBRARY_SEARCH_DEFAULT_DIRS) && (flags & LOAD_WITH_ALTERED_SEARCH_PATH)); + /* Windows 10 22H2 have such validation */ + invalid_parameter = invalid_parameter || + ((flags & LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR) && + (flags & (LOAD_LIBRARY_AS_DATAFILE | LOAD_LIBRARY_AS_DATAFILE_EXCLUSIVE | LOAD_LIBRARY_AS_IMAGE_RESOURCE))); + + if (flags & LOAD_LIBRARY_UNSUPPORTED_MASK) + { + WARN( "Pretending to be Windows 10 22H2, unsupported flags: 0x%lx\n, returning ERROR_INVALID_PARAMETER", + flags & LOAD_LIBRARY_UNSUPPORTED_MASK ); + invalid_parameter = TRUE; + } + if (invalid_parameter) { SetLastError( ERROR_INVALID_PARAMETER ); diff --git a/include/winbase.h b/include/winbase.h index 93b592c3791..bcfd0fc8d1f 100644 --- a/include/winbase.h +++ b/include/winbase.h @@ -1006,6 +1006,10 @@ DECL_WINELIB_TYPE_AW(ENUMRESLANGPROC) #define LOAD_LIBRARY_SEARCH_USER_DIRS 0x00000400 #define LOAD_LIBRARY_SEARCH_SYSTEM32 0x00000800 #define LOAD_LIBRARY_SEARCH_DEFAULT_DIRS 0x00001000 +#define LOAD_LIBRARY_SAFE_CURRENT_DIRS 0x00002000 +#define LOAD_LIBRARY_SEARCH_SYSTEM32_NO_FORWARDER 0x00004000 +#define LOAD_LIBRARY_OS_INTEGRITY_CONTINUITY 0x00008000 +#define LOAD_LIBRARY_UNSUPPORTED_MASK 0xFFFF0000 /* If you add new flags update this mask */ #define GET_MODULE_HANDLE_EX_FLAG_PIN 1 #define GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT 2 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4587
On Mon Dec 4 17:56:11 2023 +0000, Etaash Mathamsetty wrote:
> btw you should change the comments from // to /* */, since that's the
> wine convention. Also make the curly braces go on the next line after
> the if. There also seems to be a lot of repetition, maybe you could do
> something to get rid of some of it?
> Some of the comments also don't really seem necessary, so it would be
> nice if those got removed.
Updated it:
* Replaced `//` to `/* */`
* `{` to newline
* reduced duplication
I removed some comments but could you point out which ones you think are unnecessary?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4587#note_54901
Etaash Mathamsetty (@etaash.mathamsetty) commented about dlls/kernelbase/loader.c:
+ BOOL invalid_parameter;
- if (!name) + invalid_parameter = !name || + file || + ((flags & LOAD_LIBRARY_AS_DATAFILE) && (flags & LOAD_LIBRARY_AS_DATAFILE_EXCLUSIVE)) || + ((flags & LOAD_LIBRARY_SEARCH_DEFAULT_DIRS) && (flags & LOAD_WITH_ALTERED_SEARCH_PATH)); + + /* Windows 10 22H2 have such validation */ + invalid_parameter = invalid_parameter || + ((flags & LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR) && + (flags & (LOAD_LIBRARY_AS_DATAFILE | LOAD_LIBRARY_AS_DATAFILE_EXCLUSIVE | LOAD_LIBRARY_AS_IMAGE_RESOURCE))); + + if (flags & LOAD_LIBRARY_UNSUPPORTED_MASK) + { + WARN( "Pretending to be Windows 10 22H2, unsupported flags: 0x%lx\n, returning ERROR_INVALID_PARAMETER", I think here it might be better to print something along the lines of "unsupported flags: LOAD_LIBRARY_UNSUPPORTED_MASK" since that's the only flag that would reach this statement
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4587#note_54919
On Mon Dec 4 17:56:11 2023 +0000, Dāvis Mosāns (davispuh) wrote:
Updated it: * Replaced `//` to `/* */` * `{` to newline * reduced duplication I removed some comments but could you point out which ones you think are unnecessary? yeah the comments look good now
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4587#note_54920
On Mon Dec 4 20:12:57 2023 +0000, Etaash Mathamsetty wrote:
I think here it might be better to print something along the lines of "unsupported flags: LOAD_LIBRARY_UNSUPPORTED_MASK" since that's the only flag that would reach this statement It's bit tricky, because new Windows versions can add new flags so that will cause Wine to go here, right now this is just max supported flags - same as in Windows 10 right now but that might change.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4587#note_54925
On Mon Dec 4 21:02:00 2023 +0000, Dāvis Mosāns (davispuh) wrote:
It's bit tricky, because new Windows versions can add new flags so that will cause Wine to go here, right now this is just max supported flags - same as in Windows 10 right now but that might change. oh wait I thought LOAD_LIBRARY_UNSUPPORTED_MASK was just a regular flag for some reason, disregard.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4587#note_54934
I also implemented test case for non-NULL hFile but I didn't bother for other flags since I don't know if we even care about those.
We don't care unless there's an app that depends on this. In particular, we don't want to reject potentially invalid flags combination unless there's a clear need. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4587#note_55110
On Tue Dec 5 21:50:41 2023 +0000, Alexandre Julliard wrote:
I also implemented test case for non-NULL hFile but I didn't bother for other flags since I don't know if we even care about those. We don't care unless there's an app that depends on this. In particular, we don't want to reject potentially invalid flags combination unless there's a clear need. There can be benefits, application that works on Windows either won't pass such flag combination at all (so doesn't matter what Wine does) or it works in Windows only because Windows does return `ERROR_INVALID_PARAMETER`.
And Wine not returning `ERROR_INVALID_PARAMETER` can cause application to fail much later which could make it more difficult to understand where the real issue is. Anyway I don't have any example where this would matter but my thinking is it would be better to stick how it works in Windows. Only `non-NULL hFile` handling makes behavior difference in LoL so I could make separate MR just for that change. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4587#note_55117
participants (3)
-
Alexandre Julliard (＠julliard) -
Dāvis Mosāns
-
Etaash Mathamsetty (＠etaash.mathamsetty)