[release/10.0] Fix missing release semantics in VolatilePtr#124070
[release/10.0] Fix missing release semantics in VolatilePtr#124070hoyosjs wants to merge 1 commit intodotnet:release/10.0from
Conversation
VolatilePtr inherits from Volatile<P>, which defines operator=(P val) to call VolatileStore() with release semantics (stlr on ARM64). However, VolatilePtr did not declare its own operator=, so the compiler-generated copy/move assignment operator hid the base class operator= and performed plain stores (str) instead, bypassing the memory barrier entirely. This affected all VolatilePtr assignments including DeadlockAwareLock's m_pHoldingThread and Thread's m_pBlockingLock fields, which were being written without release semantics despite being read with acquire semantics (ldapr) on the other side. Fix by adding a using declaration to bring Volatile<P>::operator= into scope, and an explicit copy assignment operator (which the using declaration cannot suppress the compiler from generating).
github-actions
bot
added
the
needs-area-label
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical memory barrier bug in the VolatilePtr template class. The class inherits from Volatile<P> which provides an operator= that uses release semantics (via VolatileStore). However, the compiler-generated copy/move assignment operators were hiding the base class operator, resulting in plain stores without memory barriers. This affected thread synchronization primitives like DeadlockAwareLock::m_pHoldingThread and Thread::m_pBlockingLock, where writes lacked release semantics despite corresponding reads using acquire semantics.
Changes:
- Added
using Volatile<P>::operator=;declaration to bring the base class assignment operator into scope - Added explicit copy assignment operator
operator=(const VolatilePtr<T,P>&)to handle copy assignment with proper memory barriers - Applied the fix consistently to both
src/coreclr/inc/volatile.handsrc/coreclr/gc/env/volatile.h
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/inc/volatile.h | Fixes VolatilePtr to preserve release semantics by adding using declaration and explicit copy assignment operator |
| src/coreclr/gc/env/volatile.h | Same fix as above for the GC subsystem's copy of the volatile.h header |
|
/backport to main |
|
Started backporting to |
|
/backport to release/9.0 |
|
Started backporting to |
|
/backport to release/8.0 |
|
Started backporting to |
|
/backport to release/8.0-staging |
|
/backport to release/9.0-staging |
|
Started backporting to |
|
Started backporting to |
hoyosjs
changed the title
AaronRobinsonMSFT
added
Servicing-consider
|
Tagging subscribers to this area: @agocke |
|
I have some concerns about this area. Not necessarily in these targeted changes but in the design itself. There is a lot of confusion in this implementation with respect to correct C++ design patterns and the issue being addressed here is simply one in a series of bad assumptions with respect to C++. I'm not convinced the solution here is even appropriate/sufficient at present. What is our servicing timeline here? |
Okay, I see there is additional work being referenced in #124096 (comment).
The above statement is my primary concern. I'd like to understand how this has been tested and validated with respect to use. I only note a copy assignment being exposed here, which when declared should disable auto generation of move assignment, MSVC has historical had slightly different rules here. I'd like to ensure we have audited all use sites and verified what MSVC is doing and what clang/gcc is doing. As I said, this narrow fix does seem appropriate at first glance but understanding the scope of the problem is needed in my opinion. |
3 participants
Related issue: #124071
Main PR: #124096
Risk: Low, purely adds a barrier which is the intended semantics of the class.
Impact: Detected as a non-deterministic lock free race that resulted in crashes in the Roslyn language server for internal customers. Mostly happens with really complex solutions that have a lot of threads accessing GC Statics.
VolatilePtrinherits fromVolatile<P>, which definesoperator=to callVolatileStore()with platform scpecific release semantics. However,VolatilePtrdid not declare its ownoperator=, so the compiler-generated copy/move assignment operator hid the base classoperator=and performed plain stores instead, bypassing memory barriers entirely.Fix by adding a using declaration to bring
Volatile<P>::operator=into scope, and an explicit copy assignment operator (which the using declaration cannot suppress the compiler from generating).