[PATCH 0/3] MR2101: ntdll: Load apiset targets from system directory

[Paul Gofman (＠gofman)]

Starting from Win10, when an apiset target dll (most notably in practice, ucrtbase and msvcrt) is loaded through import resolution or LoadLibrary, the dll from system32 directory is loaded regardless of presence of the same dll in a higher priority search path (e. g., at .exe directory). The other version of apiset target dll is still loaded if LoadLibrary() has a path (even if relative).

The practical issue I am trying to solve is the following scenario (encountered with a game using certain Uplay plugin) which has regressed since apisets implementation in Wine. The game imports ucrtbase.dll. Then, a plugin calls GetModuleHandle("api-ms-win-crt-runtime-l1-1-0.dll") and expects GetProcAddress("_crt_atexit") to succeed on the returned handle. The problem is that the game has ucrtbase.dll in its .exe directory. That one currently gets loaded. Then, find_dll_file() resolves api-ms-win-crt-runtime-l1-1-0.dll to the ucrtbase with a full path in system32 and tries to find that one (this part matches Windows behaviour as far as tests show). Since we have the other ucrtbase loaded this results in NULL module handle.

That issue is reproduced in my test in line 1661 (```ok( hapiset == hsystem || broken( old_behaviour && !hapiset )...```), current Wine behaviour corresponds to old Windows before Win10.

The third patch (which removes the last todo in the added test) is not something I hit with any real application so far, just stomped on that in tests and the fix looks trivial.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2101

[PATCH 1/3] kernel32/tests: Add tests for apiset target load specifics.

[Paul Gofman]

From: Paul Gofman <pgofman(a)codeweavers.com>

---
 dlls/kernel32/tests/module.c | 81 ++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c
index 956595bf2a1..50ad86bd600 100644
--- a/dlls/kernel32/tests/module.c
+++ b/dlls/kernel32/tests/module.c
@@ -1627,6 +1627,86 @@ static void test_ddag_node(void)
     ok( se == node->Dependencies.Tail, "Expected end of the list.\n" );
 }
 
+static void test_apiset_target_load(void)
+{
+    static const char apiset_dll[] = "api-ms-win-shcore-obsolete-l1-1-0.dll";
+    HMODULE hlocal, hsystem, hapiset, h;
+    char system_path[MAX_PATH];
+    BOOL old_behaviour;
+
+    if (GetModuleHandleA( "shcore.dll" ))
+    {
+        skip( "shcore.dll is already loaded, skipping test.\n" );
+        return;
+    }
+
+    GetSystemDirectoryA( system_path, MAX_PATH );
+    strcat( system_path, "\\shcore.dll" );
+
+    create_test_dll( ".\\shcore.dll" );
+
+    hapiset = GetModuleHandleA( apiset_dll );
+    ok( !hapiset, "Got %p.\n", hapiset );
+
+    hsystem = LoadLibraryA( "shcore.dll" );
+    ok( !!hsystem, "Got NULL.\n" );
+
+    hlocal = LoadLibraryA( ".\\shcore.dll" );
+    ok( !!hlocal, "Got NULL.\n" );
+
+    old_behaviour = (hsystem == hlocal);
+    todo_wine ok( hsystem != hlocal || broken( old_behaviour ), "Got same module.\n" );
+
+    hapiset = GetModuleHandleA( apiset_dll );
+    todo_wine ok( hapiset == hsystem || broken( old_behaviour && !hapiset ), "Got %p, %p.\n", hapiset, hsystem );
+
+    h = GetModuleHandleA( "shcore.dll" );
+    ok( h == hsystem, "Got %p, %p.\n", h, hapiset );
+
+    FreeLibrary( hsystem );
+    FreeLibrary( hlocal );
+
+    hapiset = GetModuleHandleA( apiset_dll );
+    ok( !hapiset, "Got %p.\n", hapiset );
+
+    hlocal = LoadLibraryA( ".\\shcore.dll" );
+    ok( !!hlocal, "Got NULL.\n" );
+
+    hapiset = GetModuleHandleA( apiset_dll );
+    ok( !hapiset, "Got %p.\n", hapiset );
+
+    hsystem = LoadLibraryA( "shcore.dll" );
+    ok( !!hsystem, "Got NULL.\n" );
+    ok( hsystem == hlocal, "Got %p, %p.\n", hsystem, hlocal );
+
+    h = GetModuleHandleA( "shcore.dll" );
+    ok( h == hlocal, "Got %p, %p.\n", h, hapiset );
+
+    FreeLibrary( hsystem );
+    FreeLibrary( hlocal );
+
+    hlocal = LoadLibraryA( ".\\shcore.dll" );
+    ok( !!hlocal, "Got NULL.\n" );
+
+    hapiset = GetModuleHandleA( apiset_dll );
+    ok( !hapiset, "Got %p.\n", hapiset );
+
+    hapiset = LoadLibraryA( apiset_dll );
+    /* hapiset is NULL before Win8 and a newly loaded module from system dir after. */
+    ok( hapiset != hlocal, "Got same module.\n" );
+
+    h = GetModuleHandleA( "shcore.dll" );
+    todo_wine ok( h == hlocal, "Got %p, %p, %p.\n", h, hlocal, hapiset );
+
+    h = GetModuleHandleA( system_path );
+    ok( h == hapiset, "Got %p, %p.\n", h, hapiset );
+
+    FreeLibrary( hlocal );
+    FreeLibrary( hapiset );
+
+    DeleteFileA( ".\\shcore.dll" );
+}
+
 START_TEST(module)
 {
     WCHAR filenameW[MAX_PATH];
@@ -1663,4 +1743,5 @@ START_TEST(module)
     test_LdrGetDllFullName();
     test_apisets();
     test_ddag_node();
+    test_apiset_target_load();
 }
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/2101

