Skip to content

[IMAGING-385] Fix int overflow in ImageBuilder pixel array sizing - #559

Open
sharanggupta wants to merge 1 commit into
apache:masterfrom
sharanggupta:IMAGING-385-image-builder-size-overflow
Open

sharanggupta wants to merge 1 commit into
apache:masterfrom
sharanggupta:IMAGING-385-image-builder-size-overflow

Conversation

@sharanggupta

Copy link
Copy Markdown

Fixes IMAGING-385.

What

new ImageBuilder(width, height, ...) sized its pixel array with Allocator.intArray(width * height). The product is an int, so for large dimensions it wraps. The ticket's BMP is 2359300 x 65538: the product is 154,623,803,400, which wraps to 4,980,744. Allocator then accepts a 19 MB array, but the builder keeps the original width and height, and Imaging.getBufferedImage fails later in Raster.createPackedRaster with a JDK IllegalArgumentException ("Dimensions ... are too large"), which is not one of the documented ImagingException / IOException.

How

The same class of bug was fixed for PCX in #552 by computing the size as a long and handing it to the long overload of the Allocator. There is no long overload for int arrays yet, so this adds Allocator.intArray(long) next to the existing byteArray(long) (it uses check(long, int), so a request that does not fit an int or exceeds the limit throws AllocationRequestException) and ImageBuilder now calls it with (long) width * height. The only behaviour change is for sizes that used to wrap: they are now rejected up front with an AllocationRequestException, as in the PCX fix. All other callers of intArray(int) resolve to the existing overload.

I did not touch the other sizing overflows in the 38x tickets (PNG scanline size in IMAGING-388, TIFF strip length in IMAGING-387, and jpeg.decoder.Block has the same width * height pattern); they are separate code paths and I wanted to keep this one reviewable. Happy to look at them in follow-ups if you want.

Tests

  • ImageBuilderTest.testConstructorDimensionsOverflowingInt: the constructor rejects 2359300 x 65538.
  • BmpImageParserTest.testDimensionsOverflowingInt: the 90-byte file attached to the ticket (added as images/bmp/IMAGING-385/crash-e506...bmp, next to the existing IMAGING-325 one) now gives AllocationRequestException instead of IllegalArgumentException.
  • AllocatorTest: the long overload rejects an overflowing request and allocates a normal one.

Without the change the first two fail (nothing thrown / IllegalArgumentException); with it all tests pass. Verified locally with the default mvn goal on Java 17 (RAT, Checkstyle, SpotBugs, PMD, Javadoc).

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute? Claude (Anthropic) helped draft the fix and the tests; I reviewed all of it, ran the builds and checked that the new tests fail without the change.
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

ImageBuilder allocated its pixel array with Allocator.intArray(width * height).
The product is an int, so a 2359300 x 65538 image wrapped to 4,980,744 and
slipped past the allocation limit, and getBufferedImage() then failed in the
JDK with an IllegalArgumentException from Raster.createPackedRaster.

Add Allocator.intArray(long), next to the existing byteArray(long), and use it
with (long) width * height, like the PCX fix (apache#552). A pixel count that does not
fit an int or exceeds the limit is now rejected with an AllocationRequestException.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant