[PATCH v3 0/1] MR1166: ntdll: don't claim to have not found a library that is in place

[Konstantin Kharlamov (＠Hi-Angel)]

When `import_dll` function gets `STATUS_DLL_NOT_FOUND` from `load_dll()`, it has no way of distinguishing whether it's a dependency for the dll wasn't found, or the dll itself is missing.

That results in confusing ERR messages, where a dll is found, but wine
claims it isn't. Fix this by moving the print to the actual place that
is aware of whether it's the parent or the child dll haven't been found.

Log before:

    0024:err:module:import_dll Library lib2.dll (which is needed by L"Z:\\tmp\\lib1.dll") not found
    0024:err:module:import_dll Library lib1.dll (which is needed by L"Z:\\tmp\\a.exe") not found

Log after:

    0024:err:module:load_dll Library L"lib2.dll" (which is needed by L"Z:\\tmp\\foo\\lib1.dll") not found
    0024:err:module:import_dll Loading library lib2.dll (which is needed by L"Z:\\tmp\\foo\\lib1.dll") failed (error c0000135).
    0024:err:module:import_dll Loading library lib1.dll (which is needed by L"Z:\\tmp\\foo\\a.exe") failed (error c0000135).

--------

While it's just about prints, but it costed me hours of wandering *(because if I know where to find the dependency dll, but I see that the main dll is also "not found" despite being "right there", I have to research the latter problem that looks more complicated. But given enough resources applied, it then turns out to be a faux)*.

As a fun fact, I am apparently not the only one to have stumbled upon. Here's a screenshot of a `#english` chat where I asked about a commit title correctness. It's the last place where you'd expect to find someone familiar with WINE, and yet here's a person that had to deal with that exact error as well:

![Screenshot_20221026_023411](/uploads/55552d7c335ef5ce0b507f61adacbb00/Screenshot_20221026_023411.png)

So, I think it's kinda serious problem.

--
  v3: ntdll: don't claim to have not found a library that is in place
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1166

[PATCH v3 1/1] ntdll: don't claim to have not found a library that is in place

[Konstantin Kharlamov]

From: Konstantin Kharlamov <Hi-Angel(a)yandex.ru>

When `import_dll` function gets `STATUS_DLL_NOT_FOUND` from
`load_dll()`, it has no way of distinguishing whether it's a dependency
for the dll wasn't found, or the dll itself is missing.

That results in confusing ERR messages, where a dll is found, but wine
claims it isn't. Fix this by adding another argument to load_dll() that
indicates whether the error is in child or parent.

Log before:

    0024:err:module:import_dll Library lib2.dll (which is needed by L"Z:\\tmp\\lib1.dll") not found
    0024:err:module:import_dll Library lib1.dll (which is needed by L"Z:\\tmp\\a.exe") not found

Log after:

    0024:err:module:load_dll Library L"lib2.dll" (which is needed by L"Z:\\tmp\\foo\\lib1.dll") not found
    0024:err:module:import_dll Loading library lib2.dll (which is needed by L"Z:\\tmp\\foo\\lib1.dll") failed (error c0000135).
    0024:err:module:import_dll Loading library lib1.dll (which is needed by L"Z:\\tmp\\foo\\a.exe") failed (error c0000135).

Signed-off-by: Konstantin Kharlamov <Hi-Angel(a)yandex.ru>
---
 dlls/ntdll/loader.c | 46 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
index 61aeb25898b..c4cc68adbd0 100644
--- a/dlls/ntdll/loader.c
+++ b/dlls/ntdll/loader.c
@@ -185,7 +185,7 @@ static WINE_MODREF *last_failed_modref;
 
 static LDR_DDAG_NODE *node_ntdll, *node_kernel32;
 
-static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, DWORD flags, WINE_MODREF** pwm, BOOL system );
+static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, DWORD flags, WINE_MODREF** pwm, BOOL system, BOOL *is_child_import_failed );
 static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved );
 static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports,
                                     DWORD exp_size, DWORD ordinal, LPCWSTR load_path );
