On Wed, 9 Apr 2025 16:58:28 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 one additional > commit since the last revision: > > 8351913: code cleanup > > This is in response to: > https://github.com/openjdk/jdk/pull/23920#discussion_r2029297962 test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 86: > 84: thumbnail = writeImage(byteOut); > 85: jpegData = byteOut.toByteArray(); > 86: } For `ByteArrayOutputStream`, it should be okay to convert it before the stream is closed, and the stream is closed by `writeImage` anyway. However, placing `jpegData = byteOut.toByteArray()` after the try-with-resources for `byteOut` won't raise any questions. Then, `jpegData` can be declared where it's assigned. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2035785315
