[PATCH 0/1] MR2059: combase: Generate apartment OXID in rpcss
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. This patch uses `rpcss` to generate a new OXID. The current process id is combined with an incrementing counter stored in rpcss. This ensures that re-creating an apartment will not re-use an OXID. 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 avoiding re-using OXIDs across re-created apartments, we ensure that the proxy side will never incorrectly re-use a stale `proxy_manager`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2059
From: Aaron Hill <aa1ronham(a)gmail.com> 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. This patch uses `rpcss` to generate a new OXID. The current process id is combined with an incrementing counter stored in rpcss. This ensures that re-creating an apartment will not re-use an OXID. 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 avoiding re-using OXIDs across re-created apartments, we ensure that the proxy side will never incorrectly re-use a stale `proxy_manager`. --- dlls/combase/apartment.c | 15 ++++------- dlls/combase/combase_private.h | 1 + dlls/combase/rpc.c | 7 +++++ dlls/ole32/tests/marshal.c | 49 ++++++++++++++++++++++++++++++++++ include/wine/irpcss.idl | 5 ++++ programs/rpcss/rpcss_main.c | 11 ++++++++ 6 files changed, 78 insertions(+), 10 deletions(-) diff --git a/dlls/combase/apartment.c b/dlls/combase/apartment.c index b951486ee82..f44ce735729 100644 --- a/dlls/combase/apartment.c +++ b/dlls/combase/apartment.c @@ -383,16 +383,11 @@ static struct apartment *apartment_construct(DWORD model) 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(); - } + /* Get a new OXID to use for this apartment. Note that re-creating + * an apartment (by calling CoUninitialize and then CoInitializeEx) + * will result in a new OXID. This ensures that we don't re-use a + * proxy_manager` that refers to a dead apartment. */ + rpcss_get_new_apartment_oxid(&apt->oxid); TRACE("Created apartment on OXID %s\n", wine_dbgstr_longlong(apt->oxid)); diff --git a/dlls/combase/combase_private.h b/dlls/combase/combase_private.h index 19e3def0b4e..fd53d997e8d 100644 --- a/dlls/combase/combase_private.h +++ b/dlls/combase/combase_private.h @@ -118,6 +118,7 @@ HRESULT apartment_disconnectproxies(struct apartment *apt) DECLSPEC_HIDDEN; /* RpcSs interface */ HRESULT rpcss_get_next_seqid(DWORD *id) DECLSPEC_HIDDEN; +HRESULT rpcss_get_new_apartment_oxid(OXID *oxid) DECLSPEC_HIDDEN; HRESULT rpc_get_local_class_object(REFCLSID rclsid, REFIID riid, void **obj) DECLSPEC_HIDDEN; HRESULT rpc_register_local_server(REFCLSID clsid, IStream *stream, DWORD flags, unsigned int *cookie) DECLSPEC_HIDDEN; HRESULT rpc_revoke_local_server(unsigned int cookie) DECLSPEC_HIDDEN; diff --git a/dlls/combase/rpc.c b/dlls/combase/rpc.c index c51b59de4bf..854d5635451 100644 --- a/dlls/combase/rpc.c +++ b/dlls/combase/rpc.c @@ -301,6 +301,13 @@ static RPC_BINDING_HANDLE get_irot_handle(void) } \ return hr; +HRESULT rpcss_get_new_apartment_oxid(OXID *oxid) +{ + RPCSS_CALL_START + hr = irpcss_get_new_apartment_oxid(get_irpcss_handle(), GetCurrentProcessId(), oxid); + RPCSS_CALL_END +} + HRESULT rpcss_get_next_seqid(DWORD *id) { RPCSS_CALL_START diff --git a/dlls/ole32/tests/marshal.c b/dlls/ole32/tests/marshal.c index 18f47a5c64c..1b8dce5616e 100644 --- a/dlls/ole32/tests/marshal.c +++ b/dlls/ole32/tests/marshal.c @@ -4546,6 +4546,54 @@ static void test_mta_creation_thread_change_apartment(void) CoUninitialize(); } +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)); +} + START_TEST(marshal) { HMODULE hOle32 = GetModuleHandleA("ole32"); @@ -4568,6 +4616,7 @@ START_TEST(marshal) test_cocreateinstance_proxy(); test_implicit_mta(); + test_apartment_oxid(); test_mta_creation_thread_change_apartment(); CoInitializeEx(NULL, COINIT_APARTMENTTHREADED); diff --git a/include/wine/irpcss.idl b/include/wine/irpcss.idl index 5d27658eba5..b851d583c43 100644 --- a/include/wine/irpcss.idl +++ b/include/wine/irpcss.idl @@ -47,4 +47,9 @@ interface Irpcss HRESULT irpcss_get_thread_seq_id( [in] handle_t handle, [out] DWORD *sequence_id); + + HRESULT irpcss_get_new_apartment_oxid( + [in] handle_t handle, + [in] DWORD process_id, + [out] OXID *oxid); } diff --git a/programs/rpcss/rpcss_main.c b/programs/rpcss/rpcss_main.c index dfe5ced7c07..92e5b65d8f4 100644 --- a/programs/rpcss/rpcss_main.c +++ b/programs/rpcss/rpcss_main.c @@ -144,6 +144,17 @@ HRESULT __cdecl irpcss_get_thread_seq_id(handle_t h, DWORD *id) return S_OK; } +/* Generates a new OXID suitable for use by an apartment. + * Each call to this function returns a distinct OXID. + * This function should not be invoked directly - use rpcss_get_new_apartment_oxid + * instead. */ +HRESULT __cdecl irpcss_get_new_apartment_oxid(handle_t h, DWORD process_id, OXID *oxid) +{ + static LONG oxid_lower_bits; + *oxid = (OXID)process_id << 32 | InterlockedIncrement(&oxid_lower_bits); + return S_OK; +} + static RPC_STATUS RPCSS_Initialize(void) { static unsigned short irot_protseq[] = IROT_PROTSEQ; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2059
Huw Davies (@huw) commented about dlls/ole32/tests/marshal.c:
+ 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)); +} + I think it would make sense to also test the `COINT_MULTITHREADED` case, perhaps by calling this function twice with a `flags` parameter. It would also make sense to split the tests off to a separate commit, before the implementation change, with appropriate `todo_wine`s which would get removed in the second commit.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2059#note_22304
Huw Davies (@huw) commented about programs/rpcss/rpcss_main.c:
return S_OK; }
+/* Generates a new OXID suitable for use by an apartment. + * Each call to this function returns a distinct OXID. + * This function should not be invoked directly - use rpcss_get_new_apartment_oxid + * instead. */ +HRESULT __cdecl irpcss_get_new_apartment_oxid(handle_t h, DWORD process_id, OXID *oxid) +{ + static LONG oxid_lower_bits; + *oxid = (OXID)process_id << 32 | InterlockedIncrement(&oxid_lower_bits);
You can obtain the client's process id by calling `I_RpcBindingInqLocalClientPID()`, passing it the handle `h`, so there shouldn't be a need to explicitly pass the process id to the server. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2059#note_22305
Huw Davies (@huw) commented about dlls/ole32/tests/marshal.c:
+ 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); + You need to reset the stream to the start and call `CoReleaseMarshalData()`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2059#note_22306
participants (3)
-
Aaron Hill -
Aaron Hill (＠Aaron1011)
-
Huw Davies (＠huw)