On Wed, 9 Apr 2025 18:09:31 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 incrementally with six additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/JDK-8351110' into JDK-8351110 > > # Conflicts: > # test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java > - 8351110: code cleanup > > This is in response to: > https://github.com/openjdk/jdk/pull/23920#discussion_r2035785315 > - 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> > - 8351110: code cleanup > > This is in response to: > https://github.com/openjdk/jdk/pull/23920#discussion_r2029297962 Marked as reviewed by aivanov (Reviewer). test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 85: > 83: ByteArrayOutputStream byteOut = new ByteArrayOutputStream(); > 84: BufferedImage thumbnail = writeImage(byteOut); > 85: byte[] jpegData = byteOut.toByteArray(); Now I see the complication. When you use try-with-resources, the stream is declared within the scope of the try-block. I didn't notice it before. I may suggest something like this to overcome the complication: BufferedImage thumbnail; ByteArrayOutputStream byteOut = new ByteArrayOutputStream(); try (byteOut) { thumbnail = writeImage(byteOut); } ImageReader reader = getJPEGImageReader(); ImageInputStream stream = ImageIO.createImageInputStream( new ByteArrayInputStream(byteOut.toByteArray())); reader.setInput(stream); Such syntax for try-with-resources is acceptable since Java 9, if my memory serves me right; yet Java 8 requires you to declare and initialise the auto-closable stream inside the `try`-parentheses. Anyway the current version looks good to me too, and it's shorter. ------------- PR Review: https://git.openjdk.org/jdk/pull/23920#pullrequestreview-2754795132 PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2036131572
