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

[Dmitry Timoshkov (＠dmitry)]

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

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

[Dmitry Timoshkov]

From: Dmitry Timoshkov <dmitry(a)baikal.ru>

This reverts commit 68a9046e30dd5ea8ab9fc753c728211751694b32.

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

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru>
---
 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;
     }
 
-- 
GitLab

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

[Rémi Bernon]

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.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8021#note_105745

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

[Rémi Bernon]

This merge request was approved by Rémi Bernon.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8021

[Huw Davies (＠huw)]

@rbernon, thanks very much for looking at this!

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8021#note_105752

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

[Huw Davies (＠huw)]

This merge request was approved by Huw Davies.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8021