Add a _malloca-like API Span<T>.Alloc(int length)

[GrabYourPitchforks]

(This is distinct from the proposal at https://github.com/dotnet/corefx/issues/26954, which considers a more powerful API that can give the caller specific information about the current stack. The proposal under consideration here is only for a "KISS" API. Both proposals could exist side-by-side depending on the scenario, and in fact this proposal could depend on the more powerful API if we decide that's the right behavior.)

As the pattern Span<T> span = stackalloc T[len]; becomes more commonplace, we're seeing a rise in incorrect usage where developers are piping through untrusted len values, potentially making their applications susceptible to reliability issues or DoS attacks. The safe language construct is Span<T> span = ((uint)len <= 128 /* or other const */) ? stackalloc T[len] : new T[len];. We can't rely on our larger developer audience remembering the safe pattern, and even our more knowledgeable advanced developers can forget the check from time to time.

Following are concrete proposals from the security team as to how this can be addressed.

First, create a utility method public static Span<T> Span<T>.Alloc(int length). This API will stackalloc in the current frame if appropriate; otherwise the method will heap-alloc, similar to the CRT's _malloca API. Adding this method and getting the desired runtime behavior would not require compiler changes, but it would require JIT changes. The implementation of the method would default simply to new T[length] if length is negative or is too large. Advanced developers who want to pull from an existing array pool as fallback wouldn't use this API; they'd perform the check manually.

Second, once the utility method is in place, the compiler should be smart enough to treat return Span<T>.Alloc(...); as a compile-time error, just as it does today for Span<T> span = stackalloc T[...]; return span;. This would require a compiler change.

Third, the developer should be warned when passing a non-constant and non-bounds-checked value into stackalloc, and the warning should direct the developer to use Span<T>.Alloc instead. The developer should also be warned when using stackalloc or Span<T>.Alloc in a loop. (We considered warning on recursive method calls, but that really belongs in a different analyzer.) Making this a proper compiler warning is probably a bit too noisy, so it could be a Roslyn code analyzer instead. This wouldn't require a compiler or JIT change; it'd just be a new version of the analyzers package for developers who run it as part of build.

Finally, remember that this is a KISS proposal. It's intended for the majority use case for developers who may want to consume Span-based APIs but need a bit of a safety net. Advanced developers could still use the other APIs under consideration or perform their own custom checks as appropriate for their applications.

[benaadams]

Have heard from people that know the pattern; "what's the right number to use"; so a built-in would be better (while still allowing those that know their levels to fallback to the pattern)

[AndyAyersMS]

An intrinsic method that conditionally turns into a stackalloc is bit tricky for the jit to handle, but I think we have most of the parts in place.

The upshot is that any method that calls Span<T>.Alloc will likely never be inlined, which is more or less true of any method that uses stackalloc.

[Drawaes]

What is the right number.....that is the real question....

I am not being smart here... But for instance the stack size on a default Ubuntu dotnet core is a lot bigger than a default windows dotnet core (approx 4* the size) and it would be nice to use some of that huge stack... ;)

[GrabYourPitchforks]

@AndyAyersMS This could also be done as a compiler only change with no JIT support. There are a few ways to pull this off depending on available resources and requirements.

@Drawaes TBD implementation detail that shouldn't really affect the proposal. The nice thing is that as an implementation detail the developer never has to be bothered with it. :)

[Tornhoof]

But you know us developers, if that stack size is too conservative/low, we won't use the API and go the manual route again. But you're obviously correct that it is an Implementation detail.

[GrabYourPitchforks]

@Tornhoof Yup, but to be honest I'm not too worried about that. The folks commenting in this thread are self-selecting from the highest group of enthusiast / advanced developers. You're always going to have scenarios that will be better served by something custom that can take advantage of domain-specific knowledge. In those cases you're not the target audience of these APIs. And that's perfectly ok! Use the APIs under consideration at https://github.com/dotnet/corefx/issues/26954 if that's more applicable to your scenario. This specific issue is for the 99% use case. If needed we can always tweak to try to make it encompass the 99.9% use case. But KISS is the driving motivation here, and that can't be compromised.

[Drawaes]

The warning in the compiler needs to go in quick... as this API might have a process to go through, how about split the warning out and get it pushed through I think the warning is super important! Aka a rosyln analyser

[svick]

@Drawaes In that case, I think an issue in dotnet/roslyn should be opened for that.

[jaredpar]

Second, once the utility method is in place, the compiler should be smart enough to treat return Span.Alloc(...); as a compile-time error, just as it does today for Span span = stackalloc T[...]; return span;. This would require a compiler change.

One case I didn't see called out in the proposal is how to deal with older compilers. Take C# 7.2 which shipped with full Span<T> support. To that compiler the return from the Alloc method is freely returnable. Have to guard against that case.

Best way to prevent this is to add a modreq to Span<T>::Alloc. Modreq is designed exactly for this purpose and will prevent older compilers from using it.

Making this a proper compiler warning is probably a bit too noisy, so it could be a Roslyn code analyzer instead.

The compiler team does not differentiate between adding new warnings and adding new errors to the compiler. We treat all new diagnostics as errors (too many customers correctly use /warnaserror). This won't come close to meeting our bar for a new diagnostic. Needs to be an opt-in analyzer.

[GrabYourPitchforks]

@krwq recently wrote a tool that pings the security team whenever stackalloc is introduced in a PR, so we can keep an eye out for this pattern. I'll link instances of the potentially dangerous pattern to this work item so that we can track how commonplace this is in the wild.

dotnet/corefx#31044 (comment)

[tannergooding]

Just as a note, the threshold for _malloca on Windows is #define _ALLOCA_S_THRESHOLD 1024. While the same size may be too big/small for managed code or for non-Windows platforms, it at least gives a rough starting point.

For reference: double.Parse currently does a 769 byte stack allocation for its purposes and so around this may be reasonable for non-recursive methods.

[GrabYourPitchforks]

I experimented with this a little bit recently, and from the runtime perspective it turns out we can make it work without too much pain. The biggest problem is what Jared pointed out earlier: the C# compiler won't restrict these spans from escaping out of the local method. So if you use it you have to be disciplined about it without relying on compiler support. At the moment that seems strictly worse.

[tannergooding]

At the moment that seems strictly worse.

Agreed.

[jkotas]

dotnet/csharplang#1130 ?

[benaadams]

Also stackalloc ref types to Span<T> receivers #46104 (comment) and #38677 (the array rather than the objects themselves); rather than having to use ArrayPool for small object Span/Arrays that are then passed to another method (issue with for params Span<T> also?)

[GrabYourPitchforks]

I'm closing this in favor of #52065, which proposes more concrete APIs.

@tannergooding - check out the original post at the top of this issue for more context. I assume your proposal argues for similar behavior.

[benaadams]

I'm closing this in favor of #25423, which proposes more concrete APIs.

That's this issue? Stack recursion..?

[GrabYourPitchforks]

Bah, you're no fun. :)