From 3e8cb92e3a3ddee90b5576f52e9cac301eeca62a Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Mon, 22 Jun 2026 23:42:42 -0700 Subject: [PATCH 1/4] Stop relying on UB in 'IContextCallback' dispatch logic Port the cross-thread access fix from #1865 to the CsWinRT 3.0 runtime. The callback state (a managed object) was previously stored in a local variable on the original thread's stack and then dereferenced from the native callback running on the target thread, which is undefined behavior per the .NET memory model (cross-thread access to local variables). Store the state in a thread-static field instead (with a throwaway array fallback for the never-expected recursion case), add a memory barrier, pin the storage, and pass a pointer to it so the target thread always reads a valid location on the managed heap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ObjectReference/ContextCallback.cs | 74 ++++++++++++++----- 1 file changed, 56 insertions(+), 18 deletions(-) diff --git a/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs b/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs index 01941a99f..fb63e9896 100644 --- a/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs +++ b/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs @@ -3,6 +3,7 @@ using System; using System.Runtime.InteropServices; +using System.Threading; namespace WindowsRuntime.InteropServices; @@ -11,6 +12,12 @@ namespace WindowsRuntime.InteropServices; /// internal static unsafe class ContextCallback { + /// + /// Storage for the user-provided state for . + /// + [ThreadStatic] + private static object? LocalContextCallbackState; + /// /// Calls the given callback in the right context, and returns the result of that invocation. /// @@ -38,21 +45,10 @@ public static HRESULT CallInContextUnsafe( return WellKnownErrorCodes.S_OK; } - ComCallData comCallData; - comCallData.dwDispid = 0; - comCallData.dwReserved = 0; - - CallbackData callbackData; - callbackData.Callback = callback; - callbackData.State = state; - - // We can just store a pointer to the callback to invoke in the context, - // so we don't need to allocate another closure or anything. The callback - // will be kept alive automatically, because 'comCallData' is address exposed. - // In 'InvokeCallback' below, we then invoke the current caller-provided callback. - comCallData.pUserDefined = &callbackData; - - // Stub to invoke on the target context + // Native method that invokes the callback on the target context. The state object is guaranteed to be pinned, + // so we can access it from a pointer. Note that the object will be stored in a static field, and it will not + // be on the stack of the original thread, so it's safe with respect to cross-thread access of managed objects. + // See: https://github.com/dotnet/runtime/blob/main/docs/design/specs/Memory-model.md#cross-thread-access-to-local-variables. [UnmanagedCallersOnly] static int InvokeCallback(ComCallData* comCallData) { @@ -60,7 +56,7 @@ static int InvokeCallback(ComCallData* comCallData) { CallbackData* callbackData = (CallbackData*)comCallData->pUserDefined; - callbackData->Callback(callbackData->State); + callbackData->Callback(*callbackData->StatePtr); return WellKnownErrorCodes.S_OK; } @@ -70,10 +66,49 @@ static int InvokeCallback(ComCallData* comCallData) } } + ref object? localContextCallbackState = ref LocalContextCallbackState; + + // Store the state object in the thread static to pass to the callback. + // We don't need a volatile write here, we have a memory barrier below. + // A thread local is the most efficient solution for this, given that + // we need the state to be somewhere on the managed heap to be valid. + // The GC doesn't allow cross-thread access to managed stack variables. + if (localContextCallbackState is null) + { + localContextCallbackState = state; + } + else + { + // In case we recursed on this thread, meaning the local storage already holds a state + // for some other caller above us in the stack, we can use a throwaway array to store + // the current state. This isn't very efficient, but in practice this case should not + // ever happen (it was validated in our entire test suite as well as in stress tests + // with the Microsoft Store app, and we never detected recursion on this code path). + // However just to be extra safe, we still want to keep this branch functional too. + object[] objects = [state]; + + localContextCallbackState = ref MemoryMarshal.GetArrayDataReference(objects)!; + } + HRESULT hresult; + // Pin the state storage, which we can now safely pass to the target thread + fixed (object* statePtr = &localContextCallbackState) fixed (Guid* riid = &WellKnownWindowsInterfaceIIDs.IID_ICallbackWithNoReentrancyToApplicationSTA) { + CallbackData callbackData; + callbackData.Callback = callback; + callbackData.StatePtr = statePtr; + + ComCallData comCallData; + comCallData.dwDispid = 0; + comCallData.dwReserved = 0; + comCallData.pUserDefined = &callbackData; + + // Add a memory barrier to be extra safe that the target thread will be able to see + // the write we just did on 'LocalContextCallbackState' with the state to pass to the callback. + Thread.MemoryBarrier(); + // Marshal the supplied callback on the target context hresult = IContextCallbackVftbl.ContextCallbackUnsafe( thisPtr: contextCallbackPtr, @@ -84,6 +119,9 @@ static int InvokeCallback(ComCallData* comCallData) pUnk: null); } + // Reset the static field to avoid keeping the state alive for longer + Volatile.Write(ref LocalContextCallbackState, null); + return hresult; } @@ -98,8 +136,8 @@ private struct CallbackData public delegate* Callback; /// - /// The additional argument to supply to . + /// A pointer to the additional argument to supply to . /// - public object State; + public object* StatePtr; } } \ No newline at end of file From b659df6ff789c8484fe0e83facb105dee04aad67 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 23 Jun 2026 10:23:58 -0700 Subject: [PATCH 2/4] Remove redundant Thread.MemoryBarrier call Delete an unnecessary Thread.MemoryBarrier() invocation and its explanatory comment from ContextCallback.cs. Cleans up redundant synchronization placed before marshaling the callback via IContextCallback. --- .../InteropServices/ObjectReference/ContextCallback.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs b/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs index fb63e9896..f7058dc47 100644 --- a/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs +++ b/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs @@ -105,10 +105,6 @@ static int InvokeCallback(ComCallData* comCallData) comCallData.dwReserved = 0; comCallData.pUserDefined = &callbackData; - // Add a memory barrier to be extra safe that the target thread will be able to see - // the write we just did on 'LocalContextCallbackState' with the state to pass to the callback. - Thread.MemoryBarrier(); - // Marshal the supplied callback on the target context hresult = IContextCallbackVftbl.ContextCallbackUnsafe( thisPtr: contextCallbackPtr, From c29d6989bc9811d63cd5ee2ab49557a06f2ae36e Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 23 Jun 2026 10:27:27 -0700 Subject: [PATCH 3/4] Use direct assignment to clear context state Replace Volatile.Write(ref LocalContextCallbackState, null) with a direct assignment LocalContextCallbackState = null in ContextCallback.cs. This simplifies the reset of the static field (avoiding the Volatile API and its memory barrier) while preserving the intent to clear the state and avoid keeping it alive longer. --- .../InteropServices/ObjectReference/ContextCallback.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs b/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs index f7058dc47..3cf22a980 100644 --- a/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs +++ b/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs @@ -3,7 +3,6 @@ using System; using System.Runtime.InteropServices; -using System.Threading; namespace WindowsRuntime.InteropServices; @@ -116,7 +115,7 @@ static int InvokeCallback(ComCallData* comCallData) } // Reset the static field to avoid keeping the state alive for longer - Volatile.Write(ref LocalContextCallbackState, null); + LocalContextCallbackState = null; return hresult; } From 3e63183d0a8314338a171832a05412ec647877c4 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 23 Jun 2026 10:27:40 -0700 Subject: [PATCH 4/4] Clarify volatile write rationale in callback Expand the comment in ContextCallback.cs to explain why a volatile write is not used: volatile writes prevent earlier memory operations from being moved after them but do not constrain reordering of subsequent operations, whereas this write must not be moved before later reads of the thread-static field. This is a documentation-only change and does not alter runtime behavior. --- .../InteropServices/ObjectReference/ContextCallback.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs b/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs index 3cf22a980..b87630265 100644 --- a/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs +++ b/src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs @@ -68,10 +68,14 @@ static int InvokeCallback(ComCallData* comCallData) ref object? localContextCallbackState = ref LocalContextCallbackState; // Store the state object in the thread static to pass to the callback. - // We don't need a volatile write here, we have a memory barrier below. // A thread local is the most efficient solution for this, given that // we need the state to be somewhere on the managed heap to be valid. // The GC doesn't allow cross-thread access to managed stack variables. + // We're not using a volatile write, as it wouldn't actually help at all. + // Volatile writes disallow the write operations to be moved before memory + // operations that precede them, but they have nothing to say with respect + // to memory operations after them (whereas here we specifically need this + // write to not be reordered before following reads from that static field). if (localContextCallbackState is null) { localContextCallbackState = state;