[11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Jayathirth D v

Hello All,

 

Please review the following fix in JDK11 :

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8191073

Webrev : http://cr.openjdk.java.net/~jdv/8191073/webrev.00/

 

Issue: When we try to read image data from JPEG input stream having tables-only information JPEGImageReader throws IndexOutOfBoundsException.

 

Exception in thread "main" java.lang.IndexOutOfBoundsException: Index -1 out-of-bounds for length 0
        at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
        at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
        at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:248)
        at java.base/java.util.Objects.checkIndex(Objects.java:372)
        at java.base/java.util.ArrayList.get(ArrayList.java:440)
        at java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageReader.checkTablesOnly(JPEGImageReader.java:378)
        at java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageReader.gotoImage(JPEGImageReader.java:493)
        at java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageReader.readHeader(JPEGImageReader.java:716)
        at java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageReader.readInternal(JPEGImageReader.java:1173)
        at java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageReader.read(JPEGImageReader.java:1153)
        at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
        at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
        at JpegReaderOOBIssue.main(JpegReaderOOBIssue.java:25)

 

Root cause: readNativeHeader() function returns the first header as tables-only image in checkTablesOnly() function. Because of this we don’t update the start of this stream position as one of the imagePositions. After that in checkTablesOnly() we try to find any image data after stream metadata using hasNextImage() and it also returns false. So we have a input stream which contains only streamMetadata(tables-only image).
In the same checkTablesOnly() we try to get initial imagePosition for this input stream in seekForwardOnly case. But since it is empty and we try "imagePositions.get(imagePositions.size()-1)" it throws IndexOutOfBoundsException.

 

Solution:

We should make changes at 2 places to fix this issue :

1) In checkTablesOnly() function if seekForward flag is enabled and the input stream is just tables-only we should not try to access imagePositions variable as it will be empty. imagePositions list is in 1:1 relationship with number on image indices available in given input stream.

2) In checkTablesOnly() function when we get to know that the given input stream is just tables only image we should maintain that state in a boolean variable like “tablesOnlyStream” so that if we try to access image information in gotoImage() we should throw an IIOException mentioning that there is no image data available.

 

Thanks,

Jay

 

Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Brian Burkhalter-2
Hi Jay,
General comment: I think it’s better to use “//“ than “/*…*/“ for method-internal comments:

JpegImageReader L373-375, 392-397, 516-519
JpegTablesOnlyReadTest L61-64

As to the fix, is the new tablesOnlyStream boolean really necessary? For example at line 520 in the new version could not one do this
        if (imagePositions.isEmpty()) { // line 520 of new version
            throw new IIOException("No image data present to read");
        }
instead? I don’t see where the value of this variable is used anywhere else.

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Jayathirth D v

Hi Brian,

 

Thanks for your valuable inputs.

 

As I was debugging the issue I started capturing this “tablesOnlyStream” boolean variable in checkTablesOnly() method and I started using it at other places also. As rightly pointed by you we don’t need this “tablesOnlyStream” as it not used anywhere else and we can use imagePositions to know whether it is tables-only stream.

 

I have removed the usage of tablesOnlyStream variable in code and updated the comment section.

Please find updated webrev for review:

http://cr.openjdk.java.net/~jdv/8191073/webrev.01/

 

Thanks,

Jay

 

From: Brian Burkhalter
Sent: Saturday, January 06, 2018 4:43 AM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

 

Hi Jay,

General comment: I think it’s better to use “//“ than “/*…*/“ for method-internal comments:

 

JpegImageReader L373-375, 392-397, 516-519

JpegTablesOnlyReadTest L61-64

 

As to the fix, is the new tablesOnlyStream boolean really necessary? For example at line 520 in the new version could not one do this

        if (imagePositions.isEmpty()) { // line 520 of new version
            throw new IIOException("No image data present to read");
        }

instead? I don’t see where the value of this variable is used anywhere else.

 

Thanks,

 

Brian

Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Brian Burkhalter-2
Hi Jay,

Sorry but I have a few picky comments.

 377         // If imagePositions list doesn't contain any of the image stream
 378         // starting position(i.e tables-only image) we should not try to access
 379         // imagePositions.size() as it done below, because it will lead to
 380         // IndexOutOfBoundsException with index -1.

You might consider this verbiage instead:

