[tfaber] 70118: [QUARTZ] - Don't cache IFilterMapper2 interface in FilterGraph2. Fixes use after free when playing videos with MPC HC. - Fix a reference leak CORE-7671

Author: tfaber
Date: Thu Nov 26 22:04:34 2015
New Revision: 70118

URL: http://svn.reactos.org/svn/reactos?rev=70118&view=rev
Log:
[QUARTZ]
- Don't cache IFilterMapper2 interface in FilterGraph2. Fixes use after free when
playing videos with MPC HC.
- Fix a reference leak
CORE-7671

Modified:
    trunk/reactos/dll/directx/wine/quartz/filtergraph.c
    trunk/rostests/winetests/quartz/filtergraph.c
    trunk/rostests/winetests/quartz/filtermapper.c

Modified: trunk/reactos/dll/directx/wine/quartz/filtergraph.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/dll/directx/wine/quartz/fi…
==============================================================================
--- trunk/reactos/dll/directx/wine/quartz/filtergraph.c	[iso-8859-1] (original)
+++ trunk/reactos/dll/directx/wine/quartz/filtergraph.c	[iso-8859-1] Thu Nov 26 22:04:34
2015
@@ -155,7 +155,6 @@
     IUnknown *outer_unk;
     LONG ref;
     IUnknown *punkFilterMapper2;
-    IFilterMapper2 * pFilterMapper2;
     IBaseFilter ** ppFiltersInGraph;
     LPWSTR * pFilterNames;
     ULONG nFilters;
