[PATCH 4/4] ntdll: Print a warning when LdrGetProcedureAddress fails

[Zebediah Figura]

On 6/8/20 5:59 PM, Alex Henrie wrote:
> On Sun, Jun 7, 2020 at 11:17 PM Zebediah Figura <z.figura12 at gmail.com> wrote:
>>
>> On 6/7/20 11:41 PM, Alex Henrie wrote:
>>> Signed-off-by: Alex Henrie <alexhenrie24 at gmail.com>
>>> ---
>>> This will help tremendously with debugging null pointer segfaults.
>>
>> Seconded wholeheartedly. I've kept a similar patch in my local tree for
>> quite a while.
>>
>> Even worse, applications will detect that a function is missing but
>> silently fail...
> 
> Glad you approve!
> 
>>> diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
>>> index d1b71efc07..2c37135105 100644
>>> --- a/dlls/ntdll/loader.c
>>> +++ b/dlls/ntdll/loader.c
>>> @@ -1865,6 +1865,17 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name,
>>>       }
>>>
>>>       RtlLeaveCriticalSection( &loader_section );
>>> +
>>> +    if (__WINE_GET_DEBUGGING_WARN( __wine_dbch___default ) && module && (name || ord) && !*address)
>>
>> Why not just WARN_ON(module)?
> 
> I didn't know that you could do that. Thanks for the tip!
> 
>> Couldn't we just check for STATUS_PROCEDURE_NOT_FOUND? (I also wouldn't
>> think it's worth checking that the parameters are valid.)
> 
> Yes, I realized after I sent v1 that the function should be checking
> ret instead of address. I put in some argument checks because I didn't
> want to see noise from this function when the failure is not a bug in
> Wine. Do you see any strong reason to drop the argument checks?

Mostly because I don't expect much of said noise, but I don't feel
strongly about it.

> 
>>> +    {
>>> +        WCHAR path_buffer[MAX_PATH];
>>> +        UNICODE_STRING module_path = { 0, sizeof(path_buffer), path_buffer };
>>> +        path_buffer[0] = 0;
>>> +        LdrGetDllFullName( module, &module_path );
>>> +        WARN( "function %s (ordinal %d) not found in module %s\n",
>>> +              wine_dbgstr_a(name ? name->Buffer : NULL), ord, wine_dbgstr_w(path_buffer) );
>>
>> As long as we're calling get_modref() in this function anyway, if we're
>> concerned about printing the module name, I think it makes sense to just
>> access the WINE_MODREF directly. That way you wouldn't have to possibly
>> truncate the buffer (not that it probably matters).
> 
> You're right, I really didn't need to use LdrGetDllFullName. I will
> send a new patch that does not depend on it.
> 
> -Alex
>