Re: [2/2] oleaut32: Implement TLB dependencies lookup in resources (resend 3 - last submit before abandon)

[Sebastian Lackner]

I had this patch on my list for a couple of weeks, and finally had time to take a closer look.
There were still a lot of issues. I've listed them below, and have also added a fixed version to our staging tree:
https://github.com/wine-compholio/wine-staging/blob/master/patches/oleaut32-...
Feel free to integrate my changes and resubmit if you like.

On 27.09.2014 20:10, Guillaume Charifi wrote:
> Fixes https://bugs.winehq.org/show_bug.cgi?id=34184
>
> This is the last time without a review before I give up.
>
> --- dlls/oleaut32/tests/typelib.c | 2 +-
>   dlls/oleaut32/typelib.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 103 insertions(+), 10 deletions(-)
>
> diff --git a/dlls/oleaut32/tests/typelib.c b/dlls/oleaut32/tests/typelib.c
> index cb5bc23..99e2aedd 100644
> --- a/dlls/oleaut32/tests/typelib.c
> +++ b/dlls/oleaut32/tests/typelib.c
> @@ -5595,7 +5595,7 @@ static void test_dep(void) {
>     ok(hr == S_OK, "got: %x\n", hr);
>
> hr = ITypeInfo_GetRefTypeInfo(ptInfo, refType, &ptInfoExt);
> -    todo_wine ok(hr == S_OK || broken(hr == TYPE_E_CANTLOADLIBRARY) /* win 2000 */, "got: %x\n", hr);
> +    ok(hr == S_OK || broken(hr == TYPE_E_CANTLOADLIBRARY) /* win 2000 */, "got: %x\n", hr);
>
> ITypeInfo_Release(ptInfo);
>     if(ptInfoExt)
> diff --git a/dlls/oleaut32/typelib.c b/dlls/oleaut32/typelib.c
> index 48b171b..d895696 100644
> --- a/dlls/oleaut32/typelib.c
> +++ b/dlls/oleaut32/typelib.c
> @@ -7572,6 +7572,84 @@ static HRESULT ITypeInfoImpl_GetDispatchRefTypeInfo( ITypeInfo *iface,
>         return E_FAIL;
> }
>
> +struct search_res_tlb_param {
> +    GUID *guid;

GUID should be const.
> +    ITypeLib *pTLib;
> +    HRESULT result;
> +};

There is no need to store 'result' here. If the lookup fails the result is not used anyway. Comparing pTlib != NULL should be sufficient.
> +
> +static BOOL CALLBACK search_res_tlb(HMODULE hModule, LPCWSTR lpszType, LPWSTR lpszIntName, LONG_PTR lParam)
> +{
> +    struct search_res_tlb_param *This = (LPVOID)lParam;
> +
> +    HRESULT ret;
> +
> +    static WCHAR resFormatStrW[] = {'\\','%','d',0};

Should be const.
> +    WCHAR szPath[MAX_PATH];
> +    WCHAR szTLBPath[MAX_PATH];
> +    WCHAR szStringName[12];
> +    LPVOID pBase = NULL;
> +    DWORD dwTLBLength = 0;
> +    IUnknown *pFile = NULL;
> +    ITypeLib2 *pTLib = NULL;
> +    RPC_STATUS rpc_stat;
> +
> +    This->pTLib = NULL;
> +    This->result = E_FAIL;
> +
> +    if(IS_INTRESOURCE(lpszIntName) == FALSE)
> +        return TRUE;
> +
> +    GetModuleFileNameW(hModule, szPath, MAX_PATH);

Missing error checks.
> +
> +    strcpyW(szTLBPath, szPath);
> +    snprintfW(szStringName, sizeof(szStringName)/sizeof(WCHAR), resFormatStrW, PtrToInt(lpszIntName));
> +    strcatW(szTLBPath, szStringName);

Here also missing error checks. Moreover copying the data all the time is a bit ugly.
> +
> +    ret = TLB_PEFile_Open(szPath, PtrToInt(lpszIntName), &pBase, &dwTLBLength, &pFile);
> +    if (ret == TYPE_E_CANTLOADLIBRARY)
> +        ret = TLB_NEFile_Open(szPath, PtrToInt(lpszIntName), &pBase, &dwTLBLength, &pFile);
> +    if (ret == TYPE_E_CANTLOADLIBRARY)
> +        ret = TLB_Mapping_Open(szPath, &pBase, &dwTLBLength, &pFile);
> +    if (SUCCEEDED(ret))
> +    {
> +        if (dwTLBLength >= 4)
> +        {
> +            DWORD dwSignature = FromLEDWord(*((DWORD*) pBase));
> +            if (dwSignature == MSFT_SIGNATURE)
> +                pTLib = ITypeLib2_Constructor_MSFT(pBase, dwTLBLength);
> +            else if (dwSignature == SLTG_SIGNATURE)
> +                pTLib = ITypeLib2_Constructor_SLTG(pBase, dwTLBLength);
> +            else
> +            {
> +                FIXME("Header type magic 0x%08x not supported.\n", dwSignature);
> +                ret = E_FAIL;
> +            }
> +        }
> +        else
> +            ret = E_FAIL;
> +        IUnknown_Release(pFile);
> +    }

As already pointed out by Alexandre, this whole block is duplicated, although that is not really necessary. You can just call existing functions to load the typelib, SearchPathW will have no influence when its an absolute path.
> +
> +    if(pTLib) {
> +        ITypeLibImpl *impl = impl_from_ITypeLib2(pTLib);
> +        ret = UuidEqual((UUID *)This->guid, (UUID *)TLB_get_guid_null(impl->guid), &rpc_stat);

Why casting and using this API? The rest of the file uses IsEqualGUID. This also allows to get rid of the dummy rpc_stat variable.
Moreover you are storing a BOOL value in a HRESULT variable.
> +        ITypeLib_Release((ITypeLib *)pTLib);
> +
> +        ret = (ret == TRUE) ? LoadTypeLibEx(szTLBPath, REGKIND_NONE, (ITypeLib **)&pTLib) : E_FAIL;

That is especially ugly, because you free the library, just to load it again immediately afterwards. I don't think there is any good reason to do that, correct me if I'm wrong.
> +    }
> +
> +    This->result = ret;
> +
> +    if(SUCCEEDED(ret)) {
> +        /* If found, stop to enumerate */
> +        This->pTLib = (ITypeLib *)pTLib;
> +        return FALSE;
> +    }
> +
> +    return TRUE;
> +}
> +
> /* ITypeInfo::GetRefTypeInfo
>  *
>  * If a type description references other type descriptions, it retrieves
> @@ -7665,20 +7743,35 @@ static HRESULT WINAPI ITypeInfo_fnGetRefTypeInfo(
>                 ITypeLib_AddRef(pTLib);
>                 result = S_OK;
>             } else {
> +                static const WCHAR TYPELIBW[] = {'T','Y','P','E','L','I','B',0};
> +                struct search_res_tlb_param param;
>                 BSTR libnam;
>
> TRACE("typeinfo in imported typelib that isn't already loaded\n");
>
> -                result = query_typelib_path(TLB_get_guid_null(ref_type->pImpTLInfo->guid),
> -                        ref_type->pImpTLInfo->wVersionMajor,
> -                        ref_type->pImpTLInfo->wVersionMinor,
> -                        This->pTypeLib->syskind,
> -                        ref_type->pImpTLInfo->lcid, &libnam, TRUE);
> -                if(FAILED(result))
> -                    libnam = SysAllocString(ref_type->pImpTLInfo->name);
> +                /* Search in resource table */
> +                param.guid = (GUID *)TLB_get_guid_null(ref_type->pImpTLInfo->guid);

You are casting away the const here. Normally there is no cast required at all.
> +                param.pTLib = NULL;
> +                param.result = E_FAIL;
> +
> +                EnumResourceNamesW(NULL, TYPELIBW, search_res_tlb, (LONG_PTR)¶m);
> +
> +                pTLib = param.pTLib;
> +                result = param.result;
>
> -                result = LoadTypeLib(libnam, &pTLib);
> -                SysFreeString(libnam);
> +                /* Search on disk */
> +                if(FAILED(result)) {
> +                    result = query_typelib_path(TLB_get_guid_null(ref_type->pImpTLInfo->guid),
> +                            ref_type->pImpTLInfo->wVersionMajor,
> +                            ref_type->pImpTLInfo->wVersionMinor,
> +                            This->pTypeLib->syskind,
> +                            ref_type->pImpTLInfo->lcid, &libnam, TRUE);
> +                    if(FAILED(result))
> +                        libnam = SysAllocString(ref_type->pImpTLInfo->name);
> +
> +                    result = LoadTypeLib(libnam, &pTLib);
> +                    SysFreeString(libnam);
> +                }
>
> if(SUCCEEDED(result)) {
>                     ref_type->pImpTLInfo->pImpTypeLib = impl_from_ITypeLib(pTLib);