diff --git a/NuGet.config b/NuGet.config
index 1046209f283..3b5a8795819 100644
--- a/NuGet.config
+++ b/NuGet.config
@@ -5,8 +5,10 @@
+
+
@@ -20,8 +22,10 @@
+
+
diff --git a/eng/Version.Details.xml b/eng/Version.Details.xml
index 29c2797195c..598498fa119 100644
--- a/eng/Version.Details.xml
+++ b/eng/Version.Details.xml
@@ -1,13 +1,13 @@
-
+
https://dev.azure.com/dnceng/internal/_git/dotnet-winforms
- bd280bbb5c9699bb93097206f076ad2f330ea8e1
+ 41a4bd690229661e3ec74276ce3f93863b22435b
-
+
https://dev.azure.com/dnceng/internal/_git/dotnet-winforms
- bd280bbb5c9699bb93097206f076ad2f330ea8e1
+ 41a4bd690229661e3ec74276ce3f93863b22435b
https://dev.azure.com/dnceng/internal/_git/dotnet-runtime
@@ -17,9 +17,9 @@
https://dev.azure.com/dnceng/internal/_git/dotnet-runtime
5535e31a712343a63f5d7d796cd874e563e5ac14
-
+
https://dev.azure.com/dnceng/internal/_git/dotnet-runtime
- 9f4b1f5d664afdfc80e1508ab7ed099dff210fbd
+ 2d7eea252964e69be94cb9c847b371b23e4dd470
https://dev.azure.com/dnceng/internal/_git/dotnet-wpf-int
@@ -29,17 +29,17 @@
https://dev.azure.com/dnceng/internal/_git/dotnet-runtime
5535e31a712343a63f5d7d796cd874e563e5ac14
-
+
https://dev.azure.com/dnceng/internal/_git/dotnet-runtime
- 9f4b1f5d664afdfc80e1508ab7ed099dff210fbd
+ 2d7eea252964e69be94cb9c847b371b23e4dd470
-
+
https://dev.azure.com/dnceng/internal/_git/dotnet-runtime
- 9f4b1f5d664afdfc80e1508ab7ed099dff210fbd
+ 2d7eea252964e69be94cb9c847b371b23e4dd470
-
+
https://dev.azure.com/dnceng/internal/_git/dotnet-runtime
- 9f4b1f5d664afdfc80e1508ab7ed099dff210fbd
+ 2d7eea252964e69be94cb9c847b371b23e4dd470
@@ -150,9 +150,9 @@
https://dev.azure.com/dnceng/internal/_git/dotnet-runtime
5535e31a712343a63f5d7d796cd874e563e5ac14
-
+
https://dev.azure.com/dnceng/internal/_git/dotnet-winforms
- bd280bbb5c9699bb93097206f076ad2f330ea8e1
+ 41a4bd690229661e3ec74276ce3f93863b22435b
diff --git a/eng/Versions.props b/eng/Versions.props
index 787ef93f144..e491f278d96 100644
--- a/eng/Versions.props
+++ b/eng/Versions.props
@@ -22,18 +22,18 @@
- 8.0.3-servicing.24116.3
+ 8.0.4-servicing.24169.11
- 8.0.3-servicing.24114.23
- 8.0.3
- 8.0.3
- 8.0.3-servicing.24114.23
+ 8.0.4-servicing.24169.9
+ 8.0.4
+ 8.0.4
+ 8.0.4-servicing.24169.9
8.0.0
8.0.0
8.0.0
- 8.0.3
+ 8.0.4
8.0.0
8.0.0
8.0.0
diff --git a/src/Microsoft.DotNet.Wpf/src/PenImc/dll/ComLockableWrapper.hpp b/src/Microsoft.DotNet.Wpf/src/PenImc/dll/ComLockableWrapper.hpp
index b16aae55806..0d3f01befc0 100644
--- a/src/Microsoft.DotNet.Wpf/src/PenImc/dll/ComLockableWrapper.hpp
+++ b/src/Microsoft.DotNet.Wpf/src/PenImc/dll/ComLockableWrapper.hpp
@@ -40,6 +40,11 @@ namespace ComUtils
// The apartment is verified during this call.
HRESULT Unlock();
+ // Unlocking a wrapper permanently nulls out the server object pointer, so a
+ // wrapper contains a non-null server object pointer if and only if it is
+ // bound to a server object which has never been unlocked.
+ bool HasNotBeenUnlocked() { return (m_serverObject != nullptr); }
+
private:
IUnknown *m_serverObject;
diff --git a/src/Microsoft.DotNet.Wpf/src/PenImc/dll/PimcManager.cpp b/src/Microsoft.DotNet.Wpf/src/PenImc/dll/PimcManager.cpp
index 0c8b10a8af4..40aba9f1a24 100644
--- a/src/Microsoft.DotNet.Wpf/src/PenImc/dll/PimcManager.cpp
+++ b/src/Microsoft.DotNet.Wpf/src/PenImc/dll/PimcManager.cpp
@@ -396,6 +396,94 @@ void CPimcManager::TerminateHookThread(__inout CHookThreadItem * pThread)
void CPimcManager::FinalRelease()
{
m_wispManagerLock.RevokeIfValid();
+
+ //
+ // The CComObject destructor is the only function which calls into this
+ // FinalRelease code.
+ //
+ // In all successful usage of CPimcManager: 1) Managed WPF code uses CoCreateInstance
+ // to acquire an IPimcManager2 interface to a brand-new CPimcManager instance (created by
+ // the ATL CComCreator::CreateInstance machinery), meaning FinalConstruct by-definition
+ // completes successfully, meaning "m_managerLock" is therefore guaranteed to be locked;
+ // 2) Managed WPF code then runs through its full end-to-end usage of the CPimcManager
+ // object (generally managed by the code in PenThreadWorker.cs); 3) When/if the managed WPF
+ // code determines that the CPimcManager object is no longer needed, it sends a
+ // RELEASE_MANAGER_EXT message (see UnsafeNativeMethods.ReleaseManagerExternalLock()) which
+ // unlocks "m_managerLock"; 4) Now that it is unlocked, the CComObject object
+ // can be destroyed when/if its refcount drops to zero, and this FinalRelease function will
+ // run at that time.
+ //
+ // So in all successful usage cases, it is guaranteed that "m_managerLock" is already
+ // unlocked when this code runs (because if it was still locked, the lock itself would have
+ // prevented the refcount from reaching zero, and would have prevented this function from
+ // ever running).
+ //
+ // That said, in unsuccessful usage cases, the ATL CComCreator::CreateInstance machinery
+ // can fail, meaning it will destroy the brand-new CPimcManager instance before returning
+ // an error back to the CreateInstance caller. Destroying the brand-new instance triggers
+ // the CComObject destructor and therefore calls into this function during
+ // the CComCreator::CreateInstance operation itself.
+ //
+ // The final step in CComCreator::CreateInstance is a QI which queries the newly-created
+ // object for whatever interface has been requested by the caller. This operation is the
+ // main way that CComCreator::CreateInstance can fail. For example, this QI is
+ // guaranteed to fail whenever the CoCreateInstance caller targets the CPimcManager CLSID
+ // but passes in a "random" IID that has nothing to do with IPimcManager2 or anything else
+ // that CPimcManager implements.
+ //
+ // (In CPimcManager construction, outside of pathological cases (e.g., where a small heap
+ // allocation in OS code fails due to out-of-memory), there are no other known ways that
+ // the CComCreator::CreateInstance sequence can fail; so the QI failure is the only
+ // failure mode that is known to be of general interest.)
+ //
+ // The QI failure can only occur after the preceding FinalConstruct call has completed
+ // successfully (since any FinalConstruct failure would have caused
+ // CComCreator::CreateInstance to abort without ever trying the QI); since
+ // CPimcManager::FinalConstruct always locks the "m_managerLock", this implies that the
+ // "m_managerLock" is guaranteed to be locked when this code runs (which is exactly
+ // opposite to what happens in all successful usage cases as discussed above).
+ //
+ // In this case, it is crucial to unlock "m_managerLock" before allowing this CPimcManager
+ // object to be destroyed. Without the unlock, this CPimcManager object would be destroyed
+ // while the associated CStdIdentity in the OS code still holds a reference to it; during
+ // any future apartment unload, the OS code would release this reference, and the release
+ // would be a use-after-free at that point.
+ //
+ // Note that the crucial unlock causes overactive ATL debug asserts to fire if a chk build
+ // of this DLL is used; specifically:
+ //
+ // - The code in the CComObject destructor always stomps the refcount to
+ // 0xc0000001 (i.e., "-(LONG_MAX/2)"), meaning this CPimcManager object's refcount is
+ // always 0xc0000001 when this code runs; unlocking "m_managerLock" will cause the refcount
+ // to drop by one (because, as discussed above, the crucial operation which prevents
+ // use-after-free problems will release the associated CStdIdentity's reference to this
+ // CPimcManager object, and in this way releases the reference that was added when
+ // "managerLock" was locked during FinalConstruct); as a result, unlocking "m_managerLock"
+ // will move this CPimcManager object's refcount through a "0xc0000001 -> 0xc0000000"
+ // transition.
+ //
+ // - Both of the CComObjectRootEx::InternalRelease specializations contain debug asserts
+ // which will fire whenever the refcount drops below 0xc0000001, so this transition always
+ // triggers a debug assert when using a chk build of this DLL.
+ //
+ // - That said, all evidence strongly suggests that this is just an overactive assert in
+ // the ATL code (probably just indicating that it is rare for FinalConstruct to add
+ // "self-references" like it does for CPimcManager (since these self-references generally
+ // prevent the server object from being destroyed unless a manual action like the
+ // RELEASE_MANAGER_EXT message is taken later on), meaning it is rare to have a situation
+ // where FinalRelease needs to release self-references that were acquired in
+ // FinalConstruct, meaning this is a rare enough case that the ATL authors either didn't
+ // test it or didn't think it was common enough to warrant adjusting the assert).
+ //
+ // Since this change is being made in servicing, attempt to change behavior as little as
+ // possible in the "successful usage" cases where "m_managerLock" is already unlocked,
+ // while still ensuring that FinalRelease will always run the crucial unlock in all
+ // "unsuccessful usage" cases.
+ //
+ if (m_managerLock.HasNotBeenUnlocked())
+ {
+ m_managerLock.Unlock();
+ }
}
/////////////////////////////////////////////////////////////////////////////