Theoretical race condition in PETypeParameterSymbol (and possibly other locations)

[333fred]

There is a theoretical race condition in how we initialize data in PETypeParameterSymbol (currently just C#, but soon to be VB as well since it's copying the same code style). This race condition is practically impossible to reproduce, to be clear; on x86/64, the memory model should make it impossible. On ARM, it is technically possible to hit due to the looser memory model, but would also be extremely unlikely to happen in reality. However, we should fix this so that we're not relying on undefined behavior for this scenario.

The race

The original file is here. I'm going to copy over a simplified version to this issue for demonstration purposes.

internal sealed class PETypeParameterSymbol
    : TypeParameterSymbol
{
    private ThreeState _lazyHasIsUnmanagedConstraint; // This is only valid when _lazyDeclaredConstaintTypes is non-default
    private ImmutableArray<TypeWithAnnotations> _lazyDeclaredConstraintTypes; // This is a sentinel value

    private ImmutableArray<TypeWithAnnotations> GetDeclaredConstraintTypes(ConsList<PETypeParameterSymbol> inProgress)
    {
        if (_lazyDeclaredConstraintTypes.IsDefault)
        {
            // ...
            _lazyHasIsUnmanagedConstraint = hasUnmanagedModreqPattern.ToThreeState();
            // This is a full read-write barrier: as long as we guarantee that _lazyDeclaredConstraintTypes is read before
            // _lazyHasIsUnmanagedConstraint, we aren't in a torn state: either _lazyDeclaredConstraintTypes is non-default,
            // and _lazyHasIsUnmanagedConstraint is valid, or _lazyDeclaredConstraintTypes is default, and we'll (re)do the
            // initialization work on this thread before observing _lazyHasIsUnmanagedConstraint
            ImmutableInterlocked.InterlockedInitialize(ref _lazyDeclaredConstraintTypes, declaredConstraintTypes);
        }
    }

    public override bool HasUnmanagedTypeConstraint
    {
        get
        {
            GetDeclaredConstraintTypes(ConsList<PETypeParameterSymbol>.Empty);
            // There is no read barrier between these locations: the JIT and/or CPU is allowed to speculatively read
            // _lazyHasIsUnmanagedConstraints before/in tandem with the `if (_lazyDeclaredConstraintTypes.IsDefault)`
            // call above. In a worst-case scenario, this is a race condition
            return this._lazyHasIsUnmanagedConstraint.Value();
        }
    }
}

In practice, for this to actually race, multiple things have to happen:

1. The JIT has to decide that it's worth it to inline GetDeclaredConstantTypes, or the CPU has to decide to start speculatively executing the code following the return.
2. Some mechanism (either JIT or CPU) has to decide that it's worth it to start speculatively executing the else of the if (_lazyDeclaredConstaintTypes.IsDefault) branch.
3. The speculative access of _lazyHasUnmanagedConstraint has to complete before the _lazyDeclaredConstraintTypes access.
4. Another thread writes to both _lazyHasUnmanagedConstraint and _lazyDeclaredConstraintTypes, causing a cache miss for the non-speculative read of _lazyDeclaredConstraintTypes.
5. At this point, we read the final, non-default value of _lazyDeclaredConstraintTypes. This causes us to go down the else branch of the if, and the speculative read of _lazyHasUnmanagedConstraint is used, which is an invalid value.

As I mentioned previously, the memory model on x86/64 guarantees that this type of speculation can't occur. On ARM, it's technically possible, just very unlikely. I have no idea what other arches, like Z, will do here.

Fixing it

We have a few options for fixing this:

1. Do some horrible unsafe hacks to manually Volatile.Read the backing array of _lazyDefaultConstraints. This will ensure that proper read fences are put in place such that _lazyDefaultConstraints is guaranteed to be read before _lazyHasUnmanagedConstraint. Possible, and should have the least runtime perf impact, but really horrible.
2. Use Interlocked.MemoryBarrier() in the else block of GetDeclaredConstraintTypes, which will ensure that in all cases, _lazyDefaultConstraints is guaranteed to be read before _lazyHasUnmanagedConstraint. Technically has some runtime impact, but not noticeable.
3. Encapsulate these fields into an immutable, lazily-allocated backing object, like we do in other PE types with UncommonData. This is the easiest to get right, since all access is through a single field and atomic operations are trivial.
4. Add an extension method like VolatileIsDefault() that does an Interlocked.MemoryBarrier() on downlevel platforms, and Volatile.ReadBarrier() on .NET 10 when available, and use that instead of IsDefault for these types of checks.

Thanks to @stephentoub for helping us figure out the semantics here.

[333fred]

Team Meeting Conclusions

1. We're going to add ImmutableArray<T> RoslynImmutableInterlocked.VolatileRead<T>(ref ImmutableArray<T> value) and use that to read these arrays, using the best barrier available on the platform.
2. We'll audit usage of PackedFlags for other possible threading issues from missing Volatile.Reads
3. We'll document this pattern in docs/compiler
4. We're not going to invest in an analyzer at this time.