[PATCH 2/6] ole32: Allow more functions to use the implicit MTA.
Huw Davies
huw at codeweavers.com
Wed Mar 28 03:43:23 CDT 2018
On Sun, Mar 25, 2018 at 12:33:58PM -0500, Zebediah Figura wrote:
> It's necessary to introduce the being_destroyed field since a destroyed
> apartment may end up releasing proxy objects, which will require calling
> various functions, e.g. CoGetPSClsid(), potentially triggering an infinite
> recursion otherwise.
>
> Signed-off-by: Zebediah Figura <z.figura12 at gmail.com>
> ---
> dlls/ole32/compobj.c | 165 +++++++++++++++++++++++++------------------
> dlls/ole32/compobj_private.h | 1 +
> dlls/ole32/tests/compobj.c | 23 ++++++
> 3 files changed, 120 insertions(+), 69 deletions(-)
>
> diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c
> index cd0e67f..61a8ae6 100644
> --- a/dlls/ole32/compobj.c
> +++ b/dlls/ole32/compobj.c
> @@ -717,6 +717,44 @@ static inline BOOL apartment_is_model(const APARTMENT *apt, DWORD model)
> return (apt->multi_threaded == !(model & COINIT_APARTMENTTHREADED));
> }
>
> +/* gets the multi-threaded apartment if it exists. The caller must
> + * release the reference from the apartment as soon as the apartment pointer
> + * is no longer required. */
> +static APARTMENT *apartment_find_multi_threaded(void)
> +{
> + APARTMENT *result = NULL;
> + struct list *cursor;
> +
> + EnterCriticalSection(&csApartment);
> +
> + LIST_FOR_EACH( cursor, &apts )
> + {
> + struct apartment *apt = LIST_ENTRY( cursor, struct apartment, entry );
> + if (apt->multi_threaded)
> + {
> + result = apt;
> + apartment_addref(result);
> + break;
> + }
> + }
I know this function was copied from below, but there's the MTA variable
(protected by csApartment) that holds the MTA if it exists. So you
could simply return that rather than loop through every apt.
> +
> + LeaveCriticalSection(&csApartment);
> + return result;
> +}
> +
> +/* Return the current apartment if it exists, or, failing that, the MTA. Caller
> + * must free the returned apartment in either case. */
> +static APARTMENT *apartment_get_current_or_mta(void)
> +{
> + APARTMENT *apt = COM_CurrentApt();
> + if (apt)
> + {
> + apartment_addref(apt);
> + return apt;
> + }
> + return apartment_find_multi_threaded();
> +}
So this patch is doing a couple of things. It's taking a ref on the
current apt if one exists or using the MTA if it doesn't. Since this
code is so fragile, I'd like these split. Something like this would
work:
2.1 Add apartment_addref()s / apartment_release()s to CoGetClassObject(),
CoLockObjectExternal(), etc. i.e. implement the 'taking a ref on the
current apt' bit.
2.2 Move apartment_find_multi_threaded() higher up in the file and rewrite
using the MTA variable. Also why not rename this to apartment_find_mta()
2.3 Add apartment_get_current_or_mta() and use it in CoGetClassObject() etc.
Also, I'm not yet 100% convinced about the being_destroyed thing. I'm
wondering whether there's a better way of doing that. Hopefully
splitting this up will help with that.
Huw.
> static void COM_RevokeRegisteredClassObject(RegisteredClass *curClass)
> {
> list_remove(&curClass->entry);
> @@ -1059,8 +1097,7 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoRevokeClassObject(
>
> TRACE("(%08x)\n",dwRegister);
>
> - apt = COM_CurrentApt();
> - if (!apt)
> + if (!(apt = apartment_get_current_or_mta()))
> {
> ERR("COM was not initialized\n");
> return CO_E_NOTINITIALIZED;
> @@ -1091,7 +1128,7 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoRevokeClassObject(
> }
>
> LeaveCriticalSection( &csRegisteredClassList );
> -
> + apartment_release(apt);
> return hr;
> }
>
> @@ -1145,9 +1182,17 @@ DWORD apartment_release(struct apartment *apt)
>
> ret = InterlockedDecrement(&apt->refs);
> TRACE("%s: after = %d\n", wine_dbgstr_longlong(apt->oxid), ret);
> +
> + if (apt->being_destroyed)
> + {
> + LeaveCriticalSection(&csApartment);
> + return ret;
> + }
> +
> /* destruction stuff that needs to happen under csApartment CS */
> if (ret == 0)
> {
> + apt->being_destroyed = TRUE;
> if (apt == MTA) MTA = NULL;
> else if (apt == MainApartment) MainApartment = NULL;
> list_remove(&apt->entry);
> @@ -1295,31 +1340,6 @@ static APARTMENT *apartment_findmain(void)
> return result;
> }
>
> -/* gets the multi-threaded apartment if it exists. The caller must
> - * release the reference from the apartment as soon as the apartment pointer
> - * is no longer required. */
> -static APARTMENT *apartment_find_multi_threaded(void)
> -{
> - APARTMENT *result = NULL;
> - struct list *cursor;
> -
> - EnterCriticalSection(&csApartment);
> -
> - LIST_FOR_EACH( cursor, &apts )
> - {
> - struct apartment *apt = LIST_ENTRY( cursor, struct apartment, entry );
> - if (apt->multi_threaded)
> - {
> - result = apt;
> - apartment_addref(result);
> - break;
> - }
> - }
> -
> - LeaveCriticalSection(&csApartment);
> - return result;
> -}
> -
> /* gets the specified class object by loading the appropriate DLL, if
> * necessary and calls the DllGetClassObject function for the DLL */
> static HRESULT apartment_getclassobject(struct apartment *apt, LPCWSTR dllpath,
> @@ -2059,9 +2079,11 @@ HRESULT WINAPI CoDisconnectObject( LPUNKNOWN lpUnk, DWORD reserved )
> return hr;
> }
>
> - apt = COM_CurrentApt();
> - if (!apt)
> + if (!(apt = apartment_get_current_or_mta()))
> + {
> + ERR("apartment not initialised\n");
> return CO_E_NOTINITIALIZED;
> + }
>
> manager = get_stub_manager_from_object(apt, lpUnk, FALSE);
> if (manager) {
> @@ -2076,6 +2098,7 @@ HRESULT WINAPI CoDisconnectObject( LPUNKNOWN lpUnk, DWORD reserved )
> * not found, making apps think that the object was disconnected, when
> * it actually wasn't */
>
> + apartment_release(apt);
> return S_OK;
> }
>
> @@ -2583,7 +2606,7 @@ HRESULT WINAPI CoGetPSClsid(REFIID riid, CLSID *pclsid)
> static const WCHAR wszInterface[] = {'I','n','t','e','r','f','a','c','e','\\',0};
> static const WCHAR wszPSC[] = {'\\','P','r','o','x','y','S','t','u','b','C','l','s','i','d','3','2',0};
> WCHAR path[ARRAYSIZE(wszInterface) - 1 + CHARS_IN_GUID - 1 + ARRAYSIZE(wszPSC)];
> - APARTMENT *apt = COM_CurrentApt();
> + APARTMENT *apt;
> struct registered_psclsid *registered_psclsid;
> ACTCTX_SECTION_KEYED_DATA data;
> HRESULT hr;
> @@ -2592,11 +2615,12 @@ HRESULT WINAPI CoGetPSClsid(REFIID riid, CLSID *pclsid)
>
> TRACE("() riid=%s, pclsid=%p\n", debugstr_guid(riid), pclsid);
>
> - if (!apt)
> + if (!(apt = apartment_get_current_or_mta()))
> {
> ERR("apartment not initialised\n");
> return CO_E_NOTINITIALIZED;
> }
> + apartment_release(apt);
>
> if (!pclsid)
> return E_INVALIDARG;
> @@ -2667,16 +2691,17 @@ HRESULT WINAPI CoGetPSClsid(REFIID riid, CLSID *pclsid)
> */
> HRESULT WINAPI CoRegisterPSClsid(REFIID riid, REFCLSID rclsid)
> {
> - APARTMENT *apt = COM_CurrentApt();
> + APARTMENT *apt;
> struct registered_psclsid *registered_psclsid;
>
> TRACE("(%s, %s)\n", debugstr_guid(riid), debugstr_guid(rclsid));
>
> - if (!apt)
> + if (!(apt = apartment_get_current_or_mta()))
> {
> ERR("apartment not initialised\n");
> return CO_E_NOTINITIALIZED;
> }
> + apartment_release(apt);
>
> EnterCriticalSection(&cs_registered_psclsid_list);
>
> @@ -2802,11 +2827,10 @@ HRESULT WINAPI CoRegisterClassObject(
> if ( (lpdwRegister==0) || (pUnk==0) )
> return E_INVALIDARG;
>
> - apt = COM_CurrentApt();
> - if (!apt)
> + if (!(apt = apartment_get_current_or_mta()))
> {
> - ERR("COM was not initialized\n");
> - return CO_E_NOTINITIALIZED;
> + ERR("apartment not initialised\n");
> + return CO_E_NOTINITIALIZED;
> }
>
> *lpdwRegister = 0;
> @@ -2826,16 +2850,21 @@ HRESULT WINAPI CoRegisterClassObject(
> if (dwClsContext & CLSCTX_LOCAL_SERVER)
> hr = CoLockObjectExternal(foundObject, TRUE, FALSE);
> IUnknown_Release(foundObject);
> + apartment_release(apt);
> return hr;
> }
> IUnknown_Release(foundObject);
> ERR("object already registered for class %s\n", debugstr_guid(rclsid));
> + apartment_release(apt);
> return CO_E_OBJISREG;
> }
>
> newClass = HeapAlloc(GetProcessHeap(), 0, sizeof(RegisteredClass));
> if ( newClass == NULL )
> + {
> + apartment_release(apt);
> return E_OUTOFMEMORY;
> + }
>
> newClass->classIdentifier = *rclsid;
> newClass->apartment_id = apt->oxid;
> @@ -2864,7 +2893,10 @@ HRESULT WINAPI CoRegisterClassObject(
>
> hr = get_local_server_stream(apt, &marshal_stream);
> if(FAILED(hr))
> + {
> + apartment_release(apt);
> return hr;
> + }
>
> hr = RPC_StartLocalServer(&newClass->classIdentifier,
> marshal_stream,
> @@ -2872,6 +2904,7 @@ HRESULT WINAPI CoRegisterClassObject(
> &newClass->RpcRegistration);
> IStream_Release(marshal_stream);
> }
> + apartment_release(apt);
> return S_OK;
> }
>
> @@ -2989,7 +3022,6 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoGetClassObject(
> IUnknown *regClassObject;
> HRESULT hres = E_UNEXPECTED;
> APARTMENT *apt;
> - BOOL release_apt = FALSE;
>
> TRACE("CLSID: %s,IID: %s\n", debugstr_guid(rclsid), debugstr_guid(iid));
>
> @@ -2998,14 +3030,10 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoGetClassObject(
>
> *ppv = NULL;
>
> - if (!(apt = COM_CurrentApt()))
> + if (!(apt = apartment_get_current_or_mta()))
> {
> - if (!(apt = apartment_find_multi_threaded()))
> - {
> - ERR("apartment not initialised\n");
> - return CO_E_NOTINITIALIZED;
> - }
> - release_apt = TRUE;
> + ERR("apartment not initialised\n");
> + return CO_E_NOTINITIALIZED;
> }
>
> if (pServerInfo) {
> @@ -3017,7 +3045,7 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoGetClassObject(
> {
> if (IsEqualCLSID(rclsid, &CLSID_InProcFreeMarshaler))
> {
> - if (release_apt) apartment_release(apt);
> + apartment_release(apt);
> return FTMarshalCF_Create(iid, ppv);
> }
> if (IsEqualCLSID(rclsid, &CLSID_GlobalOptions))
> @@ -3043,7 +3071,7 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoGetClassObject(
>
> hres = get_inproc_class_object(apt, &clsreg, &comclass->clsid, iid, !(dwClsContext & WINE_CLSCTX_DONT_HOST), ppv);
> ReleaseActCtx(data.hActCtx);
> - if (release_apt) apartment_release(apt);
> + apartment_release(apt);
> return hres;
> }
> }
> @@ -3064,7 +3092,7 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoGetClassObject(
> * is good since we are not returning it in the "out" parameter.
> */
> IUnknown_Release(regClassObject);
> - if (release_apt) apartment_release(apt);
> + apartment_release(apt);
> return hres;
> }
>
> @@ -3099,7 +3127,7 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoGetClassObject(
> * other types */
> if (SUCCEEDED(hres))
> {
> - if (release_apt) apartment_release(apt);
> + apartment_release(apt);
> return hres;
> }
> }
> @@ -3135,11 +3163,11 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoGetClassObject(
> * other types */
> if (SUCCEEDED(hres))
> {
> - if (release_apt) apartment_release(apt);
> + apartment_release(apt);
> return hres;
> }
> }
> - if (release_apt) apartment_release(apt);
> + apartment_release(apt);
>
> /* Next try out of process */
> if (CLSCTX_LOCAL_SERVER & dwClsContext)
> @@ -3298,15 +3326,12 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoCreateInstanceEx(
> if(FAILED(hres))
> clsid = *rclsid;
>
> - if (!(apt = COM_CurrentApt()))
> + if (!(apt = apartment_get_current_or_mta()))
> {
> - if (!(apt = apartment_find_multi_threaded()))
> - {
> - ERR("apartment not initialised\n");
> - return CO_E_NOTINITIALIZED;
> - }
> - apartment_release(apt);
> + ERR("apartment not initialised\n");
> + return CO_E_NOTINITIALIZED;
> }
> + apartment_release(apt);
>
> /*
> * The Standard Global Interface Table (GIT) object is a process-wide singleton.
> @@ -3640,8 +3665,11 @@ HRESULT WINAPI CoLockObjectExternal(
> TRACE("pUnk=%p, fLock=%s, fLastUnlockReleases=%s\n",
> pUnk, fLock ? "TRUE" : "FALSE", fLastUnlockReleases ? "TRUE" : "FALSE");
>
> - apt = COM_CurrentApt();
> - if (!apt) return CO_E_NOTINITIALIZED;
> + if (!(apt = apartment_get_current_or_mta()))
> + {
> + ERR("apartment not initialised\n");
> + return CO_E_NOTINITIALIZED;
> + }
>
> stubmgr = get_stub_manager_from_object(apt, pUnk, fLock);
> if (!stubmgr)
> @@ -3650,6 +3678,7 @@ HRESULT WINAPI CoLockObjectExternal(
> /* Note: native is pretty broken here because it just silently
> * fails, without returning an appropriate error code, making apps
> * think that the object was disconnected, when it actually wasn't */
> + apartment_release(apt);
> return S_OK;
> }
>
> @@ -3659,6 +3688,7 @@ HRESULT WINAPI CoLockObjectExternal(
> stub_manager_ext_release(stubmgr, 1, FALSE, fLastUnlockReleases);
>
> stub_manager_int_release(stubmgr);
> + apartment_release(apt);
> return S_OK;
> }
>
> @@ -4998,22 +5028,19 @@ HRESULT WINAPI CoGetObjectContext(REFIID riid, void **ppv)
> HRESULT WINAPI CoGetContextToken( ULONG_PTR *token )
> {
> struct oletls *info = COM_CurrentInfo();
> + APARTMENT *apt;
>
> TRACE("(%p)\n", token);
>
> if (!info)
> return E_OUTOFMEMORY;
>
> - if (!info->apt)
> + if (!(apt = apartment_get_current_or_mta()))
> {
> - APARTMENT *apt;
> - if (!(apt = apartment_find_multi_threaded()))
> - {
> - ERR("apartment not initialised\n");
> - return CO_E_NOTINITIALIZED;
> - }
> - apartment_release(apt);
> + ERR("apartment not initialised\n");
> + return CO_E_NOTINITIALIZED;
> }
> + apartment_release(apt);
>
> if (!token)
> return E_POINTER;
> diff --git a/dlls/ole32/compobj_private.h b/dlls/ole32/compobj_private.h
> index 9e65c3e..12413f7 100644
> --- a/dlls/ole32/compobj_private.h
> +++ b/dlls/ole32/compobj_private.h
> @@ -142,6 +142,7 @@ struct apartment
> DWORD host_apt_tid; /* thread ID of apartment hosting objects of differing threading model (CS cs) */
> HWND host_apt_hwnd; /* handle to apartment window of host apartment (CS cs) */
> LocalServer *local_server; /* A marshallable object exposing local servers (CS cs) */
> + BOOL being_destroyed; /* is currently being destroyed */
>
> /* FIXME: OIDs should be given out by RPCSS */
> OID oidc; /* object ID counter, starts at 1, zero is invalid OID (CS cs) */
> diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c
> index da4223b..ecfbac8 100644
> --- a/dlls/ole32/tests/compobj.c
> +++ b/dlls/ole32/tests/compobj.c
> @@ -3701,6 +3701,7 @@ static DWORD CALLBACK implicit_mta_proc(void *param)
> IComThreadingInfo *threading_info;
> ULONG_PTR token;
> IUnknown *unk;
> + DWORD cookie;
> CLSID clsid;
> HRESULT hr;
>
> @@ -3721,6 +3722,28 @@ static DWORD CALLBACK implicit_mta_proc(void *param)
> hr = CoGetContextToken(&token);
> ok_ole_success(hr, "CoGetContextToken");
>
> + hr = CoRegisterPSClsid(&IID_IWineTest, &CLSID_WineTestPSFactoryBuffer);
> + ok_ole_success(hr, "CoRegisterPSClsid");
> +
> + hr = CoGetPSClsid(&IID_IClassFactory, &clsid);
> + ok_ole_success(hr, "CoGetPSClsid");
> +
> + hr = CoRegisterClassObject(&CLSID_WineOOPTest, (IUnknown *)&Test_ClassFactory,
> + CLSCTX_INPROC_SERVER, REGCLS_SINGLEUSE, &cookie);
> + ok_ole_success(hr, "CoRegisterClassObject");
> +
> + hr = CoRevokeClassObject(cookie);
> + ok_ole_success(hr, "CoRevokeClassObject");
> +
> + hr = CoRegisterMessageFilter(NULL, NULL);
> + ok(hr == CO_E_NOT_SUPPORTED, "got %#x\n", hr);
> +
> + hr = CoLockObjectExternal((IUnknown *)&Test_Unknown, TRUE, TRUE);
> + ok_ole_success(hr, "CoLockObjectExternal");
> +
> + hr = CoDisconnectObject((IUnknown *)&Test_Unknown, 0);
> + ok_ole_success(hr, "CoDisconnectObject");
> +
> return 0;
> }
>
> --
> 2.7.4
>
>
>
More information about the wine-devel
mailing list