On Fri, 4 Apr 2025 22:57:34 GMT, Jeremy Wood <d...@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 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.iva...@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.iva...@oracle.com> > - Update test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java > > Co-authored-by: Alexey Ivanov <alexey.iva...@oracle.com> > - Update test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java > > Co-authored-by: Alexey Ivanov <alexey.iva...@oracle.com> > - Update > src/java.desktop/share/classes/com/sun/imageio/plugins/jpeg/JFIFMarkerSegment.java > > Co-authored-by: Alexey Ivanov <alexey.iva...@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/8ef20634...ecd7883c Changes requested by aivanov (Reviewer). test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 65: > 63: // we wrote a corrupt JPEG file.) > 64: boolean b2 = new WriteJPEGThumbnailTest(100, 219).run(); > 65: if (!(b1 && b2)) { Suggestion: boolean b2 = new WriteJPEGThumbnailTest(100, 219).run(); if (!(b1 && b2)) { I'd add a blank line here to visually separate the test from the condition. test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 71: > 69: } > 70: > 71: final int thumbWidth, thumbHeight; Suggestion: final int thumbWidth; final int thumbHeight; I prefer them on separate lines, although it's not that important for a test. test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 123: > 121: > 122: // Write the main image > 123: IIOImage img = new javax.imageio.IIOImage(bi, > List.of(thumbnail), null); Suggestion: IIOImage img = new IIOImage(bi, List.of(thumbnail), null); The package name on right side of the assignment is redundant. ------------- PR Review: https://git.openjdk.org/jdk/pull/23920#pullrequestreview-2753786051 PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2035553786 PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2035563230 PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2035559293
