WindowsRuntimeBufferExtensions.ToArray incorrectly checks Capacity

[AaronRobinsonMSFT]

Issue moved from dotnet/runtime - see dotnet/runtime#23383

The following code will throw an ArgumentException:

var buffer = new Windows.Storage.Streams.Buffer(0);
var array = buffer.ToArray();

WindowsRuntimeBufferExtensions.ToArray checks the buffer capacity as a precondition, which isn't used in the method. Obviously the expected behavior would be to return an array of length zero.

I also can't find a unit test for this case.

[weltkante]

For what its worth this edge case is also present in other methods of WindowsRuntimeBufferExtensions. (Count is allowed to be zero but then the index-check against the capacity is off by one to actually let the code through. This allows a count of zero on every position except the last, which is odd.)

[j0shuams]

This comment suggests making the condition in ToArray a strict greater-than.

[j0shuams]

The other methods this check happens in is CopyTo. I tried the following code, with no change to the exception condition:

var buffer = new Windows.Storage.Streams.Buffer(0);
byte[] bytes = { };
buffer.CopyTo(bytes);

And there was no error, so I think changing the condition there is unnecessary.

[weltkante]

The fix was incomplete, there are various extension methods in WindowsRuntimeBufferExtensions which still throw incorrectly.

[j0shuams]

@weltkante Can you be more descriptive and/or give examples? Of the remaining public methods: AsBuffer, CopyTo and GetByte, I do not see a situation like you describe of count being zero causing an index check against capacity to incorrectly throw.

I see in CopyTo (for byte[] -> Buffer) there is a check of source.Length - sourceIndex < count ; but I do not see how this can cause false errors

[weltkante]

The cause of the false checks are incorrect <= checks - in particular of the style if (source.Length <= sourceIndex) throw new ArgumentException(...);

An index can be at the end of the buffer when count is zero, see my comment above, so many of the extension methods incorrectly throw in this case even though they do not throw if length is zero at any place except the end of buffer.

If that description isn't enough and you need a full list of failure scenarios I can do one tomorrow, its a bit late today on my end.

[j0shuams]

@weltkante thanks for your reply, do you think changing to < is enough or should the <= be anded (&&) with count != 0 ?

[weltkante]

I think its an off-by-one error and changing <= into < should be enough, but I didn't do a full review. The PR which was used to fix some of the errors might not have had to introduce new conditionals if these off-by-one were fixed, so thats also worth taking a second look.

Generally the invariants for upper bounds are that offset <= buffer.Length and count <= buffer.Length - offset or some permutation thereof. Notice that the inverse (the conditional for throwing) would never throw on equality. You always get away with exactly two conditionals and no special casing zero lengths, you only special case it if its worth doing early exit for performance.

[j0shuams]

@weltkante I've made some additional changes, review/feedback would be greatly appreciated! #504