Skip to content

Commit

Permalink
Fix args and add tests
Browse files Browse the repository at this point in the history
  • Loading branch information
JimBobSquarePants committed Apr 3, 2024
1 parent 7dd3c43 commit 63d4b20
Show file tree
Hide file tree
Showing 21 changed files with 131 additions and 71 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public ComponentProcessor(MemoryAllocator memoryAllocator, JpegFrame frame, Size
this.Component = component;

this.BlockAreaSize = component.SubSamplingDivisors * blockSize;
this.ColorBuffer = memoryAllocator.Allocate2DOveraligned<float>(
this.ColorBuffer = memoryAllocator.Allocate2DOverAligned<float>(
postProcessorBufferSize.Width,
postProcessorBufferSize.Height,
this.BlockAreaSize.Height);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public ComponentProcessor(MemoryAllocator memoryAllocator, Component component,
this.blockAreaSize = component.SubSamplingDivisors * 8;

// alignment of 8 so each block stride can be sampled from a single 'ref pointer'
this.ColorBuffer = memoryAllocator.Allocate2DOveraligned<float>(
this.ColorBuffer = memoryAllocator.Allocate2DOverAligned<float>(
postProcessorBufferSize.Width,
postProcessorBufferSize.Height,
8,
Expand Down
3 changes: 3 additions & 0 deletions src/ImageSharp/Formats/Png/PngDecoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1968,6 +1968,9 @@ private IMemoryOwner<byte> ReadChunkData(int length)
}

// We rent the buffer here to return it afterwards in Decode()
// We don't want to throw a degenerated memory exception here as we want to allow partial decoding
// so limit the length.
length = (int)Math.Min(length, this.currentStream.Length - this.currentStream.Position);
IMemoryOwner<byte> buffer = this.configuration.MemoryAllocator.Allocate<byte>(length, AllocationOptions.Clean);

this.currentStream.Read(buffer.GetSpan(), 0, length);
Expand Down
2 changes: 1 addition & 1 deletion src/ImageSharp/Formats/Tga/TgaDecoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken
throw new UnknownImageFormatException("Width or height cannot be 0");
}

Image<TPixel> image = Image.CreateUninitialized<TPixel>(this.configuration, this.fileHeader.Width, this.fileHeader.Height, this.metadata);
Image<TPixel> image = new(this.configuration, this.fileHeader.Width, this.fileHeader.Height, this.metadata);
Buffer2D<TPixel> pixels = image.GetRootFramePixelBuffer();

if (this.fileHeader.ColorMapType == 1)
Expand Down
21 changes: 8 additions & 13 deletions src/ImageSharp/Memory/Allocators/MemoryAllocator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Six Labors Split License.

using System.Buffers;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;

namespace SixLabors.ImageSharp.Memory;
Expand All @@ -24,9 +23,7 @@ public abstract class MemoryAllocator
/// </summary>
public static MemoryAllocator Default { get; } = Create();

internal long MemoryGroupAllocationLimitBytes { get; private set; } = Environment.Is64BitProcess ?
4L * OneGigabyte :
OneGigabyte;
internal long MemoryGroupAllocationLimitBytes { get; private set; } = Environment.Is64BitProcess ? 4L * OneGigabyte : OneGigabyte;

internal int SingleBufferAllocationLimitBytes { get; private set; } = OneGigabyte;

Expand Down Expand Up @@ -82,6 +79,7 @@ public virtual void ReleaseRetainedResources()
/// <summary>
/// Allocates a <see cref="MemoryGroup{T}"/>.
/// </summary>
/// <typeparam name="T">The type of element to allocate.</typeparam>
/// <param name="totalLength">The total length of the buffer.</param>
/// <param name="bufferAlignment">The expected alignment (eg. to make sure image rows fit into single buffers).</param>
/// <param name="options">The <see cref="AllocationOptions"/>.</param>
Expand All @@ -93,22 +91,19 @@ internal MemoryGroup<T> AllocateGroup<T>(
AllocationOptions options = AllocationOptions.None)
where T : struct
{
long totalLengthInBytes = totalLength * Unsafe.SizeOf<T>();
if (totalLengthInBytes < 0)
if (totalLength < 0)
{
ThrowNotRepresentable();
InvalidMemoryOperationException.ThrowNegativeAllocationException(totalLength);
}

if (totalLengthInBytes > this.MemoryGroupAllocationLimitBytes)
ulong totalLengthInBytes = (ulong)totalLength * (ulong)Unsafe.SizeOf<T>();
if (totalLengthInBytes > (ulong)this.MemoryGroupAllocationLimitBytes)
{
InvalidMemoryOperationException.ThrowAllocationOverLimitException(totalLengthInBytes, this.MemoryGroupAllocationLimitBytes);
}

return this.AllocateGroupCore<T>(totalLengthInBytes, totalLength, bufferAlignment, options);

[DoesNotReturn]
static void ThrowNotRepresentable() =>
throw new InvalidMemoryOperationException("Attempted to allocate a MemoryGroup of a size that is not representable.");
// Cast to long is safe because we already checked that the total length is within the limit.
return this.AllocateGroupCore<T>(totalLength, (long)totalLengthInBytes, bufferAlignment, options);
}

internal virtual MemoryGroup<T> AllocateGroupCore<T>(long totalLengthInElements, long totalLengthInBytes, int bufferAlignment, AllocationOptions options)
Expand Down
4 changes: 2 additions & 2 deletions src/ImageSharp/Memory/Allocators/MemoryAllocatorOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public struct MemoryAllocatorOptions
/// </summary>
public int? MaximumPoolSizeMegabytes
{
get => this.maximumPoolSizeMegabytes;
readonly get => this.maximumPoolSizeMegabytes;
set
{
if (value.HasValue)
Expand All @@ -35,7 +35,7 @@ public int? MaximumPoolSizeMegabytes
/// </summary>
public int? AllocationLimitMegabytes
{
get => this.allocationLimitMegabytes;
readonly get => this.allocationLimitMegabytes;
set
{
if (value.HasValue)
Expand Down
10 changes: 6 additions & 4 deletions src/ImageSharp/Memory/Allocators/SimpleGcMemoryAllocator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ public sealed class SimpleGcMemoryAllocator : MemoryAllocator
/// <inheritdoc />
public override IMemoryOwner<T> Allocate<T>(int length, AllocationOptions options = AllocationOptions.None)
{
Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length));

int lengthInBytes = length * Unsafe.SizeOf<T>();
if (length < 0)
{
InvalidMemoryOperationException.ThrowNegativeAllocationException(length);
}

if (lengthInBytes > this.SingleBufferAllocationLimitBytes)
ulong lengthInBytes = (ulong)length * (ulong)Unsafe.SizeOf<T>();
if (lengthInBytes > (ulong)this.SingleBufferAllocationLimitBytes)
{
InvalidMemoryOperationException.ThrowAllocationOverLimitException(lengthInBytes, this.SingleBufferAllocationLimitBytes);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Six Labors Split License.

using System.Buffers;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using SixLabors.ImageSharp.Memory.Internals;

Expand Down Expand Up @@ -84,15 +83,18 @@ public override IMemoryOwner<T> Allocate<T>(
int length,
AllocationOptions options = AllocationOptions.None)
{
Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length));
int lengthInBytes = length * Unsafe.SizeOf<T>();
if (length < 0)
{
InvalidMemoryOperationException.ThrowNegativeAllocationException(length);
}

if (lengthInBytes > this.SingleBufferAllocationLimitBytes)
ulong lengthInBytes = (ulong)length * (ulong)Unsafe.SizeOf<T>();
if (lengthInBytes > (ulong)this.SingleBufferAllocationLimitBytes)
{
InvalidMemoryOperationException.ThrowAllocationOverLimitException(lengthInBytes, this.SingleBufferAllocationLimitBytes);
}

if (lengthInBytes <= this.sharedArrayPoolThresholdInBytes)
if (lengthInBytes <= (ulong)this.sharedArrayPoolThresholdInBytes)
{
var buffer = new SharedArrayPoolBuffer<T>(length);
if (options.Has(AllocationOptions.Clean))
Expand All @@ -103,7 +105,7 @@ public override IMemoryOwner<T> Allocate<T>(
return buffer;
}

if (lengthInBytes <= this.poolBufferSizeInBytes)
if (lengthInBytes <= (ulong)this.poolBufferSizeInBytes)
{
UnmanagedMemoryHandle mem = this.pool.Rent();
if (mem.IsValid)
Expand Down
13 changes: 7 additions & 6 deletions src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,16 @@ public static MemoryGroup<T> Allocate(
{
int bufferCapacityInBytes = allocator.GetBufferCapacityInBytes();
Guard.NotNull(allocator, nameof(allocator));
Guard.MustBeGreaterThanOrEqualTo(totalLengthInElements, 0, nameof(totalLengthInElements));
Guard.MustBeGreaterThanOrEqualTo(bufferAlignmentInElements, 0, nameof(bufferAlignmentInElements));

int blockCapacityInElements = bufferCapacityInBytes / ElementSize;
if (totalLengthInElements < 0)
{
InvalidMemoryOperationException.ThrowNegativeAllocationException(totalLengthInElements);
}

if (bufferAlignmentInElements > blockCapacityInElements)
int blockCapacityInElements = bufferCapacityInBytes / ElementSize;
if (bufferAlignmentInElements < 0 || bufferAlignmentInElements > blockCapacityInElements)
{
throw new InvalidMemoryOperationException(
$"The buffer capacity of the provided MemoryAllocator is insufficient for the requested buffer alignment: {bufferAlignmentInElements}.");
InvalidMemoryOperationException.ThrowInvalidAlignmentException(bufferAlignmentInElements);
}

if (totalLengthInElements == 0)
Expand Down
11 changes: 10 additions & 1 deletion src/ImageSharp/Memory/InvalidMemoryOperationException.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,15 @@ public InvalidMemoryOperationException()
}

[DoesNotReturn]
internal static void ThrowAllocationOverLimitException(long length, long limit) =>
internal static void ThrowNegativeAllocationException(long length) =>
throw new InvalidMemoryOperationException($"Attempted to allocate a buffer of negative length={length}.");

[DoesNotReturn]
internal static void ThrowInvalidAlignmentException(long alignment) =>
throw new InvalidMemoryOperationException(
$"The buffer capacity of the provided MemoryAllocator is insufficient for the requested buffer alignment: {alignment}.");

[DoesNotReturn]
internal static void ThrowAllocationOverLimitException(ulong length, long limit) =>
throw new InvalidMemoryOperationException($"Attempted to allocate a buffer of length={length} that exceeded the limit {limit}.");
}
14 changes: 7 additions & 7 deletions src/ImageSharp/Memory/MemoryAllocatorExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,20 @@ public static class MemoryAllocatorExtensions
/// <param name="memoryAllocator">The memory allocator.</param>
/// <param name="width">The buffer width.</param>
/// <param name="height">The buffer height.</param>
/// <param name="preferContiguosImageBuffers">A value indicating whether the allocated buffer should be contiguous, unless bigger than <see cref="int.MaxValue"/>.</param>
/// <param name="preferContiguousImageBuffers">A value indicating whether the allocated buffer should be contiguous, unless bigger than <see cref="int.MaxValue"/>.</param>
/// <param name="options">The allocation options.</param>
/// <returns>The <see cref="Buffer2D{T}"/>.</returns>
public static Buffer2D<T> Allocate2D<T>(
this MemoryAllocator memoryAllocator,
int width,
int height,
bool preferContiguosImageBuffers,
bool preferContiguousImageBuffers,
AllocationOptions options = AllocationOptions.None)
where T : struct
{
long groupLength = (long)width * height;
MemoryGroup<T> memoryGroup;
if (preferContiguosImageBuffers && groupLength < int.MaxValue)
if (preferContiguousImageBuffers && groupLength < int.MaxValue)
{
IMemoryOwner<T> buffer = memoryAllocator.Allocate<T>((int)groupLength, options);
memoryGroup = MemoryGroup<T>.CreateContiguous(buffer, false);
Expand Down Expand Up @@ -69,16 +69,16 @@ public static Buffer2D<T> Allocate2D<T>(
/// <typeparam name="T">The type of buffer items to allocate.</typeparam>
/// <param name="memoryAllocator">The memory allocator.</param>
/// <param name="size">The buffer size.</param>
/// <param name="preferContiguosImageBuffers">A value indicating whether the allocated buffer should be contiguous, unless bigger than <see cref="int.MaxValue"/>.</param>
/// <param name="preferContiguousImageBuffers">A value indicating whether the allocated buffer should be contiguous, unless bigger than <see cref="int.MaxValue"/>.</param>
/// <param name="options">The allocation options.</param>
/// <returns>The <see cref="Buffer2D{T}"/>.</returns>
public static Buffer2D<T> Allocate2D<T>(
this MemoryAllocator memoryAllocator,
Size size,
bool preferContiguosImageBuffers,
bool preferContiguousImageBuffers,
AllocationOptions options = AllocationOptions.None)
where T : struct =>
Allocate2D<T>(memoryAllocator, size.Width, size.Height, preferContiguosImageBuffers, options);
Allocate2D<T>(memoryAllocator, size.Width, size.Height, preferContiguousImageBuffers, options);

/// <summary>
/// Allocates a buffer of value type objects interpreted as a 2D region
Expand All @@ -96,7 +96,7 @@ public static Buffer2D<T> Allocate2D<T>(
where T : struct =>
Allocate2D<T>(memoryAllocator, size.Width, size.Height, false, options);

internal static Buffer2D<T> Allocate2DOveraligned<T>(
internal static Buffer2D<T> Allocate2DOverAligned<T>(
this MemoryAllocator memoryAllocator,
int width,
int height,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ private ResizeKernelMap(
this.sourceLength = sourceLength;
this.DestinationLength = destinationLength;
this.MaxDiameter = (radius * 2) + 1;
this.data = memoryAllocator.Allocate2D<float>(this.MaxDiameter, bufferHeight, preferContiguosImageBuffers: true, AllocationOptions.Clean);
this.data = memoryAllocator.Allocate2D<float>(this.MaxDiameter, bufferHeight, preferContiguousImageBuffers: true, AllocationOptions.Clean);
this.pinHandle = this.data.DangerousGetSingleMemory().Pin();
this.kernels = new ResizeKernel[destinationLength];
this.tempValues = new double[this.MaxDiameter];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public ResizeWorker(
this.transposedFirstPassBuffer = configuration.MemoryAllocator.Allocate2D<Vector4>(
this.workerHeight,
targetWorkingRect.Width,
preferContiguosImageBuffers: true,
preferContiguousImageBuffers: true,
options: AllocationOptions.Clean);

this.tempRowBuffer = configuration.MemoryAllocator.Allocate<Vector4>(this.sourceRectangle.Width);
Expand Down
9 changes: 9 additions & 0 deletions tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -558,4 +558,13 @@ public void BmpDecoder_CanDecode_Os2BitmapArray<TPixel>(TestImageProvider<TPixel
// Compare to reference output instead.
image.CompareToReferenceOutput(provider, extension: "png");
}

[Theory]
[WithFile(Issue2696, PixelTypes.Rgba32)]
public void BmpDecoder_ThrowsException_Issue2696<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
=> Assert.Throws<InvalidImageContentException>(() =>
{
using Image<TPixel> image = provider.GetImage(BmpDecoder.Instance);
});
}
18 changes: 4 additions & 14 deletions tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -338,21 +338,11 @@ public void Issue2564_DecodeWorks<TPixel>(TestImageProvider<TPixel> provider)
}

[Theory]
[WithFile(TestImages.Jpeg.Issues.HangBadScan, PixelTypes.L8)]
public void DecodeHang<TPixel>(TestImageProvider<TPixel> provider)
[WithFile(TestImages.Jpeg.Issues.HangBadScan, PixelTypes.Rgb24)]
public void DecodeHang_ThrowsException<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
if (TestEnvironment.IsWindows &&
TestEnvironment.RunsOnCI)
{
// Windows CI runs consistently fail with OOM.
return;
}

using Image<TPixel> image = provider.GetImage(JpegDecoder.Instance);
Assert.Equal(65503, image.Width);
Assert.Equal(65503, image.Height);
}
=> Assert.Throws<InvalidImageContentException>(
() => { using Image<TPixel> image = provider.GetImage(JpegDecoder.Instance); });

// https://github.com/SixLabors/ImageSharp/issues/2517
[Theory]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@ public BufferTests()

protected SimpleGcMemoryAllocator MemoryAllocator { get; } = new SimpleGcMemoryAllocator();

[Theory]
[InlineData(-1)]
public void Allocate_IncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length)
public static TheoryData<int> InvalidLengths { get; set; } = new()
{
ArgumentOutOfRangeException ex = Assert.Throws<ArgumentOutOfRangeException>(() => this.MemoryAllocator.Allocate<BigStruct>(length));
Assert.Equal("length", ex.ParamName);
}
{ -1 },
{ (1 << 30) + 1 }
};

[Theory]
[MemberData(nameof(InvalidLengths))]
public void Allocate_IncorrectAmount_ThrowsCorrect_InvalidMemoryOperationException(int length)
=> Assert.Throws<InvalidMemoryOperationException>(
() => this.MemoryAllocator.Allocate<BigStruct>(length));

[Fact]
public unsafe void Allocate_MemoryIsPinnableMultipleTimes()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,20 @@ public void AllocateGroup_MultipleTimes_ExceedPoolLimit()
public void AllocateGroup_SizeInBytesOverLongMaxValue_ThrowsInvalidMemoryOperationException()
{
var allocator = new UniformUnmanagedMemoryPoolMemoryAllocator(null);
Assert.Throws<InvalidMemoryOperationException>(() => allocator.AllocateGroup<S4>(int.MaxValue * (long)int.MaxValue, int.MaxValue));
Assert.Throws<InvalidMemoryOperationException>(() => allocator.AllocateGroup<byte>(int.MaxValue * (long)int.MaxValue, int.MaxValue));
}

public static TheoryData<int> InvalidLengths { get; set; } = new()
{
{ -1 },
{ (1 << 30) + 1 }
};

[Theory]
[MemberData(nameof(InvalidLengths))]
public void Allocate_IncorrectAmount_ThrowsCorrect_InvalidMemoryOperationException(int length)
=> Assert.Throws<InvalidMemoryOperationException>(() => new UniformUnmanagedMemoryPoolMemoryAllocator(null).Allocate<S512>(length));

[Fact]
public unsafe void Allocate_MemoryIsPinnableMultipleTimes()
{
Expand Down
Loading

0 comments on commit 63d4b20

Please sign in to comment.