[PATCH 2/3] ntdll: Load apiset targets without explicit path from system dir.

[Paul Gofman]

From: Paul Gofman <pgofman(a)codeweavers.com>

---
 dlls/kernel32/tests/module.c |  4 ++--
 dlls/ntdll/loader.c          | 38 +++++++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c
index 50ad86bd600..c05af6a235a 100644
--- a/dlls/kernel32/tests/module.c
+++ b/dlls/kernel32/tests/module.c
@@ -1655,10 +1655,10 @@ static void test_apiset_target_load(void)
     ok( !!hlocal, "Got NULL.\n" );
 
     old_behaviour = (hsystem == hlocal);
-    todo_wine ok( hsystem != hlocal || broken( old_behaviour ), "Got same module.\n" );
+    ok( hsystem != hlocal || broken( old_behaviour ), "Got same module.\n" );
 
     hapiset = GetModuleHandleA( apiset_dll );
-    todo_wine ok( hapiset == hsystem || broken( old_behaviour && !hapiset ), "Got %p, %p.\n", hapiset, hsystem );
+    ok( hapiset == hsystem || broken( old_behaviour && !hapiset ), "Got %p, %p.\n", hapiset, hsystem );
 
     h = GetModuleHandleA( "shcore.dll" );
     ok( h == hsystem, "Got %p, %p.\n", h, hapiset );
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
index 01a30742678..e68c6ac6c05 100644
--- a/dlls/ntdll/loader.c
+++ b/dlls/ntdll/loader.c
@@ -683,6 +683,27 @@ static NTSTATUS get_apiset_target( const API_SET_NAMESPACE *map, const API_SET_N
 }
 
 
+/******************************************************************************
+ *	is_apiset_target
+ */
+static BOOL is_apiset_target( const WCHAR *name )
+{
+    const API_SET_NAMESPACE *map = NtCurrentTeb()->Peb->ApiSetMap;
+    const API_SET_NAMESPACE_ENTRY *entry;
+    UNICODE_STRING target;
+    ULONG i, len;
+
+    for (i = 0; i < map->Count; ++i)
+    {
+        entry = (API_SET_NAMESPACE_ENTRY *)((char *)map + map->EntryOffset) + i;
+        if (get_apiset_target( map, entry, NULL, &target )) continue;
+        len = target.Length / sizeof(WCHAR);
+        if (!wcsnicmp( name, target.Buffer, len ) && !name[len]) return TRUE;
+    }
+    return FALSE;
+}
+
+
 /**********************************************************************
  *	    build_import_name
  */
