[PATCH 0/1] MR8021: Revert "combase: Find correct apartment in ClientRpcChannelBuffer_SendReceive()."

[PATCH 1/1] Revert "combase: Find correct apartment in ClientRpcChannelBuffer_SendReceive()."

[Dmitry Timoshkov]

From: Dmitry Timoshkov

This reverts commit 68a9046e30dd5ea8ab9fc753c728211751694b32.

This commit broke dlls/ole32/marshal.c tests.

Signed-off-by: Dmitry Timoshkov 
---
 dlls/combase/rpc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/dlls/combase/rpc.c b/dlls/combase/rpc.c
index 160c5c579b2..c51b59de4bf 100644
--- a/dlls/combase/rpc.c
+++ b/dlls/combase/rpc.c
@@ -1344,6 +1344,15 @@ static DWORD WINAPI rpc_sendreceive_thread(LPVOID param)
     return 0;
 }

+static inline HRESULT ClientRpcChannelBuffer_IsCorrectApartment(ClientRpcChannelBuffer *This, const struct apartment *apt)
+{
+    if (!apt)
+        return S_FALSE;
+    if (This->oxid != apartment_getoxid(apt))
+        return S_FALSE;
+    return S_OK;
+}
+
 static HRESULT WINAPI ClientRpcChannelBuffer_SendReceive(LPRPCCHANNELBUFFER iface, RPCOLEMESSAGE *olemsg, ULONG *pstatus)
 {
     ClientRpcChannelBuffer *This = (ClientRpcChannelBuffer *)iface;
@@ -1356,15 +1365,17 @@ static HRESULT WINAPI ClientRpcChannelBuffer_SendReceive(LPRPCCHANNELBUFFER ifac
     ORPC_EXTENT_ARRAY orpc_ext_array;
     WIRE_ORPC_EXTENT *first_wire_orpc_extent = NULL;
     HRESULT hrFault = S_OK;
-    struct apartment *apt = apartment_findfromoxid(This->oxid);
+    struct apartment *apt = apartment_get_current_or_mta();
     struct tlsdata *tlsdata;

TRACE("%p, iMethod %ld\n", olemsg, olemsg->iMethod);

-    if (!apt)
+    hr = ClientRpcChannelBuffer_IsCorrectApartment(This, apt);
+    if (hr != S_OK)
     {
         ERR("called from wrong apartment, should have been 0x%s\n",
             wine_dbgstr_longlong(This->oxid));
+        if (apt) apartment_release(apt);
         return RPC_E_WRONG_THREAD;
     }

[Rémi Bernon (＠rbernon)]

I think this should be reverted and overall it doesn't look like to be a similar scenario as what
b4da14035741f287130002b512c7d0e9117da80a is addressing (fwiw I added a test for b4da14035741f287130002b512c7d0e9117da80a in !8235 which shows that it was correct indeed).

The `apartment_get_current_or_mta` functions returns the current thread
apartment first, then the MTA if it exists, or NULL if neither.

The `ClientRpcChannelBuffer` oxid is set in `rpc_create_clientchannel`,
called from `std_unmarshal_interface`, from the apartment returned by
`apartment_get_current_or_mta` at that time.

In `ClientRpcChannelBuffer_IsCorrectApartment`, it was compared to the
oxid of the apartment returned from `apartment_get_current_or_mta`, when
any remote method is being called.

In Wine, OXID are not dynamically generated but instead are based on
PID:TID for STAs, and PID:0xcafe for the MTA, so they would only change
between thread STAs and between STA and MTA, even if apartments are
re-created.

So, IIUC the check would fail if:

* Interface has been unmarshaled in the MTA, and:

* The thread calling the method is in an STA. This is tested already
    by `implicit_mta_unmarshal_proc` and `implicit_mta_use_proc`: calling
    from another thread in the MTA works but if that thread joins an
    STA it should fail.

* The thread calling the method doesn't have an apartment. This is
    tested in `test_proxy_used_in_wrong_thread` already and should fail.

* Interface has been unmarshaled in an STA, and:

* The thread calling the method is not in an apartment. This is
    tested in `test_proxy_used_in_wrong_thread` already and should fail.

* The thread calling the method is explicitly in the MTA. This is
    tested in `test_proxy_used_in_wrong_thread` already and should fail.

* The thread calling the method is in an implicit MTA. !8235 adds
    such a case to `test_proxy_used_in_wrong_thread` but it should fail too.

* The thread calling the method is in another STA. !8235 adds such
    a case to `test_proxy_used_in_wrong_thread` but it should fail too.

I don't see any other case where the check could be wrong, so if there's
something going on it probably comes from somewhere else and the change
should be reverted.

[PATCH 0/1] MR8021: Revert "combase: Find correct apartment in ClientRpcChannelBuffer_SendReceive()." - approved

[Rémi Bernon (＠rbernon)]

This merge request was approved by Rémi Bernon.

[Huw Davies (＠huw)]

@rbernon, thanks very much for looking at this!

[PATCH 0/1] MR8021: Revert "combase: Find correct apartment in ClientRpcChannelBuffer_SendReceive()." - approved

[Huw Davies (＠huw)]

This merge request was approved by Huw Davies.