[PATCH v4 0/2] 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`. -- v4: combase: Generate apartment OXID in rpcss ole32/tests: Test that re-creating an apartment results in a new OXID https://gitlab.winehq.org/wine/wine/-/merge_requests/2059
From: Aaron Hill <aa1ronham(a)gmail.com> Wine currently re-uses the original OXID when an apartment is re-created, so this is marked with `todo_wine` for now. --- dlls/ole32/tests/marshal.c | 57 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/dlls/ole32/tests/marshal.c b/dlls/ole32/tests/marshal.c index 18f47a5c64c..499f1a91938 100644 --- a/dlls/ole32/tests/marshal.c +++ b/dlls/ole32/tests/marshal.c @@ -4546,6 +4546,61 @@ 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); + + hr = IStream_Seek(pStream, ullZero, STREAM_SEEK_SET, NULL); + ok_ole_success(hr, IStream_Seek); + + hr = CoReleaseMarshalData(pStream); + ok_ole_success(hr, CoReleaseMarshalData); + + 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(DWORD flags) +{ + OXID first_oxid; + OXID second_oxid; + + cLocks = 0; + external_connections = 0; + + CoUninitialize(); + + CoInitializeEx(NULL, flags); + first_oxid = get_apartment_oxid(); + CoUninitialize(); + + CoInitializeEx(NULL, flags); + second_oxid = get_apartment_oxid(); + CoUninitialize(); + + todo_wine ok(first_oxid != second_oxid, "Re-created apartment with flags %ld has old OXID: %s\n", flags, wine_dbgstr_longlong(first_oxid)); +} + START_TEST(marshal) { HMODULE hOle32 = GetModuleHandleA("ole32"); @@ -4567,6 +4622,8 @@ START_TEST(marshal) register_test_window(); test_cocreateinstance_proxy(); + test_apartment_oxid(COINIT_APARTMENTTHREADED); + test_apartment_oxid(COINIT_MULTITHREADED); test_implicit_mta(); test_mta_creation_thread_change_apartment(); -- GitLab 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 | 23 ++++++++++++----------- dlls/combase/combase_private.h | 1 + dlls/combase/rpc.c | 7 +++++++ dlls/ole32/tests/marshal.c | 2 +- include/wine/irpcss.idl | 4 ++++ programs/rpcss/rpcss_main.c | 22 ++++++++++++++++++++++ 6 files changed, 47 insertions(+), 12 deletions(-) diff --git a/dlls/combase/apartment.c b/dlls/combase/apartment.c index b951486ee82..dee4faccaa0 100644 --- a/dlls/combase/apartment.c +++ b/dlls/combase/apartment.c @@ -363,6 +363,7 @@ HRESULT apartment_get_local_server_stream(struct apartment *apt, IStream **ret) /* Creates new apartment for given model */ static struct apartment *apartment_construct(DWORD model) { + HRESULT hr = S_OK; struct apartment *apt; TRACE("creating new apartment, model %ld\n", model); @@ -370,6 +371,17 @@ static struct apartment *apartment_construct(DWORD model) apt = calloc(1, sizeof(*apt)); apt->tid = 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. */ + hr = rpcss_get_new_apartment_oxid(&apt->oxid); + if (FAILED(hr)) + { + ERR("rpcss_get_new_apartment_oxid failed, hr %#lx", hr); + return NULL; + } + list_init(&apt->proxies); list_init(&apt->stubmgrs); list_init(&apt->loaded_dlls); @@ -383,17 +395,6 @@ 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(); - } - TRACE("Created apartment on OXID %s\n", wine_dbgstr_longlong(apt->oxid)); list_add_head(&apts, &apt->entry); 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..9082171d02c 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(), 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 499f1a91938..4ffc0a5d878 100644 --- a/dlls/ole32/tests/marshal.c +++ b/dlls/ole32/tests/marshal.c @@ -4598,7 +4598,7 @@ static void test_apartment_oxid(DWORD flags) second_oxid = get_apartment_oxid(); CoUninitialize(); - todo_wine ok(first_oxid != second_oxid, "Re-created apartment with flags %ld has old OXID: %s\n", flags, wine_dbgstr_longlong(first_oxid)); + ok(first_oxid != second_oxid, "Re-created apartment with flags %ld has old OXID: %s\n", flags, wine_dbgstr_longlong(first_oxid)); } START_TEST(marshal) diff --git a/include/wine/irpcss.idl b/include/wine/irpcss.idl index 5d27658eba5..8456b34efda 100644 --- a/include/wine/irpcss.idl +++ b/include/wine/irpcss.idl @@ -47,4 +47,8 @@ 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, + [out] OXID *oxid); } diff --git a/programs/rpcss/rpcss_main.c b/programs/rpcss/rpcss_main.c index dfe5ced7c07..7888fae5515 100644 --- a/programs/rpcss/rpcss_main.c +++ b/programs/rpcss/rpcss_main.c @@ -144,6 +144,28 @@ 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, OXID *oxid) +{ + HRESULT status; + ULONG oxid_upper_bits; + static LONG oxid_lower_bits; + + status = I_RpcBindingInqLocalClientPID(h, &oxid_upper_bits); + if (status != S_OK) + { + ERR("Failed to create apartment oxid: %ld", status); + return status; + } + + /* Combine the caller PID and an incrementing counter to create a unique OXID */ + *oxid = (OXID) oxid_upper_bits << 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
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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=129027 Your paranoid android. === debian11 (build log) === WineRunWineTest.pl:error: The task timed out === debian11b (build log) === Task: Could not create the wow32 wineprefix: Failed to disable the crash dialogs: Task: WineTest did not produce the wow32 report
@huw I've changed the first commit message, and addressed your latest round of comments. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2059#note_22763
The pipeline failed - it looks like we're trying to create an apartment before rpcss has started, resulting in a failure. I'm investigating further. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2059#note_22766
participants (3)
-
Aaron Hill -
Aaron Hill (＠Aaron1011)
-
Marvin