Stop relying on UB in 'IContextCallback' dispatch logic#2470
Open
Sergio0694 wants to merge 4 commits into
Open
Stop relying on UB in 'IContextCallback' dispatch logic#2470Sergio0694 wants to merge 4 commits into
Sergio0694 wants to merge 4 commits into
Conversation
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>
Sergio0694
added
bug
Member
Author
|
@hamarb123 @jkotas I've ported the updated changes to the 3.0 branch, could you take a look when you have time to confirm whether this is the best solution for this and whether the change makes sense? I can also backport to 2.x once we confirm the changes are correct. Thank you! 🙂 |
hamarb123
reviewed
Jun 23, 2026
hamarb123
left a comment
hamarb123
left a comment
There was a problem hiding this comment.
Lgtm, other than these 2 comments.
jkotas
reviewed
Jun 23, 2026
jkotas
reviewed
Jun 23, 2026
jkotas
reviewed
Jun 23, 2026
Delete an unnecessary Thread.MemoryBarrier() invocation and its explanatory comment from ContextCallback.cs. Cleans up redundant synchronization placed before marshaling the callback via IContextCallback.
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.
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.
Sergio0694
force-pushed
the
user/sergiopedri/fix-context-callback-ub
branch
from
196046c to
3e63183
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Port the cross-thread access fix from #1865 to the CsWinRT 3.0 runtime (
WinRT.Runtime2). The context callback dispatch logic no longer stores the managed callback state in a stack local that is then dereferenced from the target thread, which is undefined behavior under the .NET memory model.Motivation
When dispatching a callback onto another context via
IContextCallback, the previous code stored the user-provided state (a managed object) in a local variable on the original thread's stack and passed a pointer to it throughComCallData. The nativeInvokeCallbackstub then dereferenced that pointer while running on the target thread. Reading a managed reference that lives on another thread's stack is explicitly undefined behavior, see the .NET memory model spec on cross-thread access to local variables. This is the same class of issue fixed for CsWinRT 2.x in #1865; this PR brings the fix to the 3.0 runtime.Changes
src/WinRT.Runtime2/InteropServices/ObjectReference/ContextCallback.cs: reworkCallInContextUnsafeto stop passing a stack-local managed reference across threads.[ThreadStatic]LocalContextCallbackStatefield so the state lives on the managed heap rather than on the calling thread's stack.CallbackData.StatePtr) to the target context, with aThread.MemoryBarrier()before the cross-context call and aVolatile.Write(ref LocalContextCallbackState, null)reset afterwards to avoid extending the state's lifetime.CallbackData.State(anobject) toCallbackData.StatePtr(anobject*).