RFR: 8351110: ImageIO.write for JPEG can write corrupt JPEG for certain thumbnail dimensions [v4]

[Alexey Ivanov]

On Wed, 12 Mar 2025 18:23:12 GMT, Jeremy Wood <duke at openjdk.org> wrote:

>> JPEG segments can only be 65535-bytes long. (The marker length is expressed as 2 bytes.) The problem in this ticket is that we were writing more than 65535 bytes to a segment, which later caused parsing errors when we tried to read the file back.
>> 
>> This includes 2 changes:
>> 
>> 1. We now clip the thumbnail width/height to fit within the available marker. We were already constraining the width & height to a max of 255. (Because the width & height are expressed as 1 byte.) So this new clipping is similar to something we were already doing, and we issue the same WARNING_THUMB_CLIPPED warning to the writer. (Does this require a CSR?)
>> 
>> 2. This adds a bounds check to `write2bytes`. IMO the bigger problem in this ticket was the `ImageIO.write(..)` caller thought they ended up with a valid JPEG. (We were violating the "do no harm / lose no data" doctrine.) Now if something like this ever comes up again: writing will fail with an IIOException. Technically you can argue this change is unnecessary because the new clipping avoids this error condition, but IMO including this safety net is an overall good thing to keep. If anyone disagrees I'm OK with removing it.
>> 
>> Separately:
>> I included (and reversed) a commit in this branch that provides an alternate solution. That commit anticipates the overflow problem and switches to a JPEG-encoded thumbnail. From an end user perspective this "just works": they get a (slightly lossly) thumbnail of the original dimensions. But I assume it would be more controversial compared to the clipping approach, since the clipping approach has a strong precedent in this codebase.
>
> Jeremy Wood has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - 8351110: adding a few clarifying comments
>  - 8351110: changing `catch` statement to support assertEquals's Error

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/com/sun/imageio/plugins/jpeg/JFIFMarkerSegment.java line 383:

> 381:             if (thumbWidth * thumbHeight > maxArea) {
> 382:                 writer.warningOccurred(JPEGImageWriter.WARNING_THUMB_CLIPPED);
> 383:                 double scale = Math.sqrt( ((double)maxArea) / (double)(thumbWidth * thumbHeight));

Does it make sense to add a space before the closing parenthesis? For consistency with the opening one; or remove these spaces just inside the method call parentheses.
Suggestion:

                double scale = Math.sqrt( ((double)maxArea) / (double)(thumbWidth * thumbHeight) );

test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 66:

> 64:         boolean b2 = new WriteJPEGThumbnailTest(100, 219).run();
> 65:         if (!(b1 && b2))
> 66:             System.err.println("Test failed");

Suggestion:

        if (!(b1 && b2)) {
            System.err.println("Test failed");
            throw new Error("Test failed");
        }

Always use braces, even for one-line statement.

You must throw an exception, otherwise the test will never fail from jtreg point of view.


jdk> ./build/windows-x86_64-server-release/jdk/bin/java -jar ../jtreg/lib/jtreg.jar \
    -w:./build/jtwork -nr -v \
    test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
runner starting test: javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
runner finished test: javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
Passed. Execution successful
Test results: passed: 1

And I don't have your fix.

test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 86:

> 84:                 } finally {
> 85:                     jpegData = byteOut.toByteArray();
> 86:                 }

Does it have to be in the `finally` block?

You can assign `jpegData` outside of the `try` block. If an exception is thrown in `try`, the method stops executing and assigning to `jpegData` makes no sense.

test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 104:

> 102:             }
> 103:             System.out.println("\tTest passed");
> 104:         } catch(Throwable e) {

Suggestion:

        } catch (Throwable e) {

test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 142:

> 140:         g.fillRect(0,900,100,100);
> 141:         g.setColor(Color.magenta);
> 142:         g.fillRect(900,900,100,100);

Spaces after commas, upper-case color constants:
Suggestion:

        g.setColor(Color.RED);
        g.fillRect(0, 0, 100, 100);
        g.setColor(Color.GREEN);
        g.fillRect(900, 0, 900, 100);
        g.setColor(Color.ORANGE);
        g.fillRect(0, 900, 100, 100);
        g.setColor(Color.MAGENTA);
        g.fillRect(900, 900, 100, 100);

test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 152:

> 150:         while(readers.hasNext()) {
> 151:             reader = readers.next();
> 152:             if(reader.canReadRaster()) {

Suggestion:

        while (readers.hasNext()) {
            reader = readers.next();
            if (reader.canReadRaster()) {

-------------

PR Review: https://git.openjdk.org/jdk/pull/23920#pullrequestreview-2743856146
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029289420
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029310112
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029297962
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029303202
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029301961
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029302608