[release/10.0] Fix missing release semantics in VolatilePtr

[hoyosjs]

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.

VolatilePtr inherits from Volatile<P>, which defines operator= to call VolatileStore() with platform scpecific release semantics. 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 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).

[Copilot]

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.h and src/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

[hoyosjs]

/backport to main

[github-actions]

Started backporting to main (link to workflow run)

[hoyosjs]

/backport to release/9.0

[github-actions]

Started backporting to release/9.0 (link to workflow run)

[hoyosjs]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0 (link to workflow run)

[hoyosjs]

/backport to release/8.0-staging

[hoyosjs]

/backport to release/9.0-staging

[github-actions]

Started backporting to release/8.0-staging (link to workflow run)

[github-actions]

Started backporting to release/9.0-staging (link to workflow run)

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[AaronRobinsonMSFT]

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?

[AaronRobinsonMSFT]

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).

so the compiler-generated copy/move assignment operator hid the base class

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.

[agocke]

I think we should get @davidwrighton's view on this

[davidwrighton]

This is good for servicing. It is NOT a complete implementation of making VolatilePtr<T> "safe", but it makes it behave more like the old Microsoft volatile semantics (which will make all writes the the volatile variable into writes with a write barrier.)

[davidwrighton]

The notable validation Juan and I performed was to build with this change, and observe that the logic used in updating the m_pBlockingLock variable in DeadlockAwareLock::BeginEnterLock went from using the str instruction, to using the stlr instruction.

[hoyosjs]

I checked with Jan and David and both agreed on the targeted fix. I've checked volatilePtr callsites and verified the assembly as well as checked that MSVC does generate move stubs but nothing uses them at the moment.