-
-
Notifications
You must be signed in to change notification settings - Fork 886
Limit all memory allocations to configurable values. #2704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
0ed17bb
4e164a5
9cdcc4f
8806189
ebfe761
9c7d6dd
5fb0f99
1eecd40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ | |
| // Licensed under the Six Labors Split License. | ||
|
|
||
| using System.Buffers; | ||
| using System.Runtime.CompilerServices; | ||
| using SixLabors.ImageSharp.PixelFormats; | ||
|
|
||
| namespace SixLabors.ImageSharp.Memory; | ||
|
|
||
|
|
@@ -10,6 +12,16 @@ namespace SixLabors.ImageSharp.Memory; | |
| /// </summary> | ||
| public abstract class MemoryAllocator | ||
| { | ||
| /// <summary> | ||
| /// Gets the default max allocatable size of a 1D buffer in bytes. | ||
| /// </summary> | ||
| public static readonly int DefaultMaxAllocatableSize1DInBytes = GetDefaultMaxAllocatableSize1DInBytes(); | ||
|
|
||
| /// <summary> | ||
| /// Gets the default max allocatable size of a 2D buffer in bytes. | ||
| /// </summary> | ||
| public static readonly Size DefaultMaxAllocatableSize2DInBytes = GetDefaultMaxAllocatableSize2DInBytes(); | ||
|
|
||
| /// <summary> | ||
| /// Gets the default platform-specific global <see cref="MemoryAllocator"/> instance that | ||
| /// serves as the default value for <see cref="Configuration.MemoryAllocator"/>. | ||
|
|
@@ -20,6 +32,18 @@ public abstract class MemoryAllocator | |
| /// </summary> | ||
| public static MemoryAllocator Default { get; } = Create(); | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the maximum allowable allocatable size of a 2 dimensional buffer. | ||
| /// Defaults to <value><see cref="DefaultMaxAllocatableSize2DInBytes"/>.</value> | ||
| /// </summary> | ||
| public Size MaxAllocatableSize2DInBytes { get; set; } = DefaultMaxAllocatableSize2DInBytes; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the maximum allowable allocatable size of a 1 dimensional buffer. | ||
| /// </summary> | ||
| /// Defaults to <value><see cref="GetDefaultMaxAllocatableSize1DInBytes"/>.</value> | ||
| public int MaxAllocatableSize1DInBytes { get; set; } = DefaultMaxAllocatableSize1DInBytes; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is in fact a limit for a single contigous buffer.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, yes, but you have a different internal value for that which is used for contiguous chunks within a 2D buffer. I had to create something new. |
||
|
|
||
| /// <summary> | ||
| /// Gets the length of the largest contiguous buffer that can be handled by this allocator instance in bytes. | ||
| /// </summary> | ||
|
|
@@ -42,7 +66,7 @@ public static MemoryAllocator Create(MemoryAllocatorOptions options) => | |
| new UniformUnmanagedMemoryPoolMemoryAllocator(options.MaximumPoolSizeMegabytes); | ||
|
|
||
| /// <summary> | ||
| /// Allocates an <see cref="IMemoryOwner{T}" />, holding a <see cref="Memory{T}"/> of length <paramref name="length"/>. | ||
| /// Allocates an <see cref="IMemoryOwner{T}"/>, holding a <see cref="Memory{T}"/> of length <paramref name="length"/>. | ||
| /// </summary> | ||
| /// <typeparam name="T">Type of the data stored in the buffer.</typeparam> | ||
| /// <param name="length">Size of the buffer to allocate.</param> | ||
|
|
@@ -64,6 +88,7 @@ public virtual void ReleaseRetainedResources() | |
| /// <summary> | ||
| /// Allocates a <see cref="MemoryGroup{T}"/>. | ||
| /// </summary> | ||
| /// <typeparam name="T">Type of the data stored in the buffer.</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> | ||
|
|
@@ -75,4 +100,43 @@ internal virtual MemoryGroup<T> AllocateGroup<T>( | |
| AllocationOptions options = AllocationOptions.None) | ||
| where T : struct | ||
| => MemoryGroup<T>.Allocate(this, totalLength, bufferAlignment, options); | ||
|
|
||
| internal static void MemoryGuardMustBeBetweenOrEqualTo<T>(int value, int min, int max, string paramName) | ||
|
||
| where T : struct | ||
| { | ||
| int typeSizeInBytes = Unsafe.SizeOf<T>(); | ||
| long valueInBytes = value * typeSizeInBytes; | ||
|
|
||
| // If a sufficiently large value is passed in, the multiplication will overflow. | ||
| // We can detect this by checking if the result is less than the original value. | ||
| if (valueInBytes < value && value > 0) | ||
| { | ||
| valueInBytes = long.MaxValue; | ||
| } | ||
|
|
||
| if (valueInBytes >= min && valueInBytes <= max) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| throw new InvalidMemoryOperationException($"Parameter \"{paramName}\" must be between or equal to {min} and {max}, was {valueInBytes}"); | ||
|
||
| } | ||
|
|
||
| private static Size GetDefaultMaxAllocatableSize2DInBytes() | ||
| { | ||
| // Limit dimensions to 65535x65535 and 32767x32767 @ 4 bytes per pixel for 64 and 32 bit processes respectively. | ||
| int maxLength = Environment.Is64BitProcess ? ushort.MaxValue : short.MaxValue; | ||
| int maxLengthInRgba32Bytes = maxLength * Unsafe.SizeOf<Rgba32>(); | ||
| return new(maxLengthInRgba32Bytes, maxLengthInRgba32Bytes); | ||
|
||
| } | ||
|
|
||
| private static int GetDefaultMaxAllocatableSize1DInBytes() | ||
| { | ||
| // It's possible to require buffers that are not related to image dimensions. | ||
| // For example, when we need to allocate buffers for IDAT chunks in PNG files or when allocating | ||
| // cache buffers for image quantization. | ||
| // Limit the maximum buffer size to 64MB for 64bit processes and 32MB for 32 bit processes. | ||
| int limitInMB = Environment.Is64BitProcess ? 64 : 32; | ||
| return limitInMB * 1024 * 1024; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of defining 2D dimension limits, we should define a memory group limit in bytes. Also see my comment on
GetDefaultMaxAllocatableSize2DInBytes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it to somehow reflect the actual dimensions of a buffer (to the user an image). A memory group is an implementation detail but if I can say don't allow a total allocation of more than X, Y then the groups are covered.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to see it the other way around. If you define a limit for a discontigous allocation it will cover the image limit.
Not entirely, we have documented that our buffers are discontiguous and
IMemoryGroup<T>is public. Regardless, we don't need to involve those concepts to when defining the limits. We can just document that maximum in-memory image size isX (mega/giga/tera)bytes. It is much better to reason about from memory management perspective.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we believe we need dimension limits (eg. bc various formats have them anyways), we can apply them separately either in
Image<T>or inBuffer2D<T>code, without involvingsizeof(TPixel). But for me that concern is orthogonal to the issue this PR is aimed to fix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue directly stems from attempt to allocate an image that is too large. Fixing any additional allocation limits was a something we needed to do in addition to that public facing allocation.
By placing a limit at the point of allocation call, before we start even looking at memory groups, I avoid the possibility of over allocation in all cases. The limits are not dimensional limits, they reflect dimensional limits. We can now say, we've defaulted to an image of X*Y at this pixel format bit depth. It's a starting point that users can understand and update when required.
Whether our buffers are discontiguous doesn't really change things. We've already established that attempting to allocate an image of overly large dimensions will cause issues.