Runtime: ArrayPool.Shared.Rent should support returning a zero-inited array

[GrabYourPitchforks]

Currently calling ArrayPool<T>.Shared.Rent does not guarantee that the returned buffer is zero-inited. While good for performance, this could cause correctness issues in applications, especially those that simply use byte[] xyz = ArrayPool<byte>.Shared.Rent(...) as a drop-in replacement for byte[] xyz = new byte[...].

I propose changing the signature of ArrayPool.Rent(int minimumLength) to ArrayPool.Rent(int minimumLength, bool clearArray = true). Thus the default behavior is to zero-init the returned array, and consumers who want the highest possible performance and don't care about the initial values can pass _false_ for this parameter. (I know we shouldn't default optional arguments to anything other than _default(T)_. Haven't fully reconciled this API design mentally yet.)

[jkotas]

ArrayPool is fundamentally unsecure API, just like manual malloc/free in C++. You have to be very careful when using it. It cannot be used as a drop-in replacement for byte[] xyz = new byte[...] without a very careful thinking.

[GrabYourPitchforks]

There's an awful lot of callers for something that's fundamentally an unsecure API, and that's just internal to CoreFX and doesn't include ASP.NET or other external callers. That's a huge indicator to me that developers are treating this is a faster drop-in replacement for array allocation, regardless of how we intended the APIs to be used.

[jkotas]

Well, Unsafe.* or Marshal.* are fundamentally unsecure in similar way and they are used way more often in corefx than ArrayPool...

[GrabYourPitchforks]

Additionally, I searched GitHub for commits to third party libraries where the commit message contains "ArrayPool", and it looks like developers are just treating it as a drop-in replacement. In fact, the _third match_ for that search term is this ImageSharp commit, which was a drop-in replacement, and their code was previously relying on that array being zero-inited.

I don't think developers know that this is an unsafe thing. There's no indication in the API that it's unsafe.

[jkotas]

this ImageSharp commit

This commit does not quality as drop in replacement for me. There are number of extra changes to make it work - explicit Dispose methods added, some arguments are passed as ref now, ... .

[jkotas]

I don't think developers know that this is an unsafe thing

Then we need to update the documentation, or the Intelisense tip you get for this to make it clear.

[morganbr]

@GrabYourPitchforks, I guess it's worth thinking about whether zero-initialization makes this API more secure in given that a previous caller can also hold onto a released array and get similar results. If we can make a case that they're different, I think exposing zero initialization as a choice makes sense. We could also have a pair of methods like
ArrayPool.Rent() (which zero-fills) and ArrayPool.UnsafeRentWithoutInitialization() which is explicit about the dangers.

[GrabYourPitchforks]

To be perfectly honest this is a gut feel about the future more than anything, as the API is still too new for me to have sufficient evidence that people are actively misusing it today. In an ideal world we could rely solely on developer documentation, but being developers ourselves we know that people rarely read documentation. :)

Furthermore, Karel showed me offline that this API has already shipped. So instead of changing the signature of the existing _Rent_ method, the proposal would be to add an overload that also accepts a _bool zeroFill_ parameter. (Or per Morgan's suggestion have an Unsafe-named method.)

To recap: my assumptions are (a) that this API will turn out to be a pit of failure because developers will use it for something it's not intended to be used for, and (b) we don't have a good way of communicating to developers the right way of using this API and ensuring that the audience sees it. If these assumptions are incorrect then there's no real need for this work item.

[jkotas]

So instead of changing the signature of the existing Rent method, the proposal would be to add an overload that also accepts a bool zeroFill parameter.

It is still a unaceptable behavior/performance change. It is not binary breaking change, but still pretty significant.

[GrabYourPitchforks]

What would move this into the "acceptable" category? What's the particular bar we need to meet?

[jkotas]

What's the particular bar we need to meet?

The existing apps using this API cannot get silently significantly slower by just recompiling against new version.

What would move this into the "acceptable" category?

Deprecate the existing API and create a new one.

[jkotas]

There are number of other things that are less than ideal about the ArrayPool:

• You have to fetch a static and invoke a virtual method to do anything - both of which are relatively expensive operations
• It would be better for the API to rent System.Memory instead of array. It would allow more efficient underlying implementation. E.g. the underlying implementation would be able to reuse space for int[] and byte[].
• It would be nice to have a option for a life-time tracking (borrow the ideas from Snowflake project) that would guarantee safety and avoid use-after-free (perhaps in combination with supporting Roslyn analyzer). It would be superior to just the zero-filling idea discussed here.

If we are rethinking how ArrayPool works, we may want to address all of them.

[danmosemsft]

Adding ArrayPool.Rent(int minimumLength, bool clearArray) (without default) is surely not a breaking change.

[jkotas]

Agree.

[morganbr]

It would also be non-breaking (and IMO a better choice for security) to change ArrayPool.Rent to always zero and introduce a new method to explicitly allow non-zeroing either via name or parameter. Of course, we'd be free to review framework code that uses ArrayPool.Rent to decide whether to move it to the new method to mitigate any performance losses.

[davidfowl]

It’s worth noting that we’re working on a more general purpose MemoryPool abstraction that will be more flexiable and should address some of @jkotas concerns.

[jkotas]

Good. So MemoryPool is where we should make the zero-initialize by default change if we believe that it is a good idea.

[safern]

Due to @jkotas concerns and final decision that MemoryPool should be the place of making this by default we should either close this issue or turn it into an API proposal to add something like ArrayPool.Rent(int minimumLength, bool clearArray) as @danmosemsft mentioned if we want the option to have a zero-initialized ArrayPool when renting it.

[danmosemsft]

We should also look at these suggestions above when we do that.
https://github.com/dotnet/corefx/issues/25843#issuecomment-351226440

[GrabYourPitchforks]

There's also resistance to making this change in MemoryPool. If I can collect experimental numbers to demonstrate that this has negligible performance impact, would people be willing to reconsider this issue?

[koszeggy]

I just wanted to suggest the same API change right now. I hope it will be implemented soon.

I have rather performance grounds than security reasons: If I use the shared pool I cannot be sure whether the array I get is clean. Of course, if I need a clean array I can call Array.Clear also on rented ones but if the array was newly created by the pool, which I cannot tell, then calling Clear at my side is a pure waste of performance.

[GrabYourPitchforks]

@koszeggy even "new" arrays returned by the array pool are not guaranteed to be zero inited. You'd still have to clear manually anyway. See https://github.com/dotnet/coreclr/pull/24504 for more details.

[GrabYourPitchforks]

Going through some old stale bugs. Seems that this is probably a non-actionable issue and should be closed?