RecyclableMemoryStream exhausts all available RAM if requestedSize > (int.MaxValue - BlockSize) about microsoft.io.recyclablememorystream

[doubleyewdee]

Making Capacity long isn't possible (note it's an override). Writing a repro test currently so I can validate a fix.

[elgonzo]

I didn't notice that Capacity is an overridden property from MemoryStream when i encountered the problem. Makes me wonder why the property RecyclableMemoryStreamManager.MaximumStreamCapacity is a long, though. Shouldn't it be an int as well? (Unless you plan to introduce something like RecyclableMemoryStream.CapacityLong, with the overridden MemoryStream.Capacity value being clamped to [0,int.MaxValue], perhaps. But that on the other hand might also perhaps cause bizarre problems for programs who rely on MemoryStream properties such as Position or Length always being <= int.MaxValue. Hmm...)

[elgonzo]

Unfortunately, there is still an issue when RecyclableMemoryStream with a size close to or exactly of int.MaxValue is being requested.

The commit from June 30th attempts clamping the property RecyclableMemoryStream.Capacity to prevent it from overflowing:

long size = this.blocks.Count * this.memoryManager.BlockSize;
return (int)Math.Min(int.MaxValue, size);

However, the calculation of the long variable size is incorrect. It will multiply two int values, and will only convert the int result to an long value. Thus, the overflow can still happen and the variable size becoming negative in the if the block count is becoming sufficiently high.

An example:
When requesting a RecyclableMemoryStream with size of int.MaxValue, the block count will eventually reach 16384 (assuming the standard block size of 131072).

The int multiplication 16384*131072 will result in the size variable (and in consequence the Capacity property) having a negative value of -2147483648. This will break the while loop in the method RecyclableMemoryStream.EnsureCapacity, which again would lead to exhaustion of the available virtual memory.

---

Suggested fix:

To fix the issue, the multiplication needs to be forced to operate on longs. Casting one or both of the multiplicands/multipliers as a long will do this:

long size = (long) this.blocks.Count * this.memoryManager.BlockSize;
return (int)Math.Min(int.MaxValue, size);

---

Additionally, the test method GiantAllocationSucceeds is not able to find this issue.
To detect this issue (using the default block size of 128KB), the requested stream size must be at least int.MaxValue - (mgr.BlockSize + 2) .

The reason why the unit test fails to find this issue is the following: int.MaxValue equals 0x7fffffff. To satisfy a requested stream size of 0x7fffffff with the default block size of 128KB (=0x20000), 16384 blocks are required. Note that the last block is not fully used, its used capacity is 0x1ffff.

However, the largest stream size requested by the unit test is only int.MaxValue - (mgr.BlockSize + 1).
(mgr.BlockSize + 1) equals 0x1ffff - which means only 16383 blocks are needed.

With only needing 16383 blocks, the unit test fails to test a scenario where the RecyclableMemoryStream.Capacity property could overflow (i.e., scenarios where 16384 blocks would be needed).

I would suggest to let the unit test request a stream size of exactly int.MaxValue to make sure that the clamping of the Capacity property works. If it works with a stream size of int.MaxValue then it should also work with any other stream sizes (as they can only be less than int.MaxValue).

[doubleyewdee]

Pretty sure the PR above resolves this per the comment (thank you for the excellent diagnosis). Will give a few days for someone to review before just merging in case any concerns arise. Thanks!