Simplify range validation in Memory.Span

[xtqqczze]

• desiredStartIndex cannot be negative as high order bit of _index has been removed.
• desiredLength cannot be negative as all public constructors validate _length is not negative.
• lengthOfUnderlyingSpan cannot be negative as _object.Length as the length property of an array, string, or span cannot be negative.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

[EgorBo]

if this + overflows the check is passed, isn't ?

[xtqqczze]

desiredLength should never be negative, this could do with an assert

runtime/src/libraries/System.Private.CoreLib/src/System/Memory.cs

Line 330 in 8b10891

int desiredLength = _length;

[Clockwork-Muse]

Yes - it's why the 32-bit target check is written the way it was below (which is a common overflow-avoiding pattern).

We can't "simplify" this, though - the 64 and 32 bit targets are different for a reason; they're taking advantage of different bit widths (or not able to) on different platforms.

[tannergooding]

This is the kind of thing where the logic is getting even more dense and hard to comprehend

I would much rather we do such transforms in the JIT when we know that a given invariant is held, so that the high level managed code can remain easy to understand.

Otherwise, this needs a significant number of comments and asserts covering those invariants and why the various overflow considerations are "safe" before it could be accepted.

[xtqqczze]

The check here is only to prevent undefined behaviour if the struct is torn, if it wasn't for this it could be removed completely.

[xtqqczze]

I would much rather we do such transforms in the JIT when we know that a given invariant is held, so that the high level managed code can remain easy to understand.

Related issue: #118587

[xtqqczze]

desiredLength should never be negative, this could do with an assert

runtime/src/libraries/System.Private.CoreLib/src/System/Memory.cs

Line 330 in 8b10891

int desiredLength = _length;

We already assume the invariant _length >= 0 holds

runtime/src/libraries/System.Private.CoreLib/src/System/Memory.cs

Lines 225 to 230 in 1e322fd

	public Memory<T> Slice(int start)
	{
	if ((uint)start > (uint)_length)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	}

[tannergooding]

The check here is only to prevent undefined behaviour if the struct is torn, if it wasn't for this it could be removed completely.

Yes, which is necessary/important. The team has a firm stance that we can't rely on users doing safe threading and that we need to do the "right things".

It's the same general reason we have both sets of lengths checks when dealing with List<T> (CC. @GrabYourPitchforks)

We already assume the invariant _length >= 0 holds

It's still something where adding an assert is beneficial as it explicitly documents the expectation.

But, I'd generally rather we have the JIT doing these types of optimizations. The managed code should prefer being readable/understandable first. If something is truly perf critical, then making it less readable with added comments/asserts is ok. However, the JIT automatically recognizing the critical pattern and doing the right thing is even better, as then the code stays readable and other paths likely benefit as well.

[xtqqczze]

@MihuBot

[MihaZupan]

Changes here overlap with #115275

[Clockwork-Muse]

Yes - it's why the 32-bit target check is written the way it was below (which is a common overflow-avoiding pattern).

We can't "simplify" this, though - the 64 and 32 bit targets are different for a reason; they're taking advantage of different bit widths (or not able to) on different platforms.

[tannergooding]

This at least needs a comment explaining why it's safe (that it can't overflow) and should likely do (uint)(desiredStartIndex + desiredLength) instead of (uint)desiredStartIndex + (uint)desiredLength

Additional numbers/info showing the wins would also be beneficial.

[xtqqczze]

// Unsigned overflow cannot occur here because of the following invariants:
// desiredStartIndex <= int.MaxValue; as ReadOnlyMemory<T>.RemoveFlagsBitMask == int.MaxValue
// desiredLength >= 0; as it is assigned from the _length field which is non-negative by construction
// lengthOfUnderlyingSpan >= 0; as it is assigned from the object's Length property which has a non-negative invariant

[tannergooding]

That seems reasonable. I might put // * desiredStartIndex and similarly for the other 2 listed invariants to help show its part of a list, but I think that fits the need here and covers future readers.

[xtqqczze]

I realised from your comment in the other issue that _length >= 0 may not hold if the struct is torn. Since the condition in question is specifically checking for a torn structure that would be issue without changes to the Slice method.

Ah right, Span<T> is stack only (well non-heap, but anything off the managed heap or stack would have to be a Span<T>* and so unsafe) and the memory model makes it UB to read from/write to the stack of another thread.

Where-as you can have class C { Memory<T> _field; } where one thread reads (objA, index: 5, length: 20) and another thread writes (objB, index: 5, length: 5) in which case a race could exist in Slice(start: 10) such as we get (objB, index: 5, length: -5) because the compare (start > _length) thought it was length: 20 but then the constructed Memory<T> ends up with objB and a length: 5 so we do 5 - 10.

Originally posted by @tannergooding in #119708

[tannergooding]

Yep, that'd be a risk and might mean this is something that can't be done.

You would need to show if the same thing could be violated via the existing logic and/or if the field was hoisted into a local.