Skip to content

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.

Sign up for GitHub

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

Closed
JimBobSquarePants wants to merge 8 commits into from
Closed

Limit all memory allocations to configurable values. #2704

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ed17bb
Limit bmp sizes to match browser max.
JimBobSquarePants Mar 20, 2024
4e164a5
Use MemoryAllocator directly to limit all allocations.
JimBobSquarePants Mar 21, 2024
9cdcc4f
Bump 1D limit
JimBobSquarePants Mar 21, 2024
8806189
Fix all tests
JimBobSquarePants Mar 21, 2024
ebfe761
Use smarter per-environment limits
JimBobSquarePants Mar 23, 2024
9c7d6dd
OCD
JimBobSquarePants Mar 23, 2024
5fb0f99
Fix 2D limit, improve message and simplify
JimBobSquarePants Mar 23, 2024
1eecd40
Fix assignment limits
JimBobSquarePants Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1422,6 +1422,16 @@ private int ReadImageHeaders(BufferedReadStream stream, out bool inverted, out b
this.ReadFileHeader(stream);
this.ReadInfoHeader(stream);

// BMPs with negative-or-zero width are invalid. Also, reject extremely wide images
// to keep the math sane. And reject int.MinValue as a height because you can't
// get its absolute value (because -int.MinValue is one more than int.MaxValue).
const int k64KWidth = 65535;
Copy link
Member

@tocsoft tocsoft Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this not be a configurable value? I can see use cases where someone might want to load very large images... they would be responsible to ensure they have enough memory. This is a sensible default looks fine but allowing a user to bypass where they understand the danger etc.... I can also see use cases in the imagesharp.web world where we might even want even lower limits to prevent bad actors blowing out memory etc.

We might even want to consider a more generic max width/max height set of rules that can be applied to all image formats uniformly.

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, updated.

bool sizeOk = this.infoHeader.Width > 0 && this.infoHeader.Width <= k64KWidth && this.infoHeader.Height != int.MinValue;
if (!sizeOk)
{
BmpThrowHelper.ThrowInvalidImageContentException($"Invalid file header dimensions found. {this.infoHeader.Width}x{this.infoHeader.Height}px.");
}

// see http://www.drdobbs.com/architecture-and-design/the-bmp-file-format-part-1/184409517
// If the height is negative, then this is a Windows bitmap whose origin
// is the upper-left corner and not the lower-left. The inverted flag
Expand Down
9 changes: 9 additions & 0 deletions tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs
View file
Open in desktop
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);
});
}
2 changes: 2 additions & 0 deletions tests/ImageSharp.Tests/TestImages.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,8 @@ public static class Bmp

public const string BlackWhitePalletDataMatrix = "Bmp/bit1datamatrix.bmp";

public const string Issue2696 = "Bmp/issue-2696.bmp";

public static readonly string[] BitFields =
{
Rgb32bfdef,
Expand Down
3 changes: 3 additions & 0 deletions tests/Images/Input/Bmp/issue-2696.bmp
View file
Open in desktop
Git LFS file not shown