From a94d8147945256840d6af4552a1dad2bef2da711 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Sat, 20 Apr 2019 16:38:27 +0200 Subject: [PATCH 1/2] [SL.Core] Throw better exceptions from MemoryAllocators --- .../Memory/ArrayPoolMemoryAllocator.cs | 11 +++++ .../Memory/SimpleGcMemoryAllocator.cs | 7 +++ ...ts.cs => ArrayPoolMemoryAllocatorTests.cs} | 24 +++++++++- .../Memory/SimpleGcMemoryAllocatorTests.cs | 45 +++++++++++++++++++ .../Memory/SimpleGcMemoryManagerTests.cs | 16 ------- 5 files changed, 85 insertions(+), 18 deletions(-) rename tests/SixLabors.Core.Tests/Memory/{ArrayPoolMemoryManagerTests.cs => ArrayPoolMemoryAllocatorTests.cs} (88%) create mode 100644 tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryAllocatorTests.cs delete mode 100644 tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryManagerTests.cs diff --git a/src/SixLabors.Core/Memory/ArrayPoolMemoryAllocator.cs b/src/SixLabors.Core/Memory/ArrayPoolMemoryAllocator.cs index 8681a594b..0a5e47f28 100644 --- a/src/SixLabors.Core/Memory/ArrayPoolMemoryAllocator.cs +++ b/src/SixLabors.Core/Memory/ArrayPoolMemoryAllocator.cs @@ -1,6 +1,7 @@ // Copyright (c) Six Labors and contributors. // Licensed under the Apache License, Version 2.0. +using System; using System.Buffers; using System.Runtime.CompilerServices; @@ -91,8 +92,15 @@ namespace SixLabors.Memory /// public override IMemoryOwner Allocate(int length, AllocationOptions options = AllocationOptions.None) { + Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length)); int itemSizeBytes = Unsafe.SizeOf(); int bufferSizeInBytes = length * itemSizeBytes; + if (bufferSizeInBytes < 0) + { + throw new ArgumentOutOfRangeException( + nameof(length), + $"{nameof(ArrayPoolMemoryAllocator)} can not allocate {length} elements of {typeof(T).Name}."); + } ArrayPool pool = this.GetArrayPool(bufferSizeInBytes); byte[] byteArray = pool.Rent(bufferSizeInBytes); @@ -109,6 +117,9 @@ namespace SixLabors.Memory /// public override IManagedByteBuffer AllocateManagedByteBuffer(int length, AllocationOptions options = AllocationOptions.None) { + Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length)); + Guard.MustBeLessThan(length, int.MaxValue, nameof(length)); + ArrayPool pool = this.GetArrayPool(length); byte[] byteArray = pool.Rent(length); diff --git a/src/SixLabors.Core/Memory/SimpleGcMemoryAllocator.cs b/src/SixLabors.Core/Memory/SimpleGcMemoryAllocator.cs index cc1157979..fc7fb607f 100644 --- a/src/SixLabors.Core/Memory/SimpleGcMemoryAllocator.cs +++ b/src/SixLabors.Core/Memory/SimpleGcMemoryAllocator.cs @@ -1,6 +1,7 @@ // Copyright (c) Six Labors and contributors. // Licensed under the Apache License, Version 2.0. +using System; using System.Buffers; using SixLabors.Memory.Internals; @@ -14,12 +15,18 @@ namespace SixLabors.Memory /// public override IMemoryOwner Allocate(int length, AllocationOptions options = AllocationOptions.None) { + Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length)); + Guard.MustBeLessThan(length, int.MaxValue, nameof(length)); + return new BasicArrayBuffer(new T[length]); } /// public override IManagedByteBuffer AllocateManagedByteBuffer(int length, AllocationOptions options = AllocationOptions.None) { + Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length)); + Guard.MustBeLessThan(length, int.MaxValue, nameof(length)); + return new BasicByteBuffer(new byte[length]); } } diff --git a/tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryManagerTests.cs b/tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryAllocatorTests.cs similarity index 88% rename from tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryManagerTests.cs rename to tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryAllocatorTests.cs index 6e7efebb8..5226c9b01 100644 --- a/tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryManagerTests.cs +++ b/tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryAllocatorTests.cs @@ -12,7 +12,7 @@ using Xunit; namespace SixLabors.Memory.Tests { - public class ArrayPoolMemoryManagerTests + public class ArrayPoolMemoryAllocatorTests { private const int MaxPooledBufferSizeInBytes = 2048; @@ -231,9 +231,29 @@ namespace SixLabors.Memory.Tests private uint dummy; } - [StructLayout(LayoutKind.Explicit, Size = MaxPooledBufferSizeInBytes / 5)] + private const int SizeOfLargeStruct = MaxPooledBufferSizeInBytes / 5; + + [StructLayout(LayoutKind.Explicit, Size = SizeOfLargeStruct)] private struct LargeStruct { } + + [Theory] + [InlineData(-1)] + [InlineData((int.MaxValue / SizeOfLargeStruct) + 1)] + public void AllocateIncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length) + { + var ex = Assert.Throws(() => this.MemoryAllocator.Allocate(length)); + Assert.Equal("length", ex.ParamName); + } + + [Theory] + [InlineData(-1)] + [InlineData(int.MaxValue)] + public void AllocateManagedByteBuffer_IncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length) + { + var ex = Assert.Throws(() => this.MemoryAllocator.AllocateManagedByteBuffer(length)); + Assert.Equal("length", ex.ParamName); + } } } \ No newline at end of file diff --git a/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryAllocatorTests.cs b/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryAllocatorTests.cs new file mode 100644 index 000000000..96f92e682 --- /dev/null +++ b/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryAllocatorTests.cs @@ -0,0 +1,45 @@ +// Copyright (c) Six Labors and contributors. +// Licensed under the Apache License, Version 2.0. + +using System; +using System.Runtime.InteropServices; +using Xunit; + +namespace SixLabors.Memory.Tests +{ + public class SimpleGcMemoryAllocatorTests + { + public class BufferTests : BufferTestSuite + { + public BufferTests() + : base(new SimpleGcMemoryAllocator()) + { + } + } + + protected SimpleGcMemoryAllocator MemoryAllocator { get; } = new SimpleGcMemoryAllocator(); + + [Theory] + [InlineData(-1)] + [InlineData(int.MaxValue)] + public void Allocate_IncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length) + { + var ex = Assert.Throws(() => this.MemoryAllocator.Allocate( length)); + Assert.Equal("length", ex.ParamName); + } + + [Theory] + [InlineData(-1)] + [InlineData(int.MaxValue)] + public void AllocateManagedByteBuffer_IncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length) + { + var ex = Assert.Throws(() => this.MemoryAllocator.AllocateManagedByteBuffer(length)); + Assert.Equal("length", ex.ParamName); + } + + [StructLayout(LayoutKind.Explicit, Size = 512)] + private struct BigStruct + { + } + } +} \ No newline at end of file diff --git a/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryManagerTests.cs b/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryManagerTests.cs deleted file mode 100644 index a6ddeb050..000000000 --- a/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryManagerTests.cs +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright (c) Six Labors and contributors. -// Licensed under the Apache License, Version 2.0. - -namespace SixLabors.Memory.Tests -{ - public class SimpleGcMemoryManagerTests - { - public class BufferTests : BufferTestSuite - { - public BufferTests() - : base(new SimpleGcMemoryAllocator()) - { - } - } - } -} \ No newline at end of file From e21eb5d18f7074ff022373dc068f52dbb85a351b Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Sun, 21 Apr 2019 00:56:40 +0200 Subject: [PATCH 2/2] [SL.Core] drop meaningless upper limit check --- src/SixLabors.Core/Memory/ArrayPoolMemoryAllocator.cs | 1 - src/SixLabors.Core/Memory/SimpleGcMemoryAllocator.cs | 2 -- .../Memory/ArrayPoolMemoryAllocatorTests.cs | 1 - .../SixLabors.Core.Tests/Memory/SimpleGcMemoryAllocatorTests.cs | 2 -- 4 files changed, 6 deletions(-) diff --git a/src/SixLabors.Core/Memory/ArrayPoolMemoryAllocator.cs b/src/SixLabors.Core/Memory/ArrayPoolMemoryAllocator.cs index 0a5e47f28..113e12785 100644 --- a/src/SixLabors.Core/Memory/ArrayPoolMemoryAllocator.cs +++ b/src/SixLabors.Core/Memory/ArrayPoolMemoryAllocator.cs @@ -118,7 +118,6 @@ namespace SixLabors.Memory public override IManagedByteBuffer AllocateManagedByteBuffer(int length, AllocationOptions options = AllocationOptions.None) { Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length)); - Guard.MustBeLessThan(length, int.MaxValue, nameof(length)); ArrayPool pool = this.GetArrayPool(length); byte[] byteArray = pool.Rent(length); diff --git a/src/SixLabors.Core/Memory/SimpleGcMemoryAllocator.cs b/src/SixLabors.Core/Memory/SimpleGcMemoryAllocator.cs index fc7fb607f..acf17ad63 100644 --- a/src/SixLabors.Core/Memory/SimpleGcMemoryAllocator.cs +++ b/src/SixLabors.Core/Memory/SimpleGcMemoryAllocator.cs @@ -16,7 +16,6 @@ namespace SixLabors.Memory public override IMemoryOwner Allocate(int length, AllocationOptions options = AllocationOptions.None) { Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length)); - Guard.MustBeLessThan(length, int.MaxValue, nameof(length)); return new BasicArrayBuffer(new T[length]); } @@ -25,7 +24,6 @@ namespace SixLabors.Memory public override IManagedByteBuffer AllocateManagedByteBuffer(int length, AllocationOptions options = AllocationOptions.None) { Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length)); - Guard.MustBeLessThan(length, int.MaxValue, nameof(length)); return new BasicByteBuffer(new byte[length]); } diff --git a/tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryAllocatorTests.cs b/tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryAllocatorTests.cs index 5226c9b01..e59590bae 100644 --- a/tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryAllocatorTests.cs +++ b/tests/SixLabors.Core.Tests/Memory/ArrayPoolMemoryAllocatorTests.cs @@ -249,7 +249,6 @@ namespace SixLabors.Memory.Tests [Theory] [InlineData(-1)] - [InlineData(int.MaxValue)] public void AllocateManagedByteBuffer_IncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length) { var ex = Assert.Throws(() => this.MemoryAllocator.AllocateManagedByteBuffer(length)); diff --git a/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryAllocatorTests.cs b/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryAllocatorTests.cs index 96f92e682..86e71717e 100644 --- a/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryAllocatorTests.cs +++ b/tests/SixLabors.Core.Tests/Memory/SimpleGcMemoryAllocatorTests.cs @@ -21,7 +21,6 @@ namespace SixLabors.Memory.Tests [Theory] [InlineData(-1)] - [InlineData(int.MaxValue)] public void Allocate_IncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length) { var ex = Assert.Throws(() => this.MemoryAllocator.Allocate( length)); @@ -30,7 +29,6 @@ namespace SixLabors.Memory.Tests [Theory] [InlineData(-1)] - [InlineData(int.MaxValue)] public void AllocateManagedByteBuffer_IncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length) { var ex = Assert.Throws(() => this.MemoryAllocator.AllocateManagedByteBuffer(length));