@@ -243,11 +242,11 @@
         TRACE("   requesting IFilterMapper interface from aggregated filtermapper
(%p)\n", *ppvObj);
         return IUnknown_QueryInterface(This->punkFilterMapper2, riid, ppvObj);
     } else if (IsEqualGUID(&IID_IFilterMapper2, riid)) {
-        *ppvObj = This->pFilterMapper2;
         TRACE("   returning IFilterMapper2 interface from aggregated filtermapper
(%p)\n", *ppvObj);
+        return IUnknown_QueryInterface(This->punkFilterMapper2, riid, ppvObj);
     } else if (IsEqualGUID(&IID_IFilterMapper3, riid)) {
-        *ppvObj = This->pFilterMapper2;
         TRACE("   returning IFilterMapper3 interface from aggregated filtermapper
(%p)\n", *ppvObj);
+        return IUnknown_QueryInterface(This->punkFilterMapper2, riid, ppvObj);
     } else if (IsEqualGUID(&IID_IGraphVersion, riid)) {
         *ppvObj = &This->IGraphConfig_iface;
         TRACE("   returning IGraphConfig interface (%p)\n", *ppvObj);
@@ -297,9 +296,6 @@
                 IUnknown_Release(This->ItfCacheEntries[i].iface);
         }
 
-        /* AddRef on controlling IUnknown, to compensate for Release of cached
IFilterMapper2 */
-        IUnknown_AddRef(This->outer_unk);
-        IFilterMapper2_Release(This->pFilterMapper2);
         IUnknown_Release(This->punkFilterMapper2);
 
         if (This->pSite) IUnknown_Release(This->pSite);
@@ -888,6 +884,7 @@
     CLSID FilterCLSID;
     PIN_DIRECTION dir;
     unsigned int i = 0;
+    IFilterMapper2 *pFilterMapper2 = NULL;
 
     TRACE("(%p/%p)->(%p, %p)\n", This, iface, ppinOut, ppinIn);
 
@@ -977,10 +974,16 @@
     TRACE("MajorType %s\n", debugstr_guid(&mt->majortype));
     TRACE("SubType %s\n", debugstr_guid(&mt->subtype));
 
+    hr = IUnknown_QueryInterface(This->punkFilterMapper2, &IID_IFilterMapper2,
(void**)&pFilterMapper2);
+    if (FAILED(hr)) {
+        WARN("Unable to get IFilterMapper2 (%x)\n", hr);
+        goto out;
+    }
+
     /* Try to find a suitable filter that can connect to the pin to render */
     tab[0] = mt->majortype;
     tab[1] = mt->subtype;
-    hr = IFilterMapper2_EnumMatchingFilters(This->pFilterMapper2, &pEnumMoniker,
0, FALSE, MERIT_UNLIKELY, TRUE, 1, tab, NULL, NULL, FALSE, FALSE, 0, NULL, NULL, NULL);
+    hr = IFilterMapper2_EnumMatchingFilters(pFilterMapper2, &pEnumMoniker, 0, FALSE,
MERIT_UNLIKELY, TRUE, 1, tab, NULL, NULL, FALSE, FALSE, 0, NULL, NULL, NULL);
     if (FAILED(hr)) {
         WARN("Unable to enum filters (%x)\n", hr);
         goto out;
@@ -1147,7 +1150,11 @@
         CoTaskMemFree(ppins);
     }
 
+    IEnumMoniker_Release(pEnumMoniker);
+
 out:
+    if (pFilterMapper2)
+        IFilterMapper2_Release(pFilterMapper2);
     if (penummt)
         IEnumMediaTypes_Release(penummt);
     if (mt)
@@ -1243,6 +1250,7 @@
     ULONG nb;
     IMoniker* pMoniker;
     INT x;
+    IFilterMapper2 *pFilterMapper2 = NULL;
 
     TRACE("(%p/%p)->(%p)\n", This, iface, ppinOut);
 
@@ -1352,10 +1360,20 @@
                 continue;
             }
 
+            if (pFilterMapper2 == NULL)
+            {
+                hr = IUnknown_QueryInterface(This->punkFilterMapper2,
&IID_IFilterMapper2, (void**)&pFilterMapper2);
+                if (FAILED(hr))
+                {
+                    WARN("Unable to query IFilterMapper2 (%x)\n", hr);
+                    break;
+                }
+            }
+
             /* Try to find a suitable renderer with the same media type */
             tab[0] = mt->majortype;
             tab[1] = mt->subtype;
-            hr = IFilterMapper2_EnumMatchingFilters(This->pFilterMapper2,
&pEnumMoniker, 0, FALSE, MERIT_UNLIKELY, TRUE, 1, tab, NULL, NULL, FALSE, FALSE, 0,
NULL, NULL, NULL);
+            hr = IFilterMapper2_EnumMatchingFilters(pFilterMapper2, &pEnumMoniker, 0,
FALSE, MERIT_UNLIKELY, TRUE, 1, tab, NULL, NULL, FALSE, FALSE, 0, NULL, NULL, NULL);
             if (FAILED(hr))
             {
                 WARN("Unable to enum filters (%x)\n", hr);
@@ -1468,6 +1486,9 @@
             break;
         hr = S_OK;
     }
+
+    if (pFilterMapper2)
+        IFilterMapper2_Release(pFilterMapper2);
 
     IEnumMediaTypes_Release(penummt);
     return hr;
@@ -5684,14 +5705,6 @@
     hr = CoCreateInstance(&CLSID_FilterMapper2, fimpl->outer_unk,
CLSCTX_INPROC_SERVER,
             &IID_IUnknown, (void**)&fimpl->punkFilterMapper2);
 
-    if (SUCCEEDED(hr))
-        hr = IUnknown_QueryInterface(fimpl->punkFilterMapper2,
&IID_IFilterMapper2,
-                (void**)&fimpl->pFilterMapper2);
-
-    if (SUCCEEDED(hr))
-        /* Release controlling IUnknown - compensate refcount increase from caching
IFilterMapper2 interface. */
-        IUnknown_Release(fimpl->outer_unk);
-
     if (FAILED(hr)) {
         ERR("Unable to create filter mapper (%x)\n", hr);
         if (fimpl->punkFilterMapper2) IUnknown_Release(fimpl->punkFilterMapper2);

Modified: trunk/rostests/winetests/quartz/filtergraph.c
URL:
http://svn.reactos.org/svn/reactos/trunk/rostests/winetests/quartz/filtergr…
==============================================================================
--- trunk/rostests/winetests/quartz/filtergraph.c	[iso-8859-1] (original)
+++ trunk/rostests/winetests/quartz/filtergraph.c	[iso-8859-1] Thu Nov 26 22:04:34 2015
@@ -1886,6 +1886,98 @@
     ok(hr == S_OK, "CoRevokeClassObject failed with %08x\n", hr);
 }
 
+typedef struct IUnknownImpl
+{
+    IUnknown IUnknown_iface;
+    int AddRef_called;
+    int Release_called;
+} IUnknownImpl;
+
+static IUnknownImpl *IUnknownImpl_from_iface(IUnknown * iface)
+{
+    return CONTAINING_RECORD(iface, IUnknownImpl, IUnknown_iface);
+}
+
+static HRESULT WINAPI IUnknownImpl_QueryInterface(IUnknown * iface, REFIID riid, LPVOID *
ppv)
+{
+    ok(0, "QueryInterface should not be called for %s\n",
wine_dbgstr_guid(riid));
+    return E_NOINTERFACE;
+}
+
+static ULONG WINAPI IUnknownImpl_AddRef(IUnknown * iface)
+{
+    IUnknownImpl *This = IUnknownImpl_from_iface(iface);
+    This->AddRef_called++;
+    return 2;
+}
+
+static ULONG WINAPI IUnknownImpl_Release(IUnknown * iface)
+{
+    IUnknownImpl *This = IUnknownImpl_from_iface(iface);
+    This->Release_called++;
+    return 1;
+}
+
+static CONST_VTBL IUnknownVtbl IUnknownImpl_Vtbl =
+{
+    IUnknownImpl_QueryInterface,
+    IUnknownImpl_AddRef,
+    IUnknownImpl_Release
+};
+
+static void test_aggregate_filter_graph(void)
+{
+    HRESULT hr;
+    IUnknown *pgraph;
+    IUnknown *punk;
+    IUnknownImpl unk_outer = { { &IUnknownImpl_Vtbl }, 0, 0 };
+
+    hr = CoCreateInstance(&CLSID_FilterGraph, &unk_outer.IUnknown_iface,
CLSCTX_INPROC_SERVER,
+                          &IID_IUnknown, (void **)&pgraph);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(pgraph != &unk_outer.IUnknown_iface, "pgraph = %p, expected not
%p\n", pgraph, &unk_outer.IUnknown_iface);
+
+    hr = IUnknown_QueryInterface(pgraph, &IID_IUnknown, (void **)&punk);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(punk != &unk_outer.IUnknown_iface, "punk = %p, expected not %p\n",
punk, &unk_outer.IUnknown_iface);
+    IUnknown_Release(punk);
+
+    ok(unk_outer.AddRef_called == 0, "IUnknownImpl_AddRef called %d times\n",
unk_outer.AddRef_called);
+    ok(unk_outer.Release_called == 0, "IUnknownImpl_Release called %d times\n",
unk_outer.Release_called);
+    unk_outer.AddRef_called = 0;
+    unk_outer.Release_called = 0;
+
+    hr = IUnknown_QueryInterface(pgraph, &IID_IFilterMapper, (void **)&punk);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(punk != &unk_outer.IUnknown_iface, "punk = %p, expected not %p\n",
punk, &unk_outer.IUnknown_iface);
+    IUnknown_Release(punk);
+
+    ok(unk_outer.AddRef_called == 1, "IUnknownImpl_AddRef called %d times\n",
unk_outer.AddRef_called);
+    ok(unk_outer.Release_called == 1, "IUnknownImpl_Release called %d times\n",
unk_outer.Release_called);
+    unk_outer.AddRef_called = 0;
+    unk_outer.Release_called = 0;
+
+    hr = IUnknown_QueryInterface(pgraph, &IID_IFilterMapper2, (void **)&punk);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(punk != &unk_outer.IUnknown_iface, "punk = %p, expected not %p\n",
punk, &unk_outer.IUnknown_iface);
+    IUnknown_Release(punk);
+
+    ok(unk_outer.AddRef_called == 1, "IUnknownImpl_AddRef called %d times\n",
unk_outer.AddRef_called);
+    ok(unk_outer.Release_called == 1, "IUnknownImpl_Release called %d times\n",
unk_outer.Release_called);
+    unk_outer.AddRef_called = 0;
+    unk_outer.Release_called = 0;
+
+    hr = IUnknown_QueryInterface(pgraph, &IID_IFilterMapper3, (void **)&punk);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(punk != &unk_outer.IUnknown_iface, "punk = %p, expected not %p\n",
punk, &unk_outer.IUnknown_iface);
+    IUnknown_Release(punk);
+
+    ok(unk_outer.AddRef_called == 1, "IUnknownImpl_AddRef called %d times\n",
unk_outer.AddRef_called);
+    ok(unk_outer.Release_called == 1, "IUnknownImpl_Release called %d times\n",
unk_outer.Release_called);
+
+    IUnknown_Release(pgraph);
+}
+
 START_TEST(filtergraph)
 {
     HRESULT hr;
@@ -1904,5 +1996,6 @@
     test_mediacontrol();
     test_filter_graph2();
     test_render_filter_priority();
+    test_aggregate_filter_graph();
     CoUninitialize();
 }

Modified: trunk/rostests/winetests/quartz/filtermapper.c
URL:
http://svn.reactos.org/svn/reactos/trunk/rostests/winetests/quartz/filterma…
==============================================================================
--- trunk/rostests/winetests/quartz/filtermapper.c	[iso-8859-1] (original)
+++ trunk/rostests/winetests/quartz/filtermapper.c	[iso-8859-1] Thu Nov 26 22:04:34 2015
@@ -541,6 +541,98 @@
         IFilterMapper2_Release(pMapper);
 }
 
+typedef struct IUnknownImpl
+{
+    IUnknown IUnknown_iface;
+    int AddRef_called;
+    int Release_called;
+} IUnknownImpl;
+
+static IUnknownImpl *IUnknownImpl_from_iface(IUnknown * iface)
+{
+    return CONTAINING_RECORD(iface, IUnknownImpl, IUnknown_iface);
+}
+
+static HRESULT WINAPI IUnknownImpl_QueryInterface(IUnknown * iface, REFIID riid, LPVOID *
ppv)
+{
+    ok(0, "QueryInterface should not be called for %s\n",
wine_dbgstr_guid(riid));
+    return E_NOINTERFACE;
+}
+
+static ULONG WINAPI IUnknownImpl_AddRef(IUnknown * iface)
+{
+    IUnknownImpl *This = IUnknownImpl_from_iface(iface);
+    This->AddRef_called++;
+    return 2;
+}
+
+static ULONG WINAPI IUnknownImpl_Release(IUnknown * iface)
+{
+    IUnknownImpl *This = IUnknownImpl_from_iface(iface);
+    This->Release_called++;
+    return 1;
+}
+
+static CONST_VTBL IUnknownVtbl IUnknownImpl_Vtbl =
+{
+    IUnknownImpl_QueryInterface,
+    IUnknownImpl_AddRef,
+    IUnknownImpl_Release
+};
+
+static void test_aggregate_filter_mapper(void)
+{
+    HRESULT hr;
+    IUnknown *pmapper;
+    IUnknown *punk;
+    IUnknownImpl unk_outer = { { &IUnknownImpl_Vtbl }, 0, 0 };
+
+    hr = CoCreateInstance(&CLSID_FilterMapper2, &unk_outer.IUnknown_iface,
CLSCTX_INPROC_SERVER,
+                          &IID_IUnknown, (void **)&pmapper);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(pmapper != &unk_outer.IUnknown_iface, "pmapper = %p, expected not
%p\n", pmapper, &unk_outer.IUnknown_iface);
+
+    hr = IUnknown_QueryInterface(pmapper, &IID_IUnknown, (void **)&punk);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(punk != &unk_outer.IUnknown_iface, "punk = %p, expected not %p\n",
punk, &unk_outer.IUnknown_iface);
+    IUnknown_Release(punk);
+
+    ok(unk_outer.AddRef_called == 0, "IUnknownImpl_AddRef called %d times\n",
unk_outer.AddRef_called);
+    ok(unk_outer.Release_called == 0, "IUnknownImpl_Release called %d times\n",
unk_outer.Release_called);
+    unk_outer.AddRef_called = 0;
+    unk_outer.Release_called = 0;
+
+    hr = IUnknown_QueryInterface(pmapper, &IID_IFilterMapper, (void **)&punk);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(punk != &unk_outer.IUnknown_iface, "punk = %p, expected not %p\n",
punk, &unk_outer.IUnknown_iface);
+    IUnknown_Release(punk);
+
+    ok(unk_outer.AddRef_called == 1, "IUnknownImpl_AddRef called %d times\n",
unk_outer.AddRef_called);
+    ok(unk_outer.Release_called == 1, "IUnknownImpl_Release called %d times\n",
unk_outer.Release_called);
+    unk_outer.AddRef_called = 0;
+    unk_outer.Release_called = 0;
+
+    hr = IUnknown_QueryInterface(pmapper, &IID_IFilterMapper2, (void **)&punk);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(punk != &unk_outer.IUnknown_iface, "punk = %p, expected not %p\n",
punk, &unk_outer.IUnknown_iface);
+    IUnknown_Release(punk);
+
+    ok(unk_outer.AddRef_called == 1, "IUnknownImpl_AddRef called %d times\n",
unk_outer.AddRef_called);
+    ok(unk_outer.Release_called == 1, "IUnknownImpl_Release called %d times\n",
unk_outer.Release_called);
+    unk_outer.AddRef_called = 0;
+    unk_outer.Release_called = 0;
+
+    hr = IUnknown_QueryInterface(pmapper, &IID_IFilterMapper3, (void **)&punk);
+    ok(hr == S_OK, "CoCreateInstance returned %x\n", hr);
+    ok(punk != &unk_outer.IUnknown_iface, "punk = %p, expected not %p\n",
punk, &unk_outer.IUnknown_iface);
+    IUnknown_Release(punk);
+
+    ok(unk_outer.AddRef_called == 1, "IUnknownImpl_AddRef called %d times\n",
unk_outer.AddRef_called);
+    ok(unk_outer.Release_called == 1, "IUnknownImpl_Release called %d times\n",
unk_outer.Release_called);
+
+    IUnknown_Release(pmapper);
+}
+
 START_TEST(filtermapper)
 {
     CoInitialize(NULL);
@@ -550,6 +642,7 @@
     test_ifiltermapper_from_filtergraph();
     test_register_filter_with_null_clsMinorType();
     test_parse_filter_data();
+    test_aggregate_filter_mapper();
 
     CoUninitialize();
 }