Skip to content

Commit

Permalink
Change exception for overflow in ArrayBufferWriter (#32587)
Browse files Browse the repository at this point in the history
* Fix #609 - Part 2
We throw a OutOfMemoryException instead of "Arithmetic operation resulted in an overflow"

* Test attributes. We don't need to limit to 64bit process, since the memory is never allocated.

* Remove platform restriction

* Explicitly throw the OutOfMemoryException instead of rely on Array.Resize

* Move ThrowOutOfMemoryException method

* Missing message, BufferMaximumSizeExceeded

* Code Coverage

* Resource file

* Missing Resx update

* Restrict test to 64bits environment

* Simple test so that we have some test coverage in inner loop
Since we are trying to avoid the OOM, don't run the GetMemory_ExceedMaximumBufferSize on Linux

* Test comment

* Move OutOfMemory test to byte specific test
  • Loading branch information
felipepessoto authored Mar 25, 2020
1 parent 25fdaa8 commit ab3d9ae
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 6 deletions.
21 changes: 18 additions & 3 deletions src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,24 @@ private void CheckAndResizeBuffer(int sizeHint)

if (sizeHint > FreeCapacity)
{
int growBy = Math.Max(sizeHint, _buffer.Length);
int currentLength = _buffer.Length;
int growBy = Math.Max(sizeHint, currentLength);

if (_buffer.Length == 0)
if (currentLength == 0)
{
growBy = Math.Max(growBy, DefaultInitialBufferSize);
}

int newSize = checked(_buffer.Length + growBy);
int newSize = currentLength + growBy;

if ((uint)newSize > int.MaxValue)
{
newSize = currentLength + sizeHint;
if ((uint)newSize > int.MaxValue)
{
ThrowOutOfMemoryException((uint)newSize);
}
}

Array.Resize(ref _buffer, newSize);
}
Expand All @@ -186,5 +196,10 @@ private static void ThrowInvalidOperationException_AdvancedTooFar(int capacity)
{
throw new InvalidOperationException(SR.Format(SR.BufferWriterAdvancedTooFar, capacity));
}

private static void ThrowOutOfMemoryException(uint capacity)
{
throw new OutOfMemoryException(SR.Format(SR.BufferMaximumSizeExceeded, capacity));
}
}
}
8 changes: 6 additions & 2 deletions src/libraries/System.Memory/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<root>
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema
Expand Down Expand Up @@ -146,4 +147,7 @@
<data name="BufferWriterAdvancedTooFar" xml:space="preserve">
<value>Cannot advance past the end of the buffer, which has a size of {0}.</value>
</data>
</root>
<data name="BufferMaximumSizeExceeded" xml:space="preserve">
<value>Cannot allocate a buffer of size {0}.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -89,5 +89,21 @@ public async Task WriteAndCopyToStreamAsync()
Assert.Equal(outputMemory.Length, streamOutput.Length);
Assert.True(outputMemory.Span.SequenceEqual(streamOutput));
}

// NOTE: GetMemory_ExceedMaximumBufferSize test is constrained to run on Windows and MacOSX because it causes
// problems on Linux due to the way deferred memory allocation works. On Linux, the allocation can
// succeed even if there is not enough memory but then the test may get killed by the OOM killer at the
// time the memory is accessed which triggers the full memory allocation.
[PlatformSpecific(TestPlatforms.Windows | TestPlatforms.OSX)]
[ConditionalFact(nameof(IsX64))]
[OuterLoop]
public void GetMemory_ExceedMaximumBufferSize()
{
var output = new ArrayBufferWriter<byte>(int.MaxValue / 2 + 1);
output.Advance(int.MaxValue / 2 + 1);
Memory<byte> memory = output.GetMemory(1); // Validate we can't double the buffer size, but can grow by sizeHint
Assert.Equal(1, memory.Length);
Assert.Throws<OutOfMemoryException>(() => output.GetMemory(int.MaxValue));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,13 @@ public void GetMemory_DefaultCtor(int sizeHint)
Assert.Equal(sizeHint <= 256 ? 256 : sizeHint, memory.Length);
}

[Fact]
public void GetMemory_ExceedMaximumBufferSize_WithSmallStartingSize()
{
var output = new ArrayBufferWriter<T>(256);
Assert.Throws<OutOfMemoryException>(() => output.GetMemory(int.MaxValue));
}

[Fact]
public void GetMemory_InitSizeCtor()
{
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Text.Json/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -503,4 +503,7 @@
<data name="ExtensionDataCannotBindToCtorParam" xml:space="preserve">
<value>The extension data property '{0}' on type '{1}' cannot bind with a parameter in constructor '{2}'.</value>
</data>
<data name="BufferMaximumSizeExceeded" xml:space="preserve">
<value>Cannot allocate a buffer of size {0}.</value>
</data>
</root>
5 changes: 4 additions & 1 deletion src/libraries/System.Text.Json/tests/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -20278,4 +20278,7 @@ tiline\"another\" String\\"],"str":"\"\""}</value>
<data name="LoremIpsum40WordsBase64" xml:space="preserve">
<value>TG9yZW0gaXBzdW0gZG9sb3Igc2l0IGFtZXQsIGNvbnNlY3RldHVyIGFkaXBpc2NpbmcgZWxpdC4gQ3VyYWJpdHVyIHZpdmVycmEgbmVxdWUgYXQgZXJhdCBsYW9yZWV0LCBhdCBzYWdpdHRpcyBhcmN1IGVmZmljaXR1ci4gVXQgcHVsdmluYXIgZXJvcyBuZWMgb2RpbyBjdXJzdXMgZWxlaWZlbmQuIE1hZWNlbmFzIHZpdmVycmEgZWxlbWVudHVtIHBvcnR0aXRvci4gTnVsbGFtIG1pIHZlbGl0LCBtYWxlc3VhZGEgY29tbW9kbyB0cmlzdGlxdWUgZXUsIG1vbGxpcyBhIHZlbGl0LiBNb3JiaS4=</value>
</data>
</root>
<data name="BufferMaximumSizeExceeded" xml:space="preserve">
<value>Cannot allocate a buffer of size {0}.</value>
</data>
</root>

0 comments on commit ab3d9ae

Please sign in to comment.