@@ -2997,17 +3018,18 @@ done:
  */
 static NTSTATUS search_dll_file( LPCWSTR paths, LPCWSTR search, UNICODE_STRING *nt_name,
                                  WINE_MODREF **pwm, HANDLE *mapping, SECTION_IMAGE_INFORMATION *image_info,
-                                 struct file_id *id )
+                                 struct file_id *id, BOOL system_dir_only )
 {
     WCHAR *name;
     BOOL found_image = FALSE;
     NTSTATUS status = STATUS_DLL_NOT_FOUND;
-    ULONG len;
+    ULONG len, system_dir_len;
 
     if (!paths) paths = default_load_path;
     len = wcslen( paths );
 
-    if (len < wcslen( system_dir )) len = wcslen( system_dir );
+    system_dir_len = wcslen( system_dir );
+    if (len < system_dir_len) len = system_dir_len;
     len += wcslen( search ) + 2;
 
     if (!(name = RtlAllocateHeap( GetProcessHeap(), 0, len * sizeof(WCHAR) )))
@@ -3022,6 +3044,11 @@ static NTSTATUS search_dll_file( LPCWSTR paths, LPCWSTR search, UNICODE_STRING *
         if (*ptr == ';') ptr++;
         memcpy( name, paths, len * sizeof(WCHAR) );
         if (len && name[len - 1] != '\\') name[len++] = '\\';
+        if (system_dir_only && (len != system_dir_len || wcsnicmp(name, system_dir, len)))
+        {
+            paths = ptr;
+            continue;
+        }
         wcscpy( name + len, search );
 
         nt_name->Buffer = NULL;
@@ -3086,7 +3113,8 @@ static NTSTATUS find_dll_file( const WCHAR *load_path, const WCHAR *libname, UNI
 
     if (RtlDetermineDosPathNameType_U( libname ) == RELATIVE_PATH)
     {
-        status = search_dll_file( load_path, libname, nt_name, pwm, mapping, image_info, id );
+        status = search_dll_file( load_path, libname, nt_name, pwm, mapping, image_info, id,
+                                  is_apiset_target( libname ) );
         if (status == STATUS_DLL_NOT_FOUND)
             status = find_builtin_without_file( libname, nt_name, pwm, mapping, image_info, id );
     }
@@ -3120,7 +3148,7 @@ static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, DWORD fl
     TRACE( "looking for %s in %s\n", debugstr_w(libname), debugstr_w(load_path) );
 
     if (system && system_dll_path.Buffer)
-        nts = search_dll_file( system_dll_path.Buffer, libname, &nt_name, pwm, &mapping, &image_info, &id );
+        nts = search_dll_file( system_dll_path.Buffer, libname, &nt_name, pwm, &mapping, &image_info, &id, FALSE );
 
     if (nts)
     {
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/2101

[PATCH 3/3] ntdll: Clear cached_modref for app requested DLL search.

[Paul Gofman]

From: Paul Gofman <pgofman(a)codeweavers.com>

---
 dlls/kernel32/tests/module.c | 2 +-
 dlls/ntdll/loader.c          | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c
index c05af6a235a..586fb9f18bb 100644
--- a/dlls/kernel32/tests/module.c
+++ b/dlls/kernel32/tests/module.c
@@ -1696,7 +1696,7 @@ static void test_apiset_target_load(void)
     ok( hapiset != hlocal, "Got same module.\n" );
 
     h = GetModuleHandleA( "shcore.dll" );
-    todo_wine ok( h == hlocal, "Got %p, %p, %p.\n", h, hlocal, hapiset );
+    ok( h == hlocal, "Got %p, %p, %p.\n", h, hlocal, hapiset );
 
     h = GetModuleHandleA( system_path );
     ok( h == hapiset, "Got %p, %p.\n", h, hapiset );
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
index e68c6ac6c05..c2f85d434ca 100644
--- a/dlls/ntdll/loader.c
+++ b/dlls/ntdll/loader.c
@@ -3241,6 +3241,8 @@ NTSTATUS WINAPI DECLSPEC_HOTPATCH LdrLoadDll(LPCWSTR path_name, DWORD flags,
 
     RtlEnterCriticalSection( &loader_section );
 
+    cached_modref = NULL;
+
     nts = load_dll( path_name, dllname ? dllname : libname->Buffer, flags, &wm, FALSE );
 
     if (nts == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS))
@@ -3320,6 +3322,8 @@ NTSTATUS WINAPI LdrGetDllHandleEx( ULONG flags, LPCWSTR load_path, ULONG *dll_ch
 
     RtlEnterCriticalSection( &loader_section );
 
+    cached_modref = NULL;
+
     status = find_dll_file( load_path, dllname ? dllname : name->Buffer,
                             &nt_name, &wm, &mapping, &image_info, &id );
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/2101

[Paul Gofman (＠gofman)]

I found that the patches are off in a couple of ways, I will update this soon.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2101#note_22963

[PATCH 0/3] MR2101: ntdll: Load apiset targets from system directory - closed

[Paul Gofman (＠gofman)]

This merge request was closed by Paul Gofman.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2101