On Wed, 9 Apr 2025 23:31:02 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: > > Update test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java > > Co-authored-by: Alexey Ivanov <alexey.iva...@oracle.com> Marked as reviewed by aivanov (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/23920#pullrequestreview-2757023905
