[PATCH 0/1] MR957: rpcrt4: MIDL_STUB_MESSAGE needs to keep a refcount on IRpcChannelBuffer.

[Kevin Puetz (＠puetzk)]

It's possible for a proxy to be released during the middle of an Invoke.
A specific scenario where this happened was a single-shot event sink
which, upon receiving the event it was waiting for, would immediately
call DispEventUnadvise. This removed the proxy pointing to that sink
from the connection point's list of subscribers and released it.

For such cases all state used to complete the Invoke must be owned by
the __proxy_frame kept on the stack; after calling NdrProxySendReceive
(which pumps STA messages and permits reentrancy), anything inside
*This could be use-after-free. MIDL_STUB_MESSAGE already had its own
IRPCChannelBuffer *pRpcChannelBuffer, but lacked its own refcount.

When this does crash, the exception is caught by RpcTryFinally, but
still leads to leaks since NdrProxyFreeBuffer wasn't able to call
IRPCChannelBuffer::FreeBuffer. To fix this, StdProxy_GetChannel() now
calls AddRef() when setting __proxy_frame::_StubMsg::pRpcChannelBuffer
and NdrProxyFreeBuffer() calls the corresponding Release().

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/957

[PATCH 1/1] rpcrt4: MIDL_STUB_MESSAGE needs to keep a refcount on IRpcChannelBuffer.

[Kevin Puetz]

From: Kevin Puetz <PuetzKevinA(a)JohnDeere.com>

It's possible for a proxy to be released during the middle of an Invoke.
A specific scenario where this happened was a single-shot event sink
which, upon receiving the event it was waiting for, would immediately
call DispEventUnadvise. This removed the proxy pointing to that sink
from the connection point's list of subscribers and released it.

For such cases all state used to complete the Invoke must be owned by
the __proxy_frame kept on the stack; after calling NdrProxySendReceive
(which pumps STA messages and permits reentrancy), anything inside
*This could be use-after-free. MIDL_STUB_MESSAGE already had its own
IRPCChannelBuffer *pRpcChannelBuffer, but lacked its own refcount.

When this does crash, the exception is caught by RpcTryFinally, but
still leads to leaks since NdrProxyFreeBuffer wasn't able to call
IRPCChannelBuffer::FreeBuffer. To fix this, StdProxy_GetChannel() now
calls AddRef() when setting __proxy_frame::_StubMsg::pRpcChannelBuffer
and NdrProxyFreeBuffer() calls the corresponding Release().
---
 dlls/rpcrt4/cproxy.c      |   4 ++
 dlls/rpcrt4/tests/cstub.c | 133 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+)

diff --git a/dlls/rpcrt4/cproxy.c b/dlls/rpcrt4/cproxy.c
index da3e19e50f3..fc197b6f468 100644
--- a/dlls/rpcrt4/cproxy.c
+++ b/dlls/rpcrt4/cproxy.c
@@ -454,6 +454,9 @@ static void StdProxy_GetChannel(LPVOID iface,
   StdProxyImpl *This = impl_from_proxy_obj( iface );
   TRACE("(%p)->GetChannel(%p) %s\n",This,ppChannel,This->name);
 
+  if(This->pChannel)
+    IRpcChannelBuffer_AddRef(This->pChannel);
+
   *ppChannel = This->pChannel;
 }
 
@@ -585,6 +588,7 @@ void WINAPI NdrProxyFreeBuffer(void *This,
                                  (RPCOLEMESSAGE*)pStubMsg->RpcMsg);
     pStubMsg->fBufferValid = TRUE;
   }
