RFR: 8351110: ImageIO.write for JPEG can write corrupt JPEG for certain thumbnail dimensions [v6]
Jeremy Wood
duke at openjdk.org
Fri Apr 4 22:57:34 UTC 2025
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 24 additional commits since the last revision:
- Update test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
Co-authored-by: Alexey Ivanov <alexey.ivanov at oracle.com>
- Merge remote-tracking branch 'origin/JDK-8351110' into JDK-8351110
- Update test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
Co-authored-by: Alexey Ivanov <alexey.ivanov at oracle.com>
- Update test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
Co-authored-by: Alexey Ivanov <alexey.ivanov at oracle.com>
- Update test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
Co-authored-by: Alexey Ivanov <alexey.ivanov at oracle.com>
- Update src/java.desktop/share/classes/com/sun/imageio/plugins/jpeg/JFIFMarkerSegment.java
Co-authored-by: Alexey Ivanov <alexey.ivanov at oracle.com>
- Merge branch 'master' into JDK-8351110
- Merge branch 'master' of https://github.com/mickleness/jdk
- Merge branch 'openjdk:master' into master
- 8351110: adding a few clarifying comments
- ... and 14 more: https://git.openjdk.org/jdk/compare/ce0122f9...ecd7883c
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/23920/files
- new: https://git.openjdk.org/jdk/pull/23920/files/21dce8e5..ecd7883c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=23920&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=23920&range=04-05
Stats: 149020 lines in 3405 files changed: 66274 ins; 63624 del; 19122 mod
Patch: https://git.openjdk.org/jdk/pull/23920.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/23920/head:pull/23920
PR: https://git.openjdk.org/jdk/pull/23920
More information about the client-libs-dev
mailing list