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

8262297: ImageIO.write() method will throw IndexOutOfBoundsException #6151

Closed
wants to merge 7 commits into from

Conversation

masyano
Copy link

@masyano masyano commented Oct 28, 2021

Could you please review the 8262297 bug fixes?

In this case, ImageIO.write() should throw java.io.IOException rather than java.lang.IndexOutOfBoundsException. IndexOutOfBoundsException is caught and wrapped in IIOException in ImageIO.write() with this fix. In addition, IndexOutOfBoundsException is not expected to throw by RandomAccessFile#write() according to its API specification. So it should be fixed.


Progress

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

Issue

  • JDK-8262297: ImageIO.write() method will throw IndexOutOfBoundsException

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6151

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 28, 2021

👋 Welcome back myano! 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 Oct 28, 2021
@openjdk
Copy link

openjdk bot commented Oct 28, 2021

@masyano The following labels will be automatically applied to this pull request:

  • client
  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added client core-libs labels Oct 28, 2021
@mlbridge
Copy link

mlbridge bot commented Oct 28, 2021

Webrevs

@@ -553,6 +553,9 @@ public void write(byte[] b) throws IOException {
* @param off the start offset in the data.
* @param len the number of bytes to write.
* @throws IOException if an I/O error occurs.
* @throws IndexOutOfBoundsException If {@code off} is negative,
* {@code len} is negative, or {@code len} is greater than
* {@code b.length - off}
Copy link
Contributor

@AlanBateman AlanBateman Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IOOBE is specified in the super interface, it's just not shown in the javadoc because it's a runtime exception. So I think what you want here is:

@throws IndexOutOfBoundsException {@inheritdoc}

Copy link
Member

@mrserb mrserb Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the parent class does not specify this exception via javadoc tags like "@throws IndexOutOfBoundsException", but instead just describe it in the method description

should we fix it separately from the imageio fix?

Copy link
Contributor

@AlanBateman AlanBateman Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the parent class does not specify this exception via javadoc tags like "@throws IndexOutOfBoundsException", but instead just describe it in the method description

should we fix it separately from the imageio fix?

Yes, it might be better to separate them because we'll like need a CSR if we are missing @throws in a few places.

Copy link
Author

@masyano masyano Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood to separate javadoc fix. I will remove the javadoc fix from this PR and issue another Bug ID.

Copy link
Contributor

@AlanBateman AlanBateman Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this means this PR will be image I/O only so I'll remove the core-libs label.

@prrace
Copy link
Contributor

prrace commented Oct 29, 2021

This needs looking at closely .. I don't know what's best but the imaging APIs are very exception heavy because of the arrays, indices, you name it that they consume. Most of the time its probably best for an IIO user to just get an IIOException wrapping the original cause. But there could be exceptions.

@AlanBateman
Copy link
Contributor

AlanBateman commented Oct 29, 2021

/label remove core-libs

@openjdk openjdk bot removed the core-libs label Oct 29, 2021
@openjdk
Copy link

openjdk bot commented Oct 29, 2021

@AlanBateman
The core-libs label was successfully removed.

@prrace
Copy link
Contributor

prrace commented Oct 29, 2021

I am not sure that this is the right thing at all here. You need to dig deeper.

  • If Image I/O reads the png and constructs a BufferedImage, then that BufferedImage ought to be valid.

  • If a valid image is written I would NOT expect an exception so catching it isn't the answer, fixing it is.

  • If the image being read is "truncated" so that the BufferedImage isn't valid, why was there no exception from read ?

  • If we do have an invalid BufferedImage then, in that case, we may indeed expect that write might throw an exception which needs to be caught but IIOBE is just one thing that could go wrong, isn't it ?

So for me this starts with the read, and a proper explanation of what happens there, and then an explanation as to what is wrong with the BufferedImage. I have no interest in slapping a sticking plaster on the observed symptom. It needs to be root cause explained and fixed.

@masyano
Copy link
Author

masyano commented Nov 12, 2021

This problem occurs because the input png image depth is 2 bits per pixel.

Because the png to read is the correct image at 2 bits per pixel, the BufferedImage is generated correctly and no exception is thrown. However, bmp can only create images with a depth of 0, 1, 4, 8, 16, 24, or 32 bits per pixel.
https://docs.microsoft.com/en-us/previous-versions/dd183376(v=vs.85)

Because the BMPImageWriter determines bitsPerPixel in the range of the color palette without checking the depth, images with depths other than 0, 1, 4, 8, 16, 24, 32 are set to the wrong bitsPerPixel and destScanlineBytes. Therefore, an incorrect length reference to the size of the Raster DataBuffer causes IIOBE to be thrown.

So the fix should change the canEncodeImage method to check the color depth and throw an IOException if it is not 0, 1, 4, 8, 16, 24, or 32.

/*
* @test
* @bug 8262297
* @summary Checks that ImageIO.write(..., ..., File) truncates the file
Copy link
Member

@jayathirthrao jayathirthrao Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with latest patch this summary should be updated to reflect what test is verifying.

Copy link
Author

@masyano masyano Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed summary to verify bit per pixel checking.

public class TruncatedPngTest {

public static void main(String[] args) {
String fileName = "0.png";
Copy link
Member

@jayathirthrao jayathirthrao Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A 2KB image will not take much space but it would be better if we can create 2bpp PNG image programmatically and use it in the test case.

Copy link
Author

@masyano masyano Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed test to use generated BufferdImage and more bpp.

@@ -1456,6 +1456,9 @@ protected boolean canEncodeImage(int compression, ImageTypeSpecifier imgType) {
}
int biType = imgType.getBufferedImageType();
int bpp = imgType.getColorModel().getPixelSize();
if (bpp != 0 && bpp != 1 && bpp != 4 && bpp != 8 && bpp != 16 && bpp != 24 && bpp != 32) {
Copy link
Member

@jayathirthrao jayathirthrao Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me.

Copy link
Member

@mrserb mrserb Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the exception message caused by this should be updated as well? Currently, it takes care of compression only.

Copy link
Author

@masyano masyano Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think the message should be change. I added bpp information at the end of the exception message.

mrserb
mrserb approved these changes Nov 19, 2021
Copy link
Member

@mrserb mrserb left a comment

I have no other comments.

@openjdk
Copy link

openjdk bot commented Nov 19, 2021

@masyano 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:

8262297: ImageIO.write() method will throw IndexOutOfBoundsException

Reviewed-by: serb, jdv

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 382 new commits pushed to the master branch:

  • 7e54d06: 8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories
  • eb4d886: 8277504: Use String.stripTrailing instead of hand-crafted method in SwingUtilities2
  • 9879920: 8277825: Remove unused ReferenceProcessorPhaseTimes::_sub_phases_total_time_ms
  • f788834: 8277786: G1: Rename log2_card_region_per_heap_region used in G1CardSet
  • 3034ae8: 8277631: ZGC: CriticalMetaspaceAllocation asserts
  • f0136ec: 8275687: runtime/CommandLine/PrintTouchedMethods test shouldn't catch RuntimeException
  • 21e302a: 8270435: UT: MonitorUsedDeflationThresholdTest failed: did not find too_many string in output
  • a81e4fc: 8258117: jar tool sets the time stamp of module-info.class entries to the current time
  • 26472bd: 8277811: ProblemList vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java
  • b5841ba: 8277806: 4 tools/jar failures per platform after JDK-8272728
  • ... and 372 more: https://git.openjdk.java.net/jdk/compare/a2f2d8fcf511de2754a76a5d9f9acdfef462919b...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mrserb, @jayathirthrao) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Nov 19, 2021
@masyano
Copy link
Author

masyano commented Nov 22, 2021

/integrate

@openjdk openjdk bot added the sponsor label Nov 22, 2021
@openjdk
Copy link

openjdk bot commented Nov 22, 2021

@masyano
Your change (at version 22b8294) is now ready to be sponsored by a Committer.

imageType = BufferedImage.TYPE_BYTE_INDEXED;
}
BufferedImage img = new BufferedImage(10, 10, imageType, (IndexColorModel)cm);
ImageIO.write(img, "BMP", new File("test.bmp"));
Copy link
Member

@jayathirthrao jayathirthrao Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed before sponsoring that this test will leave test.bmp file.
We should create temporary file and delete it on exit.

Copy link
Member

@jayathirthrao jayathirthrao Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am withdrawing my approval, since this needs to be fixed.

Copy link
Author

@masyano masyano Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it to use createTempFile and deleteOnExit.

@jayathirthrao
Copy link
Member

jayathirthrao commented Nov 23, 2021

/reviewer remove @jayathirthrao

@openjdk
Copy link

openjdk bot commented Nov 23, 2021

@jayathirthrao Only the author (@masyano) is allowed to issue the reviewer command.

imageType = BufferedImage.TYPE_BYTE_INDEXED;
}
BufferedImage img = new BufferedImage(10, 10, imageType, (IndexColorModel)cm);
ImageIO.write(img, "BMP", new File("test.bmp"));
Copy link
Member

@jayathirthrao jayathirthrao Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am withdrawing my approval, since this needs to be fixed.

@jayathirthrao
Copy link
Member

jayathirthrao commented Nov 23, 2021

/reviewers 2

@openjdk
Copy link

openjdk bot commented Nov 23, 2021

@jayathirthrao
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@openjdk openjdk bot removed sponsor ready labels Nov 23, 2021
@openjdk openjdk bot added the ready label Nov 24, 2021
@masyano
Copy link
Author

masyano commented Nov 26, 2021

/integrate

@openjdk openjdk bot added the sponsor label Nov 26, 2021
@openjdk
Copy link

openjdk bot commented Nov 26, 2021

@masyano
Your change (at version 9d0f386) is now ready to be sponsored by a Committer.

@jayathirthrao
Copy link
Member

jayathirthrao commented Dec 1, 2021

/sponsor

@openjdk
Copy link

openjdk bot commented Dec 1, 2021

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

  • da2be99: 8277026: Remove blank lines remaining from snippet markup
  • 0a01baa: 8277986: Typo in javadoc of java.util.zip.ZipEntry#setTime
  • 7049c13: 8231107: Allow store password to be null when saving a PKCS12 KeyStore
  • ab867f6: 8272162: S4U2Self ticket without forwardable flag
  • dd73e3c: 8277814: ConcurrentRefineThread should report rate when deactivating
  • 65251f7: 8151594: Move concurrent refinement thread activation logging out of GC pause
  • f1c20e9: 8190748: java/text/Format/DateFormat/DateFormatTest.java and NonGregorianFormatTest fail intermittently
  • 2942646: 8276683: Malformed Javadoc inline tags in JDK source in com/sun/tools/javac/util/RawDiagnosticFormatter.java
  • e30e676: 8277606: String(String) constructor could copy hashIsZero
  • 5a4a9bb: 8278019: ProblemList java/awt/dnd/BadSerializationTest/BadSerializationTest.java on linux and windows
  • ... and 424 more: https://git.openjdk.java.net/jdk/compare/a2f2d8fcf511de2754a76a5d9f9acdfef462919b...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Dec 1, 2021
@openjdk
Copy link

openjdk bot commented Dec 1, 2021

@jayathirthrao @masyano Pushed as commit c733193.

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

@jayathirthrao
Copy link
Member

jayathirthrao commented Dec 1, 2021

This change has caused regression : https://bugs.openjdk.java.net/browse/JDK-8278047
So this change will be reverted for further analysis.

@PKI-YanliSang
Copy link

PKI-YanliSang commented Dec 14, 2021

@jayathirthrao @masyano @prrace @AlanBateman We also meet the same issue in PNGImageWriter.write, Do you have any idea how to fix it ? Thanks in advance.

at java.desktop/javax.imageio.ImageWriter.write(ImageWriter.java:613)
at java.desktop/com.sun.imageio.plugins.png.PNGImageWriter.write(PNGImageWriter.java:1283)
at java.desktop/com.sun.imageio.plugins.png.PNGImageWriter.write_IDAT(PNGImageWriter.java:1050)
at java.desktop/com.sun.imageio.plugins.png.IDATOutputStream.finish(PNGImageWriter.java:273)
at java.desktop/com.sun.imageio.plugins.png.IDATOutputStream.deflate(PNGImageWriter.java:248)
at java.desktop/com.sun.imageio.plugins.png.IDATOutputStream.finishChunk(PNGImageWriter.java:200)
at java.desktop/javax.imageio.stream.FileCacheImageOutputStream.seek(FileCacheImageOutputStream.java:170)
java.lang.IndexOutOfBoundsException

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client integrated
6 participants