[PATCH v2] ole32: Return IUnknown from GITCF_CreateInstance if requested

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com>
---
Without this patch, ArcMap.exe (part of ArcGIS Desktop 10.6) crashes
shortly after starting.

v2: Move the error message to the QueryInterface function and clean up
that function a bit.
---
 dlls/ole32/git.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/dlls/ole32/git.c b/dlls/ole32/git.c
index f7a0460154..e2ed8be91d 100644
--- a/dlls/ole32/git.c
+++ b/dlls/ole32/git.c
@@ -117,18 +117,18 @@ StdGlobalInterfaceTable_QueryInterface(IGlobalInterfaceTable* iface,
   /* Make sure silly coders can't crash us */
   if (ppvObject == 0) return E_INVALIDARG;
 
-  *ppvObject = 0; /* assume we don't have the interface */
-
   /* Do we implement that interface? */
   if (IsEqualIID(&IID_IUnknown, riid) ||
       IsEqualIID(&IID_IGlobalInterfaceTable, riid))
+  {
+    IGlobalInterfaceTable_AddRef(iface);
     *ppvObject = iface;
-  else
-    return E_NOINTERFACE;
+    return S_OK;
+  }
 
-  /* Now inc the refcount */
-  IGlobalInterfaceTable_AddRef(iface);
-  return S_OK;
+  *ppvObject = NULL;
+  ERR("(%s), not supported.\n", debugstr_guid(riid));
+  return E_NOINTERFACE;
 }
 
 static ULONG WINAPI
@@ -314,13 +314,7 @@ static HRESULT WINAPI
 GITCF_CreateInstance(LPCLASSFACTORY iface, LPUNKNOWN pUnk,
                      REFIID riid, LPVOID *ppv)
 {
-  if (IsEqualIID(riid,&IID_IGlobalInterfaceTable)) {
-    IGlobalInterfaceTable *git = get_std_git();
-    return IGlobalInterfaceTable_QueryInterface(git, riid, ppv);
-  }
-
-  FIXME("(%s), not supported.\n",debugstr_guid(riid));
-  return E_NOINTERFACE;
+  return IGlobalInterfaceTable_QueryInterface(get_std_git(), riid, ppv);
 }
 
 static HRESULT WINAPI GITCF_LockServer(LPCLASSFACTORY iface, BOOL fLock)
-- 
2.18.0

[Huw Davies]

On Sat, Jul 07, 2018 at 02:30:25PM +0200, Alex Henrie wrote:
> Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com>
> ---
> Without this patch, ArcMap.exe (part of ArcGIS Desktop 10.6) crashes
> shortly after starting.
>
> v2: Move the error message to the QueryInterface function and clean up
> that function a bit.
> ---
>  dlls/ole32/git.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/dlls/ole32/git.c b/dlls/ole32/git.c
> index f7a0460154..e2ed8be91d 100644
> --- a/dlls/ole32/git.c
> +++ b/dlls/ole32/git.c
> @@ -117,18 +117,18 @@ StdGlobalInterfaceTable_QueryInterface(IGlobalInterfaceTable* iface,
>    /* Make sure silly coders can't crash us */
>    if (ppvObject == 0) return E_INVALIDARG;
>
> -  *ppvObject = 0; /* assume we don't have the interface */
> -
>    /* Do we implement that interface? */
>    if (IsEqualIID(&IID_IUnknown, riid) ||
>        IsEqualIID(&IID_IGlobalInterfaceTable, riid))
> +  {
> +    IGlobalInterfaceTable_AddRef(iface);
>      *ppvObject = iface;
> -  else
> -    return E_NOINTERFACE;
> +    return S_OK;
> +  }
>
> -  /* Now inc the refcount */
> -  IGlobalInterfaceTable_AddRef(iface);
> -  return S_OK;
> +  *ppvObject = NULL;
> +  ERR("(%s), not supported.\n", debugstr_guid(riid));
> +  return E_NOINTERFACE;
>  }

I don't think there's any need to re-write this, it looks ok as it is.
You can add a FIXME (not an ERR) to the else block if you want.
> static ULONG WINAPI
> @@ -314,13 +314,7 @@ static HRESULT WINAPI
>  GITCF_CreateInstance(LPCLASSFACTORY iface, LPUNKNOWN pUnk,
>                       REFIID riid, LPVOID *ppv)
>  {
> -  if (IsEqualIID(riid,&IID_IGlobalInterfaceTable)) {
> -    IGlobalInterfaceTable *git = get_std_git();
> -    return IGlobalInterfaceTable_QueryInterface(git, riid, ppv);
> -  }
> -
> -  FIXME("(%s), not supported.\n",debugstr_guid(riid));
> -  return E_NOINTERFACE;
> +  return IGlobalInterfaceTable_QueryInterface(get_std_git(), riid, ppv);
>  }

What you should do is here is to something like:

  IGlobalInterfaceTable *git = get_std_git();
  hr = IGlobalInterfaceTable_QI(git, riid, ppv);
  IGlobalInterfaceTable_Release(git);
  return hr;
 
It doesn't really matter in this case because the git is a static
object, but it's good practice to keep the ref counts in order.

Huw.