SequenceReader contains invalid volatile memory access patterns

[GrabYourPitchforks]

See code:

runtime/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs

Lines 97 to 108 in c8a97ff

	public readonly long Length
	{
	get
	{
	if (_length < 0)
	{
	// Cast-away readonly to initialize lazy field
	Volatile.Write(ref Unsafe.AsRef(_length), Sequence.Length);
	}
	return _length;
	}
	}

Accesses to the _length field are not guaranteed atomic on 32-bit platforms without going through the Volatile or Interlocked APIs. While the write is protected by such a guard, the read is not. This could result in the _length backing field being torn and the Length property returning an incorrect value on 32-bit platforms.

[GrabYourPitchforks]

A possible fix:

• On all platforms, use Volatile for both reads and writes. (On 32-bit platforms, this results in an interlocked operation.) OR

• On 64-bit platforms, don't use Volatile, since reads + writes are atomic. On 32-bit platforms, while reading, perform a volatile read of the high 32 bits first, then a non-volatile read of the low 32 bits. If the result is negative, write the low 32 bits first, then perform a volatile write of the high 32 bits. This will not result in an interlocked operation.

[gfoidl]

SequenceReader is a ref struct, so stack-only. Is atomicity a problem here? Also is visibility (volatile) a concern?

runtime/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs

Line 11 in c8a97ff

public ref partial struct SequenceReader<T> where T : unmanaged, IEquatable<T>

The Volatile.Write introduced in dotnet/corefx#37718 could be a plain write instead.

[GrabYourPitchforks]

SequenceReader is a ref struct, so stack-only. Is atomicity a problem here? Also is visibility (volatile) a concern?

Good point. I'm trying to remember why it's a ref struct since it doesn't contain any byref fields. If it's intended to be a ref struct, then yeah, we can kill all of the volatile goop.

[gfoidl]

trying to remember why it's a ref struct

From #27522

SequenceReader can't be used in async methods

So it's intentional to prevent this?!

[benaadams]

I'm trying to remember why it's a ref struct since it doesn't contain any byref fields

Except this one 😉

public ReadOnlySpan<T> CurrentSpan { readonly get; private set; }

[GrabYourPitchforks]

Aaaargh. I hate that it's buried with other properties and isn't grouped with the fields at the top of the file.

[GrabYourPitchforks]

Then it sounds like @gfoidl's suggestion is the best one. Find places in SequenceReader<T> that try to be thread-safe (are there others aside from this?) and remove the unneeded thread safety aspects.

[gfoidl]

@GrabYourPitchforks are you working on this? Otherwise I can have a look.

and isn't grouped with the fields at the top of the file

That should be done in the same PR.

[GrabYourPitchforks]

I wouldn't change the layout of the file as part of this PR. Just lamenting generally. 😞

[gfoidl]

This was the only occurance --> #45437 (hope it's an easy review 😉).

[GrabYourPitchforks]

Thanks @gfoidl and @adamsitnik for handling this! :)