@@ -396,6 +396,94 @@ void CPimcManager::TerminateHookThread(__inout CHookThreadItem * pThread)
396396void CPimcManager::FinalRelease ()
397397{
398398 m_wispManagerLock.RevokeIfValid ();
399+
400+ //
401+ // The CComObject<CPimcManager> destructor is the only function which calls into this
402+ // FinalRelease code.
403+ //
404+ // In all successful usage of CPimcManager: 1) Managed WPF code uses CoCreateInstance
405+ // to acquire an IPimcManager2 interface to a brand-new CPimcManager instance (created by
406+ // the ATL CComCreator<T>::CreateInstance machinery), meaning FinalConstruct by-definition
407+ // completes successfully, meaning "m_managerLock" is therefore guaranteed to be locked;
408+ // 2) Managed WPF code then runs through its full end-to-end usage of the CPimcManager
409+ // object (generally managed by the code in PenThreadWorker.cs); 3) When/if the managed WPF
410+ // code determines that the CPimcManager object is no longer needed, it sends a
411+ // RELEASE_MANAGER_EXT message (see UnsafeNativeMethods.ReleaseManagerExternalLock()) which
412+ // unlocks "m_managerLock"; 4) Now that it is unlocked, the CComObject<CPimcManager> object
413+ // can be destroyed when/if its refcount drops to zero, and this FinalRelease function will
414+ // run at that time.
415+ //
416+ // So in all successful usage cases, it is guaranteed that "m_managerLock" is already
417+ // unlocked when this code runs (because if it was still locked, the lock itself would have
418+ // prevented the refcount from reaching zero, and would have prevented this function from
419+ // ever running).
420+ //
421+ // That said, in unsuccessful usage cases, the ATL CComCreator<T>::CreateInstance machinery
422+ // can fail, meaning it will destroy the brand-new CPimcManager instance before returning
423+ // an error back to the CreateInstance caller. Destroying the brand-new instance triggers
424+ // the CComObject<CPimcManager> destructor and therefore calls into this function during
425+ // the CComCreator<T>::CreateInstance operation itself.
426+ //
427+ // The final step in CComCreator<T>::CreateInstance is a QI which queries the newly-created
428+ // object for whatever interface has been requested by the caller. This operation is the
429+ // main way that CComCreator<T>::CreateInstance can fail. For example, this QI is
430+ // guaranteed to fail whenever the CoCreateInstance caller targets the CPimcManager CLSID
431+ // but passes in a "random" IID that has nothing to do with IPimcManager2 or anything else
432+ // that CPimcManager implements.
433+ //
434+ // (In CPimcManager construction, outside of pathological cases (e.g., where a small heap
435+ // allocation in OS code fails due to out-of-memory), there are no other known ways that
436+ // the CComCreator<T>::CreateInstance sequence can fail; so the QI failure is the only
437+ // failure mode that is known to be of general interest.)
438+ //
439+ // The QI failure can only occur after the preceding FinalConstruct call has completed
440+ // successfully (since any FinalConstruct failure would have caused
441+ // CComCreator<T>::CreateInstance to abort without ever trying the QI); since
442+ // CPimcManager::FinalConstruct always locks the "m_managerLock", this implies that the
443+ // "m_managerLock" is guaranteed to be locked when this code runs (which is exactly
444+ // opposite to what happens in all successful usage cases as discussed above).
445+ //
446+ // In this case, it is crucial to unlock "m_managerLock" before allowing this CPimcManager
447+ // object to be destroyed. Without the unlock, this CPimcManager object would be destroyed
448+ // while the associated CStdIdentity in the OS code still holds a reference to it; during
449+ // any future apartment unload, the OS code would release this reference, and the release
450+ // would be a use-after-free at that point.
451+ //
452+ // Note that the crucial unlock causes overactive ATL debug asserts to fire if a chk build
453+ // of this DLL is used; specifically:
454+ //
455+ // - The code in the CComObject<CPimcManager> destructor always stomps the refcount to
456+ // 0xc0000001 (i.e., "-(LONG_MAX/2)"), meaning this CPimcManager object's refcount is
457+ // always 0xc0000001 when this code runs; unlocking "m_managerLock" will cause the refcount
458+ // to drop by one (because, as discussed above, the crucial operation which prevents
459+ // use-after-free problems will release the associated CStdIdentity's reference to this
460+ // CPimcManager object, and in this way releases the reference that was added when
461+ // "managerLock" was locked during FinalConstruct); as a result, unlocking "m_managerLock"
462+ // will move this CPimcManager object's refcount through a "0xc0000001 -> 0xc0000000"
463+ // transition.
464+ //
465+ // - Both of the CComObjectRootEx<T>::InternalRelease specializations contain debug asserts
466+ // which will fire whenever the refcount drops below 0xc0000001, so this transition always
467+ // triggers a debug assert when using a chk build of this DLL.
468+ //
469+ // - That said, all evidence strongly suggests that this is just an overactive assert in
470+ // the ATL code (probably just indicating that it is rare for FinalConstruct to add
471+ // "self-references" like it does for CPimcManager (since these self-references generally
472+ // prevent the server object from being destroyed unless a manual action like the
473+ // RELEASE_MANAGER_EXT message is taken later on), meaning it is rare to have a situation
474+ // where FinalRelease needs to release self-references that were acquired in
475+ // FinalConstruct, meaning this is a rare enough case that the ATL authors either didn't
476+ // test it or didn't think it was common enough to warrant adjusting the assert).
477+ //
478+ // Since this change is being made in servicing, attempt to change behavior as little as
479+ // possible in the "successful usage" cases where "m_managerLock" is already unlocked,
480+ // while still ensuring that FinalRelease will always run the crucial unlock in all
481+ // "unsuccessful usage" cases.
482+ //
483+ if (m_managerLock.HasNotBeenUnlocked ())
484+ {
485+ m_managerLock.Unlock ();
486+ }
399487}
400488
401489// ///////////////////////////////////////////////////////////////////////////
0 commit comments