[PATCH v3 0/2] MR957: rpcrt4: MIDL_STUB_MESSAGE needs to keep a refcount on IRpcChannelBuffer.
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(). -- v3: rpcrt4: Add a refcount owned by MIDL_STUB_MESSAGE. rpcrt4/tests: Add tests for lifetime of IRpcChannelBuffer. https://gitlab.winehq.org/wine/wine/-/merge_requests/957
From: Kevin Puetz <PuetzKevinA(a)JohnDeere.com> It's possible for a proxy to be released during the middle of an Invoke. E.g. a specific case 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 the last refcount on the proxy itself. Therefore, all state used to complete an RPC call must be on the stack; once NdrProxySendReceive pumps STA messages and permits reentrancy, the proxy cannot be accessed or relied on to own anything. Add test showing MIDL_STUB_MESSAGE::pRpcChannelBuffer owns a refcount (to ensure it can read [out] parameters from the channel) --- dlls/rpcrt4/tests/cstub.c | 135 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/dlls/rpcrt4/tests/cstub.c b/dlls/rpcrt4/tests/cstub.c index d8b61d10a1a..57e69389ebd 100644 --- a/dlls/rpcrt4/tests/cstub.c +++ b/dlls/rpcrt4/tests/cstub.c @@ -1415,6 +1415,140 @@ 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 */ + todo_wine ok(test_chanbuf.RefCount == 2, "got %ld\n", test_chanbuf.RefCount); + ok(stubMessage.pRpcChannelBuffer != NULL, "NULL pRocChannelBuffer\n"); + + /* 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); + todo_wine 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); + todo_wine 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); + todo_wine ok(!stubMessage.pRpcChannelBuffer, "dangling pRpcChannelBuffer = %p\n", stubMessage.pRpcChannelBuffer); +} + START_TEST( cstub ) { IPSFactoryBuffer *ppsf; @@ -1440,6 +1574,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
From: Kevin Puetz <PuetzKevinA(a)JohnDeere.com> In cases where this could have been use-after-free, exceptions were caught/hidden by RpcTryFinally, but still lead to leaks since NdrProxyFreeBuffer wasn't able to call IRPCChannelBuffer::FreeBuffer. StdProxy_GetChannel() now AddRef() on its return value (used to set __proxy_frame::_StubMsg::pRpcChannelBuffer), and NdrProxyFreeBuffer() calls the corresponding Release() and clears the now-weak pointer. This makes the behavior of these function match the observed test results, and fixes the crash/leak when a proxy is released mid-Invoke. --- dlls/rpcrt4/cproxy.c | 5 +++++ dlls/rpcrt4/tests/cstub.c | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/dlls/rpcrt4/cproxy.c b/dlls/rpcrt4/cproxy.c index da3e19e50f3..426b1425d3a 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,8 @@ void WINAPI NdrProxyFreeBuffer(void *This, (RPCOLEMESSAGE*)pStubMsg->RpcMsg); pStubMsg->fBufferValid = TRUE; } + IRpcChannelBuffer_Release(pStubMsg->pRpcChannelBuffer); + pStubMsg->pRpcChannelBuffer = NULL; } /*********************************************************************** diff --git a/dlls/rpcrt4/tests/cstub.c b/dlls/rpcrt4/tests/cstub.c index 57e69389ebd..d44ee36088c 100644 --- a/dlls/rpcrt4/tests/cstub.c +++ b/dlls/rpcrt4/tests/cstub.c @@ -1527,7 +1527,7 @@ static void test_ChannelBufferRefCount(IPSFactoryBuffer *ppsf) NdrProxyInitialize(proxy_if1, &rpcMessage, &stubMessage, &stubDesc, 0); /* stubMessage should add its own refcount on test_chanbuf */ - todo_wine ok(test_chanbuf.RefCount == 2, "got %ld\n", test_chanbuf.RefCount); + ok(test_chanbuf.RefCount == 2, "got %ld\n", test_chanbuf.RefCount); ok(stubMessage.pRpcChannelBuffer != NULL, "NULL pRocChannelBuffer\n"); /* stubMessage doesn't add its own refcounts on proxy_if1 or proxy_buffer, @@ -1536,17 +1536,17 @@ static void test_ChannelBufferRefCount(IPSFactoryBuffer *ppsf) * 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); - todo_wine ok(test_chanbuf.RefCount == 1, "got %ld\n", test_chanbuf.RefCount); + 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); - todo_wine ok(test_chanbuf.RefCount == 1, "got %ld\n", test_chanbuf.RefCount); + 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); - todo_wine ok(!stubMessage.pRpcChannelBuffer, "dangling pRpcChannelBuffer = %p\n", stubMessage.pRpcChannelBuffer); + ok(!stubMessage.pRpcChannelBuffer, "dangling pRpcChannelBuffer = %p\n", stubMessage.pRpcChannelBuffer); } START_TEST( cstub ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/957
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124880 Your paranoid android. === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24825. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24825. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24825.
On Tue Oct 11 06:30:50 2022 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124880 Your paranoid android. === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24825. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24825. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24825. ``` I see these `Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720` messages on every recent MR, so I don't think they have anything to do with my changes.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/957#note_10286
This merge request was approved by Huw Davies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/957
participants (4)
-
Huw Davies (＠huw) -
Kevin Puetz
-
Kevin Puetz (＠puetzk)
-
Marvin