Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8270893: IndexOutOfBoundsException while reading large TIFF file #4836

Closed
wants to merge 2 commits into from

Conversation

jayathirthrao
Copy link
Member

@jayathirthrao jayathirthrao commented Jul 20, 2021

We are incorrectly passing source offset to ImageInputStream.readFully() which is getting used on destination buffer. streamPos maintained in each implementation of stream maintain's appropriate source offset while reading the data. Since we are completely utilizing destination buffer any offset greater than 0 would cause IOOBE. In our case we should use 0 as offset value.

Also to hit this code we need stream/file with at-least 1MB of IFD data, that's why there is no regression test. This change can be verified using image attached in JBS. All test run is green.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8270893: IndexOutOfBoundsException while reading large TIFF file

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4836/head:pull/4836
$ git checkout pull/4836

Update a local copy of the PR:
$ git checkout pull/4836
$ git pull https://git.openjdk.java.net/jdk pull/4836/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4836

View PR using the GUI difftool:
$ git pr show -t 4836

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4836.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 20, 2021

👋 Welcome back jdv! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jul 20, 2021
@openjdk
Copy link

openjdk bot commented Jul 20, 2021

@jayathirthrao The following label will be automatically applied to this pull request:

  • 2d

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.

@openjdk openjdk bot added the 2d label Jul 20, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 20, 2021

Webrevs

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Jul 20, 2021

RFR(Adding comment to check whether git sends a mail)

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Jul 21, 2021

Ping

@mrserb
Copy link
Member

mrserb commented Jul 21, 2021

Is it possible to create a testcase for this fix? Probably this regression should be fixed in jdk17?

@prrace
Copy link
Contributor

prrace commented Jul 21, 2021

Is it possible to create a testcase for this fix? Probably this regression should be fixed in jdk17?

FWIW the fixer had already written :
"Also to hit this code we need stream/file with at-least 1MB of IFD data, that's why there is no regression test"

Maybe 1Mb isn't actually a blocker, and it would be good to learn if we can create a TIFF image of that size and have github accept it.

But we definitely should run all the relevant existing regression tests.

As for JDk 17, we are in RDP 2 and this has been out there for exactly (?) 12 months until the first report and will need backporting anyway to other release trains.
So it is safer to put in 18 and backport later to a 17 update

prrace
prrace approved these changes Jul 21, 2021
Copy link
Contributor

@prrace prrace left a comment

Approving but I would like some more evidence that a test is not practical.

@openjdk
Copy link

openjdk bot commented Jul 21, 2021

@jayathirthrao This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8270893: IndexOutOfBoundsException while reading large TIFF file

Reviewed-by: prr, serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 189 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jul 21, 2021
@jayathirthrao
Copy link
Member Author

jayathirthrao commented Jul 22, 2021

Is it possible to create a testcase for this fix? Probably this regression should be fixed in jdk17?

FWIW the fixer had already written :
"Also to hit this code we need stream/file with at-least 1MB of IFD data, that's why there is no regression test"

Maybe 1Mb isn't actually a blocker, and it would be good to learn if we can create a TIFF image of that size and have github accept it.

But we definitely should run all the relevant existing regression tests.

As for JDk 17, we are in RDP 2 and this has been out there for exactly (?) 12 months until the first report and will need backporting anyway to other release trains.
So it is safer to put in 18 and backport later to a 17 update

Sure i will take a look at creating regression test case for this issue and see whether we can hit this particular path.
All client tier test cases were run and CI job link is present in the JBS and it is green with fix.
Yes i am planning to backport it to the trains where the original issue was fixed.

@mrserb
Copy link
Member

mrserb commented Jul 22, 2021

We can save the header of the image as a binary array and then generate the image on the fly by filling the content with zeros. So it is not necessary to store such file in the repo.

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Jul 23, 2021

I went through TIFF spec and image provided in the bug to understand whether we can find a way to pass similar data to reproduce the issue.

The image attached in JBS has ICCProfile as one of the TIFFTag(This is considered as UNDEFINED tag by our standard reader) and its count is more than 1024000. And for this ICCProfile tag corresponding data is also present in the stream, it is not some corrupt header scenario where we can just write bad data in header and hit the issue. We divide the tag data in chunks on 1024000 bytes, when we are done reading first chunk of ICCProfile data and start reading the second chunk we hit this issue.

So to add regression test for this scenario we need more than 1024000 bytes of data in one of the TIFFTag type where the present change is done. We will not be able to pass that amount of data in byteArray stream. Also if we want to pass raw data as part of a TIFFTag i need relevant TIFFtag data like ICCProfile in the image attached in JBS.

So i am leaving discussion open so that others can give inputs on ways we can put relevant data into our TIFFImageWriter to hit this issue.

@mrserb
Copy link
Member

mrserb commented Jul 24, 2021

1024000 bytes does not look like a big problem especially if it is generated on the fly during the test execution?
You can try to load the data for our PYCC color profile and set one of its tag data to a big chunk like icSigCopyrightTag, or you can try to just fill it by zeros at the end.

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Aug 3, 2021

@mrserb added regression test, please review.

mrserb
mrserb approved these changes Aug 3, 2021
Copy link
Member

@mrserb mrserb left a comment

Thank you! look fine.
BTW Looks like stream.readFully(unit, 0, sz) can be simplified to the stream.readFully(unit)?

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Aug 4, 2021

Thank you! look fine.
BTW Looks like stream.readFully(unit, 0, sz) can be simplified to the stream.readFully(unit)?

Thanks for the review. Yes we can simplify readFully(), looks like we have other instances of similar usage of readFully() in this file. I would like to cleanup all of them but then i dont want to face issues while backporting it to different trains. So i would not refactor readFully() in this PR, if needed i will do refactoring as part of another bug.

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Aug 4, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Aug 4, 2021

Going to push as commit efcdcc7.
Since your change was applied there have been 206 commits pushed to the master branch:

  • 977b8c4: 8271836: runtime/ErrorHandling/ClassPathEnvVar.java fails with release VMs
  • 04134fc: 8264543: Cross modify fence optimization for x86
  • 9e76909: 8271824: mark hotspot runtime/CompressedOops tests which ignore external VM flags
  • e49b7d9: 8271828: mark hotspot runtime/classFileParserBug tests which ignore external VM flags
  • 68f7847: 8271825: mark hotspot runtime/LoadClass tests which ignore external VM flags
  • 3d40cac: 8271821: mark hotspot runtime/MinimalVM tests which ignore external VM flags
  • 33ec3a4: 8271744: mark hotspot runtime/getSysPackage tests which ignore external VM flags
  • b48f31d: 8271743: mark hotspot runtime/jni tests which ignore external VM flags
  • 66c653c: 8271721: Split gc/g1/TestMixedGCLiveThreshold into separate tests
  • 68dd828: 8271224: runtime/EnclosingMethodAttr/EnclMethodAttr.java doesn't check exit code
  • ... and 196 more: https://git.openjdk.java.net/jdk/compare/e7cdfebbeebb274b28495b469f39d5874af45e65...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Aug 4, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Aug 4, 2021
@openjdk
Copy link

openjdk bot commented Aug 4, 2021

@jayathirthrao Pushed as commit efcdcc7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@jayathirthrao jayathirthrao deleted the IOOBE_JDK-8270893 branch Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d integrated
3 participants