Limit all memory allocations to configurable values.

SixLabors / ImageSharp

:camera: A modern, cross-platform, 2D Graphics library for .NET

https://sixlabors.com/products/imagesharp/

Other

7.39k stars 851 forks source link

Limit all memory allocations to configurable values. #2704

Closed JimBobSquarePants closed 6 months ago

JimBobSquarePants commented 6 months ago

Prerequisites

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

Description

While the specification does not apply a limitation on BMP dimensions, on some machines attempting to decode a malformed or extremely large BMP can lead to OOM exceptions.

This change limits the dimensions to match the default set by browsers. For example Firefox

Fixes #2696

JimBobSquarePants commented 6 months ago

I've now added configuration to MemoryAllocator directly. I couldn't touch MemoryAllocatorOptions since that would mean breaking several constructors. This limits all allocations for all operations.

JimBobSquarePants commented 6 months ago

@antonfirsov I really don't understand what you mean by this.

In practice this means that the 2D limit logic has to be pushed down to the memory group allocation code. I think the resulting code will be cleaner.

It is at the very root of the memory allocation code. We limit any and all allocations from within MemoryAllocator itself.

Most importantly, we need good evidence for the default buffer limits we define.

65535*65535 is the maximum valid Jpeg dimension. 32767x32767 is half of that and what System.Drawing limits you to on all platforms.

The 1D buffer allocations could possibly be lower than 64/32MB but I chose those values because we already attempt to allocate 64MBin our memory tests.

JimBobSquarePants commented 6 months ago

@antonfirsov @tocsoft Can you please review at your earliest opportunity. I have something in place now that will enforce a limit while maintaining a sane configurable API.

antonfirsov commented 6 months ago

@JimBobSquarePants I decided to communicate my ideas in an alternative PR #2706, let me know what you think.

JimBobSquarePants commented 6 months ago

@JimBobSquarePants I decided to communicate my ideas in an alternative PR #2706, let me know what you think.

Your PR Rulez, mine droolz.

JimBobSquarePants commented 6 months ago

Closed in favor of #2706