If the image positions list is empty as in the case of a tables-only stream, then attempting to access the element at index
imagePositions.size() - 1 will cause an IndexOutOfBoundsException.

 381         if (seekForwardOnly && (!(imagePositions.isEmpty()))) {

I think this is more readable if some parentheses are eliminated:

 381         if (seekForwardOnly && !imagePositions.isEmpty()) {

 499         // We should not try to read image information from an input stream
 500         // which only contains tables-only(StreamMetadata) information.

You might consider this verbiage instead:

If the image positions list is empty as in the case of a tables-only stream, then no image data can be read.

No need to update the webrev.

Thanks,

Brian

On Jan 7, 2018, at 11:23 PM, Jayathirth D V <[hidden email]> wrote:

I have removed the usage of tablesOnlyStream variable in code and updated the comment section.
Please find updated webrev for review:

Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Jayathirth D v

Hello Brian,

 

Thanks for reviewing the changes.

 

I am posting updated webrev since I need one more review and approval  from 2d-dev list.

Updated webrev for review : http://cr.openjdk.java.net/~jdv/8191073/webrev.02/

 

Thanks,

Jay

 

From: Brian Burkhalter
Sent: Tuesday, January 09, 2018 2:30 AM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

 

Hi Jay,

 

Sorry but I have a few picky comments.

 

 377         // If imagePositions list doesn't contain any of the image stream

 378         // starting position(i.e tables-only image) we should not try to access

 379         // imagePositions.size() as it done below, because it will lead to

 380         // IndexOutOfBoundsException with index -1.

 

You might consider this verbiage instead:

 

If the image positions list is empty as in the case of a tables-only stream, then attempting to access the element at index

imagePositions.size() - 1 will cause an IndexOutOfBoundsException.

 

 381         if (seekForwardOnly && (!(imagePositions.isEmpty()))) {

 

I think this is more readable if some parentheses are eliminated:

 

 381         if (seekForwardOnly && !imagePositions.isEmpty()) {

 

 499         // We should not try to read image information from an input stream

 500         // which only contains tables-only(StreamMetadata) information.

 

You might consider this verbiage instead:

 

If the image positions list is empty as in the case of a tables-only stream, then no image data can be read.

 

No need to update the webrev.

 

Thanks,

 

Brian

 

On Jan 7, 2018, at 11:23 PM, Jayathirth D V <[hidden email]> wrote:



I have removed the usage of tablesOnlyStream variable in code and updated the comment section.

Please find updated webrev for review:

 

Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Prahalad kumar Narayanan
Hello Jay

I imported your changes & tested the same.
The change doesn't cause new regressions & attached test case passes.

Looks good.

Thank you
Have a good day

Prahalad N.

----- Original Message -----
From: Jayathirth D V
Sent: Tuesday, January 09, 2018 2:00 PM
To: Brian Burkhalter; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Hello Brian,

Thanks for reviewing the changes.

I am posting updated webrev since I need one more review and approval  from 2d-dev list.
Updated webrev for review : http://cr.openjdk.java.net/~jdv/8191073/webrev.02/ 

Thanks,
Jay

From: Brian Burkhalter
Sent: Tuesday, January 09, 2018 2:30 AM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Hi Jay,

Sorry but I have a few picky comments.

 377         // If imagePositions list doesn't contain any of the image stream
 378         // starting position(i.e tables-only image) we should not try to access
 379         // imagePositions.size() as it done below, because it will lead to
 380         // IndexOutOfBoundsException with index -1.

You might consider this verbiage instead:

If the image positions list is empty as in the case of a tables-only stream, then attempting to access the element at index
imagePositions.size() - 1 will cause an IndexOutOfBoundsException.

 381         if (seekForwardOnly && (!(imagePositions.isEmpty()))) {

I think this is more readable if some parentheses are eliminated:

 381         if (seekForwardOnly && !imagePositions.isEmpty()) {

 499         // We should not try to read image information from an input stream
 500         // which only contains tables-only(StreamMetadata) information.

You might consider this verbiage instead:

If the image positions list is empty as in the case of a tables-only stream, then no image data can be read.

No need to update the webrev.

Thanks,

Brian

On Jan 7, 2018, at 11:23 PM, Jayathirth D V <[hidden email]> wrote:

I have removed the usage of tablesOnlyStream variable in code and updated the comment section.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8191073/webrev.01/