Span<byte> on ICryptoTransform

[vcsjones]

Background and Motivation

The current APIs on ICryptoTransform are byte[] based, and accept an offset and length. This pattern is better represented as a Span<byte> today, as developers are more familiar with Span and expect these APIs where the previous pattern was used.

For TransformFinalBlock, a byte array is returned, which forces the implementation to allocate on return. The Span based implementations could allow transformation in to an existing buffer, or in-place.

Proposed API

namespace System.Security.Cryptography {
    public partial interface ICryptoTransform {
+        int TransformBlock(ReadOnlySpan<byte> inputBuffer, Span<byte> outputBuffer);
+        bool TryTransformBlock(ReadOnlySpan<byte> inputBuffer, Span<byte> outputBuffer, out int bytesWritten);

+        int TransformFinalBlock(ReadOnlySpan<byte> inputBuffer, Span<byte> outputBuffer);
+        bool TryTransformFinalBlock(ReadOnlySpan<byte> inputBuffer, Span<byte> outputBuffer, out int bytesWritten);
    }
}

Risks

This would require the use of DIM, which to my knowledge hasn't been used in any significant capacity within the core libraries.

The DIM implementation would either need to throw something like NotSupported_SubclassOverride, or, provide a default implementation where the Span overloads defer back to the byte overloads. For implementations that end up using the DIM, the performance characteristics would be counter-intuitive. The Spans would need to allocate arrays to use the existing implementations, where the caller might have forced the Span overloads to be called since there is some reason for folks to believe that Span = Better.

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

[vcsjones]

cc @GrabYourPitchforks since I think you had thoughts on this.

[GrabYourPitchforks]

In general I like this concept. @terrajobst and @bartonjs might have thoughts on the use of DIMs, but this seems like a straightforward enough application.

The TransformFinalBlock and TryTransformFinalBlock methods are potentially problematic, however. Normally, a Try* method returning false means "resize the destination buffer and try again." However, in this case the default implementation of TryTransformFinalBlock would delegate to TransformFinalBlock, which has a side effect. This means that if the caller were to resize the destination buffer and try again, they could potentially receive the wrong value in the destination buffer, or the API could simply fail unexpectedly.

[vcsjones]

methods are potentially problematic

I hadn't given the implementation of the DIM too much thought, but you're right - it seems that likely the best solution then is for it to throw. Though if we have to throw for TryTransformFinalBlock, I think we should also throw for TryTransformBlock. I do not think we should throw for one and best effort the other.

Another option is to just get rid of the Try APIs.

I had also forgotten that the abstract HashAlgorithm (and thus all digest and HMAC algorithms) implement ICryptoTransform, which is, uh, fine, and I don't think it affects the API shape, but again pushes me toward thinking that the DIM should throw.

[GrabYourPitchforks]

Another option would be to introduce a new interface and not give the methods default implementations. Then if you want spanny APIs you'd cast to the new interface. The built in ICryptoTransform-implementing types could implement both interfaces implicitly to make things more convenient for callers.

[bartonjs]

Duplicate of #27118.

Since it requires the use of DIMs, it's going to be "no" for a long time. An entirely new approach, not modifying the current interface, is much likely a better plan (such as the new oneshot methods, which can be use virtuals to call ICryptoTransform or overridden in the real implementations to ignore all of the middle management.

[GrabYourPitchforks]

Is there ever a world in which the runtime repo can ship an interface with a default implementation? I feel this keeps coming up with some regularity. Perhaps it's time to doc that such a thing is not feasible within the runtime repo.

[bartonjs]

Is there ever a world in which the runtime repo can ship an interface with a default implementation?

Something net new introduced for net5+-only... perhaps.

Modifying an existing interface? Probably not for several years, we need to understand what goes wrong with it before we're comfortable accepting whatever risk we understand at the time. The unacceptable breaking changes list includes modifying an interface, that still stands for the shared framework, even with DIMs now part of the runtime.

[GSPP]

It should be possible to pass an empty outputBuffer because some algorithms produce no usable output.

https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.hashalgorithm.transformfinalblock?view=netcore-3.1