Runtime: Feature Request: Add Stream.ValidateBufferArguments helper

[geoffkizer]

Background and Motivation

Every Stream implementation needs to validate buffer arguments passed to the non-Span/Memory overloads that take byte[], int, int. We should provide a helper method in the base class to make this easy.

Example: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/HttpBaseStream.cs#L39

Proposed API

namespace System.IO
{
  public abstract class Stream
  {
    protected static void ValidateBufferArguments(byte[] buffer, int offset, int count);
    protected static void ValidateCopyToArguments(Stream source, Stream destination, int bufferSize);
    ...
  }
}

Usage Examples

C# class MyStream : Stream { public override int Read(byte[] buffer, int offset, int count) { ValidateBufferArguments(buffer, offset, count); // my implementation logic } }

@stephentoub Thoughts?

EDITED by @stephentoub 10/8 with updated proposal

[stephentoub]

I think this makes sense; you can see we do this ourselves in a bunch of places, e.g.
https://github.com/dotnet/runtime/blob/1beb1c635a2961d96d56d52025ee0fa7322933bb/src/libraries/System.Net.Http/src/System/Net/Http/HttpBaseStream.cs
https://github.com/dotnet/runtime/blob/1beb1c635a2961d96d56d52025ee0fa7322933bb/src/libraries/Common/src/System/Net/Http/aspnetcore/Quic/QuicStream.cs
https://github.com/dotnet/runtime/blob/1beb1c635a2961d96d56d52025ee0fa7322933bb/src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs#L391-L398
https://github.com/dotnet/runtime/blob/1beb1c635a2961d96d56d52025ee0fa7322933bb/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs#L460-L467
and multiple other places. We often even end up doing it open-coded multiple times in the same type, e.g. in each of Read, ReadAsync, Write, and WriteAsync. It'd be nice to consolidate it all.

As long as we're doing that, though, we should also do it for the other method that gets overridden frequently and needs argument validation: CopyTo{Async}. You can see in:
https://github.com/dotnet/runtime/blob/1beb1c635a2961d96d56d52025ee0fa7322933bb/src/libraries/Common/src/System/IO/StreamHelpers.CopyValidation.cs#L10
there's even more logic that goes into validating proper inputs (and current state of the this stream) for those methods, and we end up using this helper in a myriad of places:
https://source.dot.net/#q=validatecopytoargs

The helper should be static rather than instance.

So, in addition to:
```C#
protected static void ValidateBufferArguments(byte[] buffer, int offset, int count);

I think we should also add:
```C#
protected static void ValidateCopyToArguments(Stream source, Stream destination, int bufferSize);

We might also consider making them public rather than protected.

[stephentoub]

I was interested in seeing the scope of this. I think the change would look something like this:
https://github.com/stephentoub/runtime/commit/b4939952895232a276fdf4b27c719270eb583229
(Note this doesn't fix Streams that either a) live in assemblies that compile for things other than netcoreappcurrent or b) are public and use a different name for one of the arguments, e.g. NetworkStream uses size instead of count.)

[geoffkizer]

That's pretty impressive.

[Joe4evr]

@stephentoub Is there a reason your sample implementation uses throw new ... in one method and ThrowHelper. in the other, or is it simply a case of Consistently Inconsistent™?

[stephentoub]

You mean why it uses ThrowHelper in ValidateBufferArguments and throw new in ValidateCopyToArguments? Simply because throw new is simpler and ValidateCopyToArguments has no chance of being inlined / the extra IL doesn't matter, whereas ValidateBufferArguments does have a chance of getting inlined (and we could even decide to add AggressiveInlining to it) in which case more complexity is warranted and IL size does matter.

[davidfowl]

Would we also use this from the Span/Memory ctor?

[stephentoub]

Would we also use this from the Span/Memory ctor?

Unlikely. Those ctors are so streamlined they don't even include argument names as part of the validation. And for better or worse (worse, IMHO), the names used with those ctors differ from what's used in stream's Read/Write methods (e.g. array/start/length vs buffer/offset/count). And certain combinations are actually allowed with span/memory that aren't with stream, e.g. you can pass a null array to span as long as the start and length are 0.

[terrajobst]

Video

• There are several derivatives of Stream that need to validate arguments
  
  
  
  • Getting the exception and parameter names is tricky and we had several bugs
  
  
• We like the pattern (Validate prefix, Arguments suffix)
  
  
  
  • The names make sense too
  
  
  • The choice of not tying it to a specific method makes sense because the buffer one applies to all methods on Stream that take a buffer (and the parameter names are thankfully consistent)
  
  
• Let's remove the Stream source argument from the second (because it's this)
  
  
  
  • The caller is responsible for validating this, these methods are only about arguments
  
  

C# namespace System.IO { public abstract class Stream { protected static void ValidateBufferArguments(byte[] buffer, int offset, int count); protected static void ValidateCopyToArguments(Stream destination, int bufferSize); } }