From df558f592b3f473ffd3d595ec981124853e3c146 Mon Sep 17 00:00:00 2001 From: Luca Bacci Date: Tue, 10 Mar 2026 16:36:18 +0100 Subject: [PATCH] WICFactory: Make thread-safe This fixes two issues: 1. A global IWICImagingFactory interface pointer was used directly from any apartment without marshaling. This goes against the rules of COM, and for all we know could lead to threading issues (perhaps it's not a problem in practice, but can't tell). 2. The COM apartment where the IWICImagingFactory is created could be finalized by the user. We take a reference on the STA but the thread could terminate. If the thread is on the MTA we don't take a reference at all. In such case the interface pointer would not be valid anymore (per the COM rules), but most importantly the WIC DLL is unloaded (this actually happens). To avoid that we create a IWICImagingFactory transiently each time we need it. We could cache the interface pointer using something like IInitializeSpy or the COM static store [1]. However performance is still great and we'll likely drop use of WIC soon (see MR !607). References: 1. The COM static store, part 1: Introduction https://devblogs.microsoft.com/oldnewthing/20210208-00/?p=104812 --- src/win32/cairo-dwrite-font.cpp | 41 +++++++---- src/win32/cairo-win32-private.h | 6 ++ src/win32/cairo-win32-system.c | 116 +++++++++++++++++++++++++++++++- 3 files changed, 149 insertions(+), 14 deletions(-) diff --git a/src/win32/cairo-dwrite-font.cpp b/src/win32/cairo-dwrite-font.cpp index 24988cfc7..f9db37bbf 100644 --- a/src/win32/cairo-dwrite-font.cpp +++ b/src/win32/cairo-dwrite-font.cpp @@ -168,17 +168,35 @@ class WICImagingFactory public: static RefPtr Instance() { - if (!mFactoryInstance) { - CoInitialize(NULL); - CoCreateInstance(CLSID_WICImagingFactory, - NULL, - CLSCTX_INPROC_SERVER, - IID_PPV_ARGS(&mFactoryInstance)); - } - return mFactoryInstance; + HRESULT hr; + + /* WIC is based on true COM, so we need to set this thread + * on a COM apartment. Usually one calls CoInitialize and + * call it a day, however this is used on threads we don't + * own. We can use whatever apartment the user has already + * initialized, but if there's no apartment we don't want + * to force the thread to a specific apartment type. Turns + * out implicit MTA is perfect for this; we have to take + * a reference on the MTA however, otherwise it can disappear + * at any time in the middle of our operations. + * + * Note: WICImagingFactory has threading model 'both', so + * the object will be accessed directly (no marshaling) + * regardless of the apartment type. + */ + cairo_win32_ensure_mta (); + + IWICImagingFactory *wic_factory; + hr = CoCreateInstance (CLSID_WICImagingFactory, + NULL, + CLSCTX_INPROC_SERVER, + IID_PPV_ARGS (&wic_factory)); + if (FAILED (hr)) { + assert (0 && "CoCreateInstance (CLSID_WICImagingFactory) failed"); + } + + return wic_factory; } -private: - static RefPtr mFactoryInstance; }; cairo_atomic_once_t DWriteFactory::mOnceFactories = CAIRO_ATOMIC_ONCE_INIT; @@ -188,9 +206,6 @@ RefPtr DWriteFactory::mFactoryInstance2; RefPtr DWriteFactory::mFactoryInstance3; RefPtr DWriteFactory::mFactoryInstance4; RefPtr DWriteFactory::mFactoryInstance8; - -RefPtr WICImagingFactory::mFactoryInstance; - cairo_atomic_once_t DWriteFactory::mOnceSystemCollection = CAIRO_ATOMIC_ONCE_INIT; RefPtr DWriteFactory::mSystemCollection; diff --git a/src/win32/cairo-win32-private.h b/src/win32/cairo-win32-private.h index 692da0de4..7596e4e64 100644 --- a/src/win32/cairo-win32-private.h +++ b/src/win32/cairo-win32-private.h @@ -181,6 +181,9 @@ _cairo_win32_gdi_compositor_get (void); cairo_status_t _cairo_win32_print_api_error (const char *context, const char *api); +cairo_status_t +_cairo_win32_api_error_fatal (const char *format, ...); + cairo_bool_t _cairo_surface_is_win32 (const cairo_surface_t *surface); @@ -239,6 +242,9 @@ cairo_win32_get_system_text_quality (void); HMODULE _cairo_win32_load_library_from_system32 (const wchar_t *name); +void +cairo_win32_ensure_mta (void); + typedef DWORD (__stdcall *stdcall_free_func_t) (void *); void diff --git a/src/win32/cairo-win32-system.c b/src/win32/cairo-win32-system.c index 9397b5a9f..8f24150bd 100644 --- a/src/win32/cairo-win32-system.c +++ b/src/win32/cairo-win32-system.c @@ -50,6 +50,27 @@ #include +#include + +typedef HRESULT (__stdcall *pCoIncrementMTAUsage_t) (CO_MTA_USAGE_COOKIE*); +typedef HRESULT (__stdcall *pCoDecrementMTAUsage_t) (CO_MTA_USAGE_COOKIE); + +static struct { + cairo_atomic_once_t once; + + struct { + CO_MTA_USAGE_COOKIE cookie; + bool cookie_is_set; + pCoDecrementMTAUsage_t pCoDecrementMTAUsage; + } mta_usage; + HANDLE thread; +} mta = +{ + CAIRO_ATOMIC_ONCE_INIT, + { 0, false, NULL }, + NULL, +}; + /** * _cairo_win32_print_api_error: * @context: context string to display along with the error @@ -109,6 +130,99 @@ _cairo_win32_load_library_from_system32 (const wchar_t *name) return module_handle; } +static DWORD __stdcall +mta_thread_main (void *user_data) +{ + HRESULT hr; + + hr = CoInitializeEx (NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); + assert (SUCCEEDED (hr)); + + HANDLE event = (HANDLE) user_data; + if (!SignalObjectAndWait (event, GetCurrentProcess (), INFINITE, FALSE)) { + assert (0 && "SignalObjectAndWait failed"); + } + + return 0; +} + +/** + * cairo_win32_ensure_mta: + * + * Ensures that the MTA is initialized and keeps running in this + * process. Helps for COM usage on threads that we don't own, + * since we don't have to call CoInitializeEx. + **/ +void +cairo_win32_ensure_mta (void) +{ + if (_cairo_atomic_init_once_enter (&mta.once)) { + HMODULE ole32 = _cairo_win32_load_library_from_system32 (L"OLE32.DLL"); + + /* Windows 8+ */ + if (ole32) { + pCoIncrementMTAUsage_t pCoIncrementMTAUsage = + (pCoIncrementMTAUsage_t) GetProcAddress (ole32, "CoIncrementMTAUsage"); + pCoDecrementMTAUsage_t pCoDecrementMTAUsage = + (pCoDecrementMTAUsage_t) GetProcAddress (ole32, "CoDecrementMTAUsage"); + + if (pCoIncrementMTAUsage && pCoDecrementMTAUsage && + SUCCEEDED (pCoIncrementMTAUsage (&mta.mta_usage.cookie))) + { + mta.mta_usage.cookie_is_set = true; + mta.mta_usage.pCoDecrementMTAUsage = pCoDecrementMTAUsage; + } + } + + /* Downlevel support for Windows 7 */ + if (!mta.mta_usage.cookie_is_set) { + HANDLE event = CreateEvent (NULL, TRUE, FALSE, NULL); + if (!event) { + assert (0 && "CreateEvent failed"); + } + + /* Since the UCRT _beginthreadex takes a reference on the "calling + * HMODULE", which makes Cairo unloadable. Use CreateThread. + */ + mta.thread = CreateThread (NULL, 0, mta_thread_main, event, 0, NULL); + if (!mta.thread) { + assert (0 && "_beginthreadex failed"); + } + + DWORD ret = WaitForSingleObject (event, INFINITE); + if (ret != WAIT_OBJECT_0) { + assert (0 && "WaitForSingleObject failed"); + } + + CloseHandle (event); + } + + _cairo_atomic_init_once_leave (&mta.once); + } +} + +static void +cairo_win32_mta_finalize (void) +{ + /* Loader-lock-safe */ + + if (_cairo_atomic_init_once_check (&mta.once)) { + if (mta.mta_usage.cookie_is_set) { + void *free_func = mta.mta_usage.pCoDecrementMTAUsage; + cairo_win32_async_stdcall_free (free_func, mta.mta_usage.cookie); + } + else if (mta.thread) { + /* Yeah, TerminateThread is generally unsafe. however, this is synchronized + * with entering of kernel-mode (SignalObjectAndWait) and thus is completely + * safe. Note also that TerminateThread is asynchronous, so it can be used + * from DllMain. + */ + TerminateThread (mta.thread, 0); + CloseHandle (mta.thread); + } + } +} + void cairo_win32_async_stdcall_free (stdcall_free_func_t func, void *data) { @@ -135,8 +249,8 @@ static void cairo_win32_finalize (void) { cairo_win32_dwrite_finalize (); - cairo_win32_thread_data_finalize (); + cairo_win32_mta_finalize (); CAIRO_MUTEX_FINALIZE (); }