@@ -871,7 +871,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS
     {
         WINE_MODREF *imp = get_modref( module );
         TRACE( "delay loading %s for '%s'\n", debugstr_w(mod_name), forward );
-        if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS &&
+        if (load_dll( load_path, mod_name, 0, &wm, imp->system, NULL ) == STATUS_SUCCESS &&
             !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS))
         {
             if (!imports_fixup_done && current_modref)
@@ -938,7 +938,7 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY
     proc = get_rva( module, functions[ordinal] );
 
     /* if the address falls into the export dir, it's a forward */
-    if (((const char *)proc >= (const char *)exports) && 
+    if (((const char *)proc >= (const char *)exports) &&
         ((const char *)proc < (const char *)exports + exp_size))
         return find_forwarded_export( module, (const char *)proc, load_path );
 
@@ -1050,6 +1050,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP
     PVOID protect_base;
     SIZE_T protect_size = 0;
     DWORD protect_old;
+    BOOL is_child_import_failed;
 
     thunk_list = get_rva( module, (DWORD)descr->FirstThunk );
     if (descr->u.OriginalFirstThunk)
@@ -1065,11 +1066,15 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP
     }
 
     status = build_import_name( buffer, name, len );
-    if (!status) status = load_dll( load_path, buffer, 0, &wmImp, system );
+    if (!status) status = load_dll( load_path, buffer, 0, &wmImp, system, &is_child_import_failed );
 
     if (status)
     {
-        if (status == STATUS_DLL_NOT_FOUND)
+        /* If import fail was printed for child DLL, then printing that again for
+         * for parent isn't very useful. Instead just print that it failed to load.
+         * The reason (that child failed to laod) will be in logs anyway.
+         */
+        if (status == STATUS_DLL_NOT_FOUND && !is_child_import_failed)
             ERR("Library %s (which is needed by %s) not found\n",
                 name, debugstr_w(current_modref->ldr.FullDllName.Buffer));
         else
@@ -1335,7 +1340,7 @@ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void *
     prev = current_modref;
     current_modref = wm;
     assert( !wm->ldr.DdagNode->Dependencies.Tail );
-    if (!(status = load_dll( load_path, L"mscoree.dll", 0, &imp, FALSE ))
+    if (!(status = load_dll( load_path, L"mscoree.dll", 0, &imp, FALSE, NULL ))
           && !add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, NULL ))
         status = STATUS_NO_MEMORY;
     current_modref = prev;
@@ -1734,7 +1739,7 @@ static void process_detach(void)
 
             /* Call detach notification */
             mod->Flags &= ~LDR_PROCESS_ATTACHED;
-            MODULE_InitDLL( CONTAINING_RECORD(mod, WINE_MODREF, ldr), 
+            MODULE_InitDLL( CONTAINING_RECORD(mod, WINE_MODREF, ldr),
                             DLL_PROCESS_DETACH, ULongToPtr(process_detaching) );
             call_ldr_notifications( LDR_DLL_NOTIFICATION_REASON_UNLOADED, mod );
 
@@ -3102,7 +3107,12 @@ done:
  * Load a PE style module according to the load order.
  * The loader_section must be locked while calling this function.
  */
-static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, DWORD flags, WINE_MODREF** pwm, BOOL system )
+static NTSTATUS load_dll( const WCHAR *load_path,
+                          const WCHAR *libname,
+                          DWORD flags,
+                          WINE_MODREF** pwm,
+                          BOOL system,
+                          BOOL *is_child_import_failed )
 {
     UNICODE_STRING nt_name;
     struct file_id id;
@@ -3113,6 +3123,9 @@ 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 (is_child_import_failed)
+        *is_child_import_failed = FALSE;
+
     if (system && system_dll_path.Buffer)
         nts = search_dll_file( system_dll_path.Buffer, libname, &nt_name, pwm, &mapping, &image_info, &id );
 
@@ -3149,11 +3162,18 @@ static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, DWORD fl
     switch (nts)
     {
     case STATUS_INVALID_IMAGE_NOT_MZ:  /* not in PE format, maybe it's a .so file */
-        if (ntdll_unix_handle) nts = load_so_dll( load_path, &nt_name, flags, pwm );
+        if (ntdll_unix_handle)
+        {
+            nts = load_so_dll( load_path, &nt_name, flags, pwm );
+            if (nts == STATUS_DLL_NOT_FOUND && is_child_import_failed)
+                *is_child_import_failed = TRUE;
+        }
         break;
 
     case STATUS_SUCCESS:  /* valid PE file */
         nts = load_native_dll( load_path, &nt_name, mapping, &image_info, &id, flags, system, pwm );
+        if (nts == STATUS_DLL_NOT_FOUND && is_child_import_failed)
+            *is_child_import_failed = TRUE;
         break;
     }
 
@@ -3207,7 +3227,7 @@ NTSTATUS WINAPI DECLSPEC_HOTPATCH LdrLoadDll(LPCWSTR path_name, DWORD flags,
 
     RtlEnterCriticalSection( &loader_section );
 
-    nts = load_dll( path_name, dllname ? dllname : libname->Buffer, flags, &wm, FALSE );
+    nts = load_dll( path_name, dllname ? dllname : libname->Buffer, flags, &wm, FALSE, NULL );
 
     if (nts == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS))
     {
@@ -3708,7 +3728,7 @@ void WINAPI LdrShutdownThread(void)
         if ( mod->Flags & LDR_NO_DLL_CALLS )
             continue;
 
-        MODULE_InitDLL( CONTAINING_RECORD(mod, WINE_MODREF, ldr), 
+        MODULE_InitDLL( CONTAINING_RECORD(mod, WINE_MODREF, ldr),
                         DLL_THREAD_DETACH, NULL );
     }
 
@@ -3985,7 +4005,7 @@ static void init_wow64( CONTEXT *context )
         NTSTATUS status;
         static const WCHAR wow64_path[] = L"C:\\windows\\system32\\wow64.dll";
 
-        if ((status = load_dll( NULL, wow64_path, 0, &wm, FALSE )))
+        if ((status = load_dll( NULL, wow64_path, 0, &wm, FALSE, NULL )))
         {
             ERR( "could not load %s, status %lx\n", debugstr_w(wow64_path), status );
             NtTerminateProcess( GetCurrentProcess(), status );
@@ -4142,7 +4162,7 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR
 
         if (NtCurrentTeb()->WowTebOffset) init_wow64( context );
 
-        if ((status = load_dll( NULL, L"kernel32.dll", 0, &kernel32, FALSE )) != STATUS_SUCCESS)
+        if ((status = load_dll( NULL, L"kernel32.dll", 0, &kernel32, FALSE, NULL )) != STATUS_SUCCESS)
         {
             MESSAGE( "wine: could not load kernel32.dll, status %lx\n", status );
             NtTerminateProcess( GetCurrentProcess(), status );
-- 
GitLab

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

[Konstantin Kharlamov (＠Hi-Angel)]

So, since nobody has any particular preferences regarding approaches to solving that, I implemented the one we discussed above that is about adding another parameter to `load_dll`.

Please review.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1166#note_19250

[Konstantin Kharlamov (＠Hi-Angel)]

Ping. Would be nice to merge this before the stable with the problem would be released.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1166#note_20072