8351110: ImageIO.write for JPEG can write corrupt JPEG for certain thumbnail dimensions #23920
+169
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge openjdk/jdk into mickleness/jdk
Merge openjdk/jdk
Merge from openjdk/jdk
Updating mickleness/jdk from openjdk/jdk
updating to openjdk/jdk
IMO the most serious part of this ticket is that the caller receives no indication that the file that was written is corrupt. With this change the caller will at least see an IIOException. This does not satisfactorily resolve the ticket (we want some sort of non-corrupt JPEG), but it's a start. This placement may be invasive, but I think it's appropriate. I'm having a hard time imagining a scenario where it's safe/ok to call `write2bytes` for -1 or 70,000.
This is my preferred resolution to this ticket, however I'm worried it's too unorthodox so I'll revert it shortly. Now if the thumbnail is going to be too large: we switch from writing an RGB-style thumbnail to a JPEG-compressed thumbnail. In this ticket's case: now the call to `ImageIO.write(..)` "just works": the caller gets a valid JPEG file with the entire thumbnail.
This reverts commit fd0ec98.
We were already clipping the thumbnail width/height to 255px. Now we're also clipping the thumbnail width/height if the total number of bytes we write will exceed the segment capacity. We may (?) want to introduce a separate warning constant (other than WARNING_THUMB_CLIPPED?), which may involve a CSR.
|
👋 Welcome back mickleness! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@mickleness The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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?)
This adds a bounds check to
write2bytes. IMO the bigger problem in this ticket was theImageIO.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.
Progress
Error
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23920/head:pull/23920$ git checkout pull/23920Update a local copy of the PR:
$ git checkout pull/23920$ git pull https://git.openjdk.org/jdk.git pull/23920/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23920View PR using the GUI difftool:
$ git pr show -t 23920Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23920.diff
Using Webrev
Link to Webrev Comment