+  IRpcChannelBuffer_Release(pStubMsg->pRpcChannelBuffer);
 }
 
 /***********************************************************************
diff --git a/dlls/rpcrt4/tests/cstub.c b/dlls/rpcrt4/tests/cstub.c
index d8b61d10a1a..90b97609bec 100644
--- a/dlls/rpcrt4/tests/cstub.c
+++ b/dlls/rpcrt4/tests/cstub.c
@@ -1415,6 +1415,138 @@ static void test_delegated_methods(void)
     ok(hr == S_OK, "got %#lx\n", hr);
 }
 
+typedef struct tagChannelBufferRefCount
+{
+    IRpcChannelBuffer IRpcChannelBuffer_iface;
+    LONG                                RefCount;
+} CChannelBufferRefCount;
+
+static CChannelBufferRefCount* impl_from_IRpcChannelBuffer(IRpcChannelBuffer* iface)
+{
+    return CONTAINING_RECORD(iface, CChannelBufferRefCount, IRpcChannelBuffer_iface);
+}
+
+static HRESULT WINAPI test_chanbuf_refcount_chan_query_interface(IRpcChannelBuffer* pchan,
+    REFIID iid,
+    void** ppv)
+{
+    if (IsEqualGUID(&IID_IRpcChannelBuffer, iid))
+    {
+        *ppv = pchan;
+        IRpcChannelBuffer_AddRef(pchan);
+        return S_OK;
+    }
+    return E_NOINTERFACE;
+}
+
+static ULONG WINAPI test_chanbuf_refcount_chan_add_ref(IRpcChannelBuffer* pchan)
+{
+    CChannelBufferRefCount* This = impl_from_IRpcChannelBuffer(pchan);
+    return InterlockedIncrement(&This->RefCount);
+}
+
+static ULONG WINAPI test_chanbuf_refcount_chan_release(IRpcChannelBuffer* pchan)
+{
+    CChannelBufferRefCount* This = impl_from_IRpcChannelBuffer(pchan);
+    return InterlockedDecrement(&This->RefCount);
+}
+
+static HRESULT WINAPI test_chanbuf_refcount_chan_get_buffer(IRpcChannelBuffer* pchan,
+    RPCOLEMESSAGE* msg,
+    REFIID iid)
+{
+    ok(0, "call to GetBuffer not expected\n");
+    return E_NOTIMPL;
+}
+
+static HRESULT WINAPI test_chanbuf_refcount_chan_send_receive(IRpcChannelBuffer* pchan,
+    RPCOLEMESSAGE* pMessage,
+    ULONG* pStatus)
+{
+    ok(0, "call to SendReceive not expected\n");
+    return E_NOTIMPL;
+}
+
+static HRESULT WINAPI test_chanbuf_refcount_chan_free_buffer(IRpcChannelBuffer* pchan,
+    RPCOLEMESSAGE* pMessage)
+{
+    ok(0, "call to FreeBuffer not expected\n");
+    return E_NOTIMPL;
+}
+
+static HRESULT WINAPI test_chanbuf_refcount_chan_get_dest_ctx(IRpcChannelBuffer* pchan,
+    DWORD* pdwDestContext,
+    void** ppvDestContext)
+{
+    *pdwDestContext = MSHCTX_LOCAL;
+    *ppvDestContext = NULL;
+    return S_OK;
+}
+
+static HRESULT WINAPI test_chanbuf_refcount_chan_is_connected(IRpcChannelBuffer* pchan)
+{
+    ok(0, "call to IsConnected not expected\n");
+    return E_NOTIMPL;
+}
+
+static IRpcChannelBufferVtbl test_chanbuf_refcount_test_rpc_chan_vtbl =
+{
+    test_chanbuf_refcount_chan_query_interface,
+    test_chanbuf_refcount_chan_add_ref,
+    test_chanbuf_refcount_chan_release,
+    test_chanbuf_refcount_chan_get_buffer,
+    test_chanbuf_refcount_chan_send_receive,
+    test_chanbuf_refcount_chan_free_buffer,
+    test_chanbuf_refcount_chan_get_dest_ctx,
+    test_chanbuf_refcount_chan_is_connected
+};
+
+static void test_ChannelBufferRefCount(IPSFactoryBuffer *ppsf)
+{
+    IRpcProxyBuffer* proxy_buffer = NULL;
+    IUnknown* proxy_if1 = NULL;
+    CChannelBufferRefCount test_chanbuf = {{&test_chanbuf_refcount_test_rpc_chan_vtbl}, 1};
+
+    RPC_MESSAGE rpcMessage = {0};
+    MIDL_STUB_MESSAGE stubMessage = {0};
+    MIDL_STUB_DESC stubDesc = {0};
+    ULONG refs;
+
+    HRESULT hr = IPSFactoryBuffer_CreateProxy(ppsf, NULL, &IID_if1, &proxy_buffer, (void**)&proxy_if1);
+    ok(hr == S_OK, "got %#lx\n", hr);
+
+    ok(test_chanbuf.RefCount == 1, "got %ld\n", test_chanbuf.RefCount);
+    hr = IRpcProxyBuffer_Connect(proxy_buffer, &test_chanbuf.IRpcChannelBuffer_iface);
+    ok(hr == S_OK, "got %#lx\n", hr);
+    /* proxy_buffer should have acquired its own refcount on test_chanbuf */
+    ok(test_chanbuf.RefCount == 2, "got %ld\n", test_chanbuf.RefCount);
+
+    /* which therefore survives releasing the initial one */
+    refs = IRpcChannelBuffer_Release(&test_chanbuf.IRpcChannelBuffer_iface);
+    ok(refs == 1, "got %ld\n", refs);
+
+    NdrProxyInitialize(proxy_if1, &rpcMessage, &stubMessage, &stubDesc, 0);
+    /* stubMessage should add its own refcount on test_chanbuf */
+    ok(test_chanbuf.RefCount == 2, "got %ld\n", test_chanbuf.RefCount);
+
+    /* stubMessage doesn't add its own refcounts on proxy_if1 or proxy_buffer,
+     * so it's possible these are freed out from under it.
+     * E.g. an event sink might unadvise upon receiving the event it was waiting for;
+     * this unadvise could be reentrant to Invoke because SendReceive pumps STA messages.
+     * The source would then erase that conection point entry and Release the proxy. */
+    IRpcProxyBuffer_Disconnect(proxy_buffer);
+    ok(test_chanbuf.RefCount == 1, "got %ld\n", test_chanbuf.RefCount);
+    IRpcProxyBuffer_Release(proxy_buffer);
+    refs = IUnknown_Release(proxy_if1);
+    ok(refs == 0, "got %ld\n", refs);
+    ok(test_chanbuf.RefCount == 1, "got %ld\n", test_chanbuf.RefCount);
+
+    /* NdrProxyFreeBuffer must not dereference the the now-freed proxy_if1,
+     * yet should still free the remaining reference on test_chanbuf */
+    NdrProxyFreeBuffer(proxy_if1, &stubMessage);
+    ok(test_chanbuf.RefCount == 0, "got %ld\n", test_chanbuf.RefCount);
+}
+
 START_TEST( cstub )
 {
     IPSFactoryBuffer *ppsf;
@@ -1440,6 +1572,7 @@ START_TEST( cstub )
     test_delegating_Invoke(ppsf);
     test_NdrDllRegisterProxy();
     test_delegated_methods();
+    test_ChannelBufferRefCount(ppsf);
 
     OleUninitialize();
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/957