On Wed, 5 Mar 2025 18:29:49 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. test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest line 2: > 1: /* > 2: * Copyright (c) 20025, Oracle and/or its affiliates. All rights reserved. Suggestion: * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest line 38: > 36: import javax.imageio.stream.ImageInputStream; > 37: import javax.imageio.stream.ImageOutputStream; > 38: import java.awt.*; Please expand the wildcard imports. test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest line 41: > 39: import java.awt.geom.AffineTransform; > 40: import java.awt.image.BufferedImage; > 41: import java.io.*; Please expand the wildcard imports. test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest line 155: > 153: return null; > 154: } > 155: } Empty line at last is missing. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r1984465597 PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r1984466151 PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r1984466228 PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r1984466870
