[PATCH] combase: Use RtlGenRandom to generate apartment OXID

21 Mar 2022

Currently, apartment OXIDs are generated using the process ID
(and the thread ID for a single threaded-apartment). This means
that if an apartment is destroyed and re-created (using
`CoUninitialize` followed by `CoInitializeEx`), the newly created
apartment will end up with the same OXID as the old apartment.

However, testing shows that Windows will generate a *new* OXID
when an apartment is created in the above manner. Using
RtlGenRandom makes our behavior more consistent with Windows -
however, the actual OXIDs we generate will still differ
from those of Windows.

Additionally, this fixes an issue that caused the .NET 4.8
installer to become stuck during the downloading stage under Wine.
The installer appears to perform the following actions:

1. Call `IBackgroundCopyJob_SetNotify` interface on a BITS
   job. This causes us to create a proxy (in the other process
   hosting 'qmgr') for the `IBackgroundCopyCallback` pointer
   passed as a parameter.
2. Trigger MTA apartment re-creation (in the process running the setup,
   *not* the process with the `IBackgroundCopyCallback` proxy)
   through `CoUninitialize` followed by `CoInitializeEx`.
3. Call `IBackgroundCopyJob_SetNotify` on a newly created job,
   but with the same `IBackgroundCopyCallback` pointer parameter.

When we deserialize the pointer passed to
`IBackgroundCopyJob_SetNotify`, we will end up re-using the same
`proxy_manager` that we created for the previous `IBackgroundCopyCallback`.
This is due to the fact that the OIDs happen to match (due to the fact that
the .NET 4.8 setup appears to perform actions in the same order between
the old and new apartments), and the apartment OXIDs match as explained above.
above. As a result, we will use the old IPID when we send RPC packets
using this `proxy_manager`. However, the new and old IPIDs will *never* match,
since their generation process includes `RtlGenRandom`. This will cause a fault
packet to be generated on the listening side of the RPC connection.

By using RtlGenRandom when we generate the apartment OXID, we ensure that
the proxy side will never incorrectly re-use a stale `proxy_manager`

Signed-off-by: Aaron Hill <aa1ronham(a)gmail.com>
---
 dlls/combase/apartment.c   | 18 ++++++--------
 dlls/ole32/tests/marshal.c | 49 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/dlls/combase/apartment.c b/dlls/combase/apartment.c
index c1b381879d3..d4d78264dea 100644
--- a/dlls/combase/apartment.c
+++ b/dlls/combase/apartment.c
@@ -32,6 +32,7 @@
 #include "windef.h"
 #include "winbase.h"
 #include "servprov.h"
+#include "ntsecapi.h"
 
 #include "combase_private.h"
 
@@ -382,17 +383,12 @@ static struct apartment *apartment_construct(DWORD model)
     apt->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": apartment");
 
     apt->multi_threaded = !(model & COINIT_APARTMENTTHREADED);
-
-    if (apt->multi_threaded)
-    {
-        /* FIXME: should be randomly generated by in an RPC call to rpcss */
-        apt->oxid = ((OXID)GetCurrentProcessId() << 32) | 0xcafe;
-    }
-    else
-    {
-        /* FIXME: should be randomly generated by in an RPC call to rpcss */
-        apt->oxid = ((OXID)GetCurrentProcessId() << 32) | GetCurrentThreadId();
-    }
+    /* FIXME: should be generated by in an RPC call to rpcss
+     * Re-creating an apartment (via `CoUninitialize` followed by `CoInitializeEx`)
+     * will result in a *different* oxid. This is consistent with the behavior of
+     * Windows, and ensures that proxies in a different process will create
+     * a new RPC connection, instead of attempting to re-use an old one.  */
+    RtlGenRandom(&apt->oxid, sizeof(apt->oxid));
 
     TRACE("Created apartment on OXID %s\n", wine_dbgstr_longlong(apt->oxid));
 
diff --git a/dlls/ole32/tests/marshal.c b/dlls/ole32/tests/marshal.c
index 6e21ed1889b..0f8a7a643de 100644
--- a/dlls/ole32/tests/marshal.c
+++ b/dlls/ole32/tests/marshal.c
@@ -1239,6 +1239,54 @@ static void test_marshal_proxy_apartment_shutdown(void)
     CoInitializeEx(NULL, COINIT_APARTMENTTHREADED);
 }
 
+static OXID get_apartment_oxid(void)
+{
+    HRESULT hr;
+    OBJREF objref;
+    DWORD size, read;
+    IStream *pStream = NULL;
+
+    hr = CreateStreamOnHGlobal(NULL, TRUE, &pStream);
+    ok_ole_success(hr, CreateStreamOnHGlobal);
+
+    hr = CoMarshalInterface(pStream, &IID_IClassFactory, (IUnknown*)&Test_ClassFactory, MSHCTX_INPROC, NULL, MSHLFLAGS_NORMAL);
+    ok_ole_success(hr, CoMarshalInterface);
+
+    hr = IStream_Seek(pStream, ullZero, STREAM_SEEK_SET, NULL);
+    ok_ole_success(hr, IStream_Seek);
+
+    size = FIELD_OFFSET(OBJREF, u_objref.u_standard.saResAddr);
+    hr = IStream_Read(pStream, &objref, size, &read);
+    ok_ole_success(hr, IStream_Read);
+
+    IStream_Release(pStream);
+
+    return objref.u_objref.u_standard.std.oxid;
+}
+
+/* tests that re-creating an apartment (via `CoUninitialize` followed by `CoInitializeEx`)
+ * results in an apartment with a different OXID than the previous apartment
+ */
+static void test_apartment_oxid(void)
+{
+
+    OXID first_oxid;
+    OXID second_oxid;
+
+    cLocks = 0;
+    external_connections = 0;
+
+    CoInitializeEx(NULL, COINIT_APARTMENTTHREADED);
+    first_oxid = get_apartment_oxid();
+    CoUninitialize();
+
+    CoInitializeEx(NULL, COINIT_APARTMENTTHREADED);
+    second_oxid = get_apartment_oxid();
+    CoUninitialize();
+
+    ok(first_oxid != second_oxid, "Re-created apartment has old OXID: %s\n", wine_dbgstr_longlong(first_oxid));
+}
+
 /* tests that proxies are released when the containing mta apartment is destroyed */
 static void test_marshal_proxy_mta_apartment_shutdown(void)
 {
@@ -4511,6 +4559,7 @@ START_TEST(marshal)
 
     test_cocreateinstance_proxy();
     test_implicit_mta();
+    test_apartment_oxid();
 
     CoInitializeEx(NULL, COINIT_APARTMENTTHREADED);
 
-- 
2.35.1

    

Aaron Hill

Huw Davies

tags

participants (2)