Limit all memory allocations in the MemoryAllocator layer #2706

antonfirsov · 2024-03-28T02:17:22Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

Fixes #2696.

This is an alternative take for #2704. Differences:

The discontiguous limit is configurable in Megabytes on MemoryAllocatorOptions (together with the pool size).
- On 64bit it defaults to 4GB, equivalent of 32k x 32k of Rgba32 which is more conservative than the limit in Limit all memory allocations to configurable values. #2704. For me this feels somewhat safer while I don't see many downsides. Note that this is the absolute hardcoded limit in WIC, while newer skia builds have a hardcoded 2GB limit (this hasn't been merged to in SkiaSharp 2.* yet)
- On 32bit it defaults to 1GB which is a significant portion of the overall virtual memory limit of 4GB. Maybe we can make it even lower.
Single contiguous buffers have a limit of 1GB (or the discontiguous limit, if it's lower). This limit is not configurable separately in order to make things simpler for users. I don't see any risks from that nor the need to make it as low as in Limit all memory allocations to configurable values. #2704. The RLE path in BMP and TIFF decoders actually allocates temporary contiguous buffers of width * height sizes so a 64MB limit would be easy to exceed with sane images.
It also significantly reduces MaxAllocationAttempts so we don't block too long if an OOM is still being hit.

The PR is missing tests and PNG tweak from #2704. Will finalize it after agreeing on the direction.

antonfirsov · 2024-03-28T03:05:29Z

src/ImageSharp/Memory/Allocators/MemoryAllocator.cs

+        4L * OneGigabyte :
+        OneGigabyte;
+
+    internal int SingleBufferAllocationLimitBytes { get; private set; } = OneGigabyte;


Actually, there is no good reason to set this to 1GB. Contiguous buffers have a natural limit of int.MaxValue ~ 2GB so I think we should set the contiguous limit to min(int.MaxValue, MemoryAllocatorOptions.AllocationLimitMegabytes * 1MB). That would get rid of all obscure logic and make the semantics of MemoryAllocatorOptions.AllocationLimitMegabytes very obvious.

Yeah. I like that. The only risk we have with this stuff is in our decoders which can be easily mitigated with some care (PNG tweaks)

JimBobSquarePants · 2024-03-28T07:11:19Z

src/ImageSharp/Memory/Allocators/MemoryAllocatorOptions.cs

+    /// Gets or sets a value defining the maximum (discontiguous) buffer size that can be allocated by the allocator in Megabytes.
+    /// <see langword="null"/> means platform default: 1GB on 32-bit processes, 4GB on 64-bit processes.
+    /// </summary>
+    public int? AllocationLimitMegabytes


You got it in the options! Nice!

JimBobSquarePants · 2024-03-28T07:14:08Z

src/ImageSharp/Memory/Allocators/MemoryAllocator.cs

+    public static MemoryAllocator Create(MemoryAllocatorOptions options)
+    {
+        UniformUnmanagedMemoryPoolMemoryAllocator allocator = new(options.MaximumPoolSizeMegabytes);
+        if (options.AllocationLimitMegabytes.HasValue)


For V4 I think we should introduce UniformUnmanagedMemoryPoolOptions which has the standard options as a property.

Actually, AllocatorOptions is specific to UniformUnmanagedMemoryAllocator (which is intantiated with MemoryAllocator.Create(), the type itself is not public). I don't think anyone ever would need another allocator IRL. The design keeps allocators pluggable just in case + SimpleGcMemoryAllocator can be useful for diagnostic purposes since managed allocations are easy to track with standard .NET tools. But SimpleGcMemoryAllocator should not be used in production.

To secure users who still use it (mostly legacy code designed to workaround 1.* issues), I applied the default limits, but you cannot configure SimpleGcMemoryAllocator with MemoryAllocatorOptions.

JimBobSquarePants · 2024-03-28T07:14:45Z

src/ImageSharp/Memory/InvalidMemoryOperationException.cs

@@ -24,4 +26,8 @@ public InvalidMemoryOperationException(string message)
    public InvalidMemoryOperationException()
    {
    }
+
+    [DoesNotReturn]
+    internal static void ThrowAllocationOverLimitException(long length, long limit) =>


I like this sitting here.

JimBobSquarePants · 2024-03-28T07:16:45Z

src/ImageSharp/Memory/Allocators/SimpleGcMemoryAllocator.cs

@@ -19,6 +20,13 @@ public override IMemoryOwner<T> Allocate<T>(int length, AllocationOptions option
    {
        Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length));

+        int lengthInBytes = length * Unsafe.SizeOf<T>();


For V4 I'd like to have this in MemoryAllocator so we don't have to repeat the check.

JimBobSquarePants · 2024-03-28T07:24:17Z

src/ImageSharp/Memory/Allocators/MemoryAllocator.cs

        long totalLength,
        int bufferAlignment,
        AllocationOptions options = AllocationOptions.None)
        where T : struct
-        => MemoryGroup<T>.Allocate(this, totalLength, bufferAlignment, options);
+    {
+        long totalLengthInBytes = totalLength * Unsafe.SizeOf<T>();


I think it might be possible to overflow this. I think one of our tests that allocates a 256 byte struct does so.

JimBobSquarePants

That's much neater than my version. I say go with this. 👍

tocsoft · 2024-03-28T10:22:01Z

src/ImageSharp/Memory/Allocators/MemoryAllocatorOptions.cs

+    /// Gets or sets a value defining the maximum (discontiguous) buffer size that can be allocated by the allocator in Megabytes.
+    /// <see langword="null"/> means platform default: 1GB on 32-bit processes, 4GB on 64-bit processes.
+    /// </summary>
+    public int? AllocationLimitMegabytes


Is this consistent with how we talk about memory across the public api? do we talk about megabytes anywhere else in the public api or is it all bytes?

we want to make sure we are being consistent on our units of measure in the public api surface.

The other property on MemoryAllocatiorOptions is MaximumPoolSizeMegabytes. When I designed the MemoryAllocator configuration API it seemed like Megabytes are easier to reason about when talking about limits.

JimBobSquarePants

I'm happy with this @antonfirsov and have created a backport PR for V2.

I'll wait for your final read-through to merge though.

antonfirsov · 2024-04-10T22:04:12Z

tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs

+            {
+                using Image<TPixel> image = provider.GetImage(BmpDecoder.Instance);
+            });
+        Assert.IsType<InvalidMemoryOperationException>(ex.InnerException);


This made me find that the file wasn't the actual file from #2696 here, leading to an exception in header parsing instead of an allocation failure. Fixed in d1cc651.

Fix an overlook from #2706. See 92b8277#r141770676.

Fix an overlook from #2706. See 92b8277#r141770676. # Conflicts: # src/ImageSharp/Memory/Allocators/MemoryAllocator.cs

antonfirsov added 2 commits March 28, 2024 02:36

Limit all allocations

92b8277

reduce UnmanagedMemoryHandle.MaxAllocationAttempts

7dd3c43

antonfirsov marked this pull request as draft March 28, 2024 02:17

antonfirsov mentioned this pull request Mar 28, 2024

Limit all memory allocations to configurable values. #2704

Closed

4 tasks

antonfirsov commented Mar 28, 2024

View reviewed changes

JimBobSquarePants reviewed Mar 28, 2024

View reviewed changes

tocsoft reviewed Mar 28, 2024

View reviewed changes

Fix args and add tests

63d4b20

JimBobSquarePants mentioned this pull request Apr 3, 2024

V4 - Limit all memory allocations to configurable values. #2705

Closed

4 tasks

JimBobSquarePants added 2 commits April 4, 2024 14:23

Merge branch 'release/3.1.x' into af/memlimit-01

eec9718

Revert additional changes

da49788

JimBobSquarePants changed the title ~~[WIP] Limit all memory allocations in the MemoryAllocator layer~~ Limit all memory allocations in the MemoryAllocator layer Apr 6, 2024

JimBobSquarePants marked this pull request as ready for review April 6, 2024 00:53

JimBobSquarePants approved these changes Apr 6, 2024

View reviewed changes

JimBobSquarePants mentioned this pull request Apr 6, 2024

V2 - Limit all memory allocations in the MemoryAllocator layer #2715

Merged

4 tasks

JimBobSquarePants added the security label Apr 6, 2024

antonfirsov added 2 commits April 10, 2024 23:18

fix BmpDecoder_ThrowsException_Issue2696

d1cc651

test allocation limits

572366e

antonfirsov commented Apr 10, 2024

View reviewed changes

antonfirsov merged commit b6b08ac into release/3.1.x Apr 10, 2024
8 checks passed

antonfirsov deleted the af/memlimit-01 branch April 10, 2024 22:16

JimBobSquarePants mentioned this pull request Apr 15, 2024

Merge latest release from v3 #2720

Merged

4 tasks

antonfirsov mentioned this pull request May 7, 2024

Fix overflow in MemoryAllocator.Create(options) #2730

Merged

4 tasks

antonfirsov added a commit that referenced this pull request May 8, 2024

Fix overflow in MemoryAllocator.Create(options) (#2730)

467850f

Fix an overlook from #2706. See 92b8277#r141770676.

antonfirsov added a commit that referenced this pull request May 8, 2024

Fix overflow in MemoryAllocator.Create(options) (#2730)

7d3f852

Fix an overlook from #2706. See 92b8277#r141770676.

antonfirsov added a commit that referenced this pull request May 8, 2024

Fix overflow in MemoryAllocator.Create(options) (#2730)

7386702

Fix an overlook from #2706. See 92b8277#r141770676. # Conflicts: # src/ImageSharp/Memory/Allocators/MemoryAllocator.cs

antonfirsov mentioned this pull request May 8, 2024

[3.1] Fix overflow in MemoryAllocator.Create(options) #2731

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit all memory allocations in the MemoryAllocator layer #2706

Limit all memory allocations in the MemoryAllocator layer #2706

antonfirsov commented Mar 28, 2024 •

edited

antonfirsov Mar 28, 2024

JimBobSquarePants Mar 28, 2024

JimBobSquarePants Mar 28, 2024

JimBobSquarePants Mar 28, 2024

antonfirsov Mar 28, 2024 •

edited

JimBobSquarePants Mar 28, 2024

JimBobSquarePants Mar 28, 2024

JimBobSquarePants Mar 28, 2024

JimBobSquarePants left a comment

tocsoft Mar 28, 2024

antonfirsov Mar 28, 2024

JimBobSquarePants left a comment

antonfirsov Apr 10, 2024

Limit all memory allocations in the MemoryAllocator layer #2706

Limit all memory allocations in the MemoryAllocator layer #2706

Conversation

antonfirsov commented Mar 28, 2024 • edited

Prerequisites

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Mar 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov commented Mar 28, 2024 •

edited

antonfirsov Mar 28, 2024 •

edited