Re: [PATCH v2] ntdll/unix: Don't access empty IMAGE_FILE_IMPORT_DIRECTORY.

[Zebediah Figura (she/her)]

On 11/23/20 7:55 PM, Kevin Puetz wrote:
> If there are no imports at all, winebuild will omit __wine_spec_imports,
> so both VirtualAddress and Size will be 0. In this case get_rva(module, 0)
> does not point to a valid IMAGE_IMPORT_DESCRIPTOR.
>
> Signed-off-by: Kevin Puetz
>
> v2: Tweaked whitespace and commit message per feedback from Gijs Vermeulen
>
> diff --git a/dlls/ntdll/unix/loader.c b/dlls/ntdll/unix/loader.c
> index c2b6ea603e3..5d81e26cc5b 100644
> --- a/dlls/ntdll/unix/loader.c
> +++ b/dlls/ntdll/unix/loader.c
> @@ -809,43 +809,49 @@ static inline void *get_rva( void *module, ULONG_PTR addr )
>  static NTSTATUS fixup_ntdll_imports( const char *name, HMODULE module )
>  {
>      const IMAGE_NT_HEADERS *nt;
> - const IMAGE_IMPORT_DESCRIPTOR *descr;
> -
>
> const IMAGE_DATA_DIRECTORY *dir;
> const IMAGE_THUNK_DATA *import_list;
> IMAGE_THUNK_DATA *thunk_list;
>
> nt = get_rva( module, ((IMAGE_DOS_HEADER *)module)->e_lfanew );
> - descr = get_rva( module, nt->OptionalHeader.DataDirectory[IMAGE_FILE_IMPORT_DIRECTORY].VirtualAddress );
> - for (; descr->Name && descr->FirstThunk; descr++)
> -
> - dir = &nt->OptionalHeader.DataDirectory[IMAGE_FILE_IMPORT_DIRECTORY];
> - if (dir->Size)
> {
> -
>    thunk_list = get_rva( module, descr->FirstThunk );
> -
>    const IMAGE_IMPORT_DESCRIPTOR *descr = get_rva( module, dir->VirtualAddress );
>
> -
>    /* ntdll must be the only import */
> -
>    if (strcmp( get_rva( module, descr->Name ), "ntdll.dll" ))
> -
>    for (; descr->Name && descr->FirstThunk; descr++)
>    {
> -
>        ERR( "module %s is importing %s\n", debugstr_a(name), (char *)get_rva( module, descr->Name ));
> -
>        return STATUS_PROCEDURE_NOT_FOUND;
> -
>    }
> -
>    if (descr->u.OriginalFirstThunk)
> -
>        import_list = get_rva( module, descr->u.OriginalFirstThunk );
> -
>    else
> -
>        import_list = thunk_list;
> -
>        thunk_list = get_rva( module, descr->FirstThunk );
>
> -
>    while (import_list->u1.Ordinal)
> -
>    {
> -
>        if (IMAGE_SNAP_BY_ORDINAL( import_list->u1.Ordinal ))
> -
>        /* ntdll must be the only import */
> -
>        if (strcmp( get_rva( module, descr->Name ), "ntdll.dll" ))
>        {
> -
>            int ordinal = IMAGE_ORDINAL( import_list->u1.Ordinal ) - ntdll_exports->Base;
> -
>            thunk_list->u1.Function = find_ordinal_export( ntdll_module, ntdll_exports, ordinal );
> -
>            if (!thunk_list->u1.Function) ERR( "%s: ntdll.%u not found\n", debugstr_a(name), ordinal );
> -
>            ERR( "module %s is importing %s\n", debugstr_a(name), (char *)get_rva( module, descr->Name ));
> -
>            return STATUS_PROCEDURE_NOT_FOUND;
>        }
> -
>        else  /* import by name */
> -
>        if (descr->u.OriginalFirstThunk)
> -
>            import_list = get_rva( module, descr->u.OriginalFirstThunk );
> -
>        else
> -
>            import_list = thunk_list;
> -
> -
>        while (import_list->u1.Ordinal)
>        {
> -
>            IMAGE_IMPORT_BY_NAME *pe_name = get_rva( module, import_list->u1.AddressOfData );
> -
>            thunk_list->u1.Function = find_pe_export( ntdll_module, ntdll_exports, pe_name );
> -
>            if (!thunk_list->u1.Function) ERR( "%s: ntdll.%s not found\n", debugstr_a(name), pe_name->Name );
> -
>            if (IMAGE_SNAP_BY_ORDINAL( import_list->u1.Ordinal ))
> -
>            {
> -
>                int ordinal = IMAGE_ORDINAL( import_list->u1.Ordinal ) - ntdll_exports->Base;
> -
>                thunk_list->u1.Function = find_ordinal_export( ntdll_module, ntdll_exports, ordinal );
> -
>                if (!thunk_list->u1.Function) ERR( "%s: ntdll.%u not found\n", debugstr_a(name), ordinal );
> -
>            }
> -
>            else  /* import by name */
> -
>            {
> -
>                IMAGE_IMPORT_BY_NAME *pe_name = get_rva( module, import_list->u1.AddressOfData );
> -
>                thunk_list->u1.Function = find_pe_export( ntdll_module, ntdll_exports, pe_name );
> -
>                if (!thunk_list->u1.Function) ERR( "%s: ntdll.%s not found\n", debugstr_a(name), pe_name->Name );
> -
>            }
> -
>            import_list++;
> -
>            thunk_list++;
>        }
> -
>        import_list++;
> -
>        thunk_list++;
>    }
> }
>    return STATUS_SUCCESS;

This seems like it could be simplified by just using early return instead.