[10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

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

[10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Jayathirth D v

Hello All,

 

Please review the following fix in JDK10 :

 

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

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

 

Issue : PNGImageReader throws “java.lang.IllegalArgumentException: Empty region!” when the IHDR width/height value in the header is negative.

 

Root cause : In PNGImageReader.readHeader() we only check whether the IHDR width/height is not equal to 0(which is compliant with PNG specification), we don’t check whether the value is negative or not.

 

Solution : Although PNG specification mentions only 0 as invalid value for IHDR width/height we should also not allow IHDR width/height with negative values. Extend the check present in PNGImageReader.readHeader() to verify negative values for IHDR width and height.

 

Thanks,

Jay

 

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Prahalad kumar Narayanan
Hello Jay

The code change looks good to me.
However, I 'm not sure if we could include the test-case here.

The test case contains the Base64 encoded image in a String.
To my knowledge, we cannot import images into JDK (even encoded in String) unless we are sure of the copyrights.

Thanks
Have a good day

Prahalad

----- Original Message -----
From: Jayathirth D V
Sent: Wednesday, November 08, 2017 2:23 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Hello All,

Please review the following fix in JDK10 :

Bug : https://bugs.openjdk.java.net/browse/JDK-8190512 
Webrev : http://cr.openjdk.java.net/~jdv/8190512/webrev.00/ 

Issue : PNGImageReader throws "java.lang.IllegalArgumentException: Empty region!" when the IHDR width/height value in the header is negative.

Root cause : In PNGImageReader.readHeader() we only check whether the IHDR width/height is not equal to 0(which is compliant with PNG specification), we don't check whether the value is negative or not.

Solution : Although PNG specification mentions only 0 as invalid value for IHDR width/height we should also not allow IHDR width/height with negative values. Extend the check present in PNGImageReader.readHeader() to verify negative values for IHDR width and height.

Thanks,
Jay
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Jayathirth D v
Hi Prahalad,

Thanks for your review.

I remember using byteArray from test case in JBS previously for one of the fix so I searched for the same :  https://bugs.openjdk.java.net/browse/JDK-8066904 . This is one of the initial issues that I worked on.

I also see that you recently didn't include submitter test case for some of BMP issues like https://bugs.openjdk.java.net/browse/JDK-6852563 & https://bugs.openjdk.java.net/browse/JDK-8167278 

I would like to get inputs from others also so that we can decide to use or not to use the encoded string for image information from submitter in the JDK.

Regards,
Jay

-----Original Message-----
From: Prahalad Kumar Narayanan
Sent: Wednesday, November 08, 2017 3:16 PM
To: Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Hello Jay

The code change looks good to me.
However, I 'm not sure if we could include the test-case here.

The test case contains the Base64 encoded image in a String.
To my knowledge, we cannot import images into JDK (even encoded in String) unless we are sure of the copyrights.

Thanks
Have a good day

Prahalad

----- Original Message -----
From: Jayathirth D V
Sent: Wednesday, November 08, 2017 2:23 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Hello All,

Please review the following fix in JDK10 :

Bug : https://bugs.openjdk.java.net/browse/JDK-8190512 
Webrev : http://cr.openjdk.java.net/~jdv/8190512/webrev.00/ 

Issue : PNGImageReader throws "java.lang.IllegalArgumentException: Empty region!" when the IHDR width/height value in the header is negative.

Root cause : In PNGImageReader.readHeader() we only check whether the IHDR width/height is not equal to 0(which is compliant with PNG specification), we don't check whether the value is negative or not.

Solution : Although PNG specification mentions only 0 as invalid value for IHDR width/height we should also not allow IHDR width/height with negative values. Extend the check present in PNGImageReader.readHeader() to verify negative values for IHDR width and height.

Thanks,
Jay
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Brian Burkhalter-2
Hi Jay,

I don’t see anything wrong with using an encoded string if it is not derived from a copyrighted image: it’s better than checking an image binary into the SCM. It would be nice however to test for negative height as well in order to get full coverage.

Thanks,

Brian

On Nov 8, 2017, at 2:35 AM, Jayathirth D V <[hidden email]> wrote:

I would like to get inputs from others also so that we can decide to use or not to use the encoded string for image information from submitter in the JDK.

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Jayathirth D v

Hi Brian,

 

Thanks for your review.

The encoded string present in test case has only the critical chunks, I have checked and verified the same with Phil also offline and it looks like it doesn’t have any text chunks like iTXt, tEXt, and zTXt which may contain copyright information. I have sent mail to submitter also to ask for copyright issues, if any with the provided images. Waiting for the response from submitter. But looking into encoded stream it looks to be safe from copyright issues.

 

Regarding negative height in IHDR. I think we can’t create PNG images with negative IHDR height using ImageIO so I have asked the submitter for another sample image with negative height and non-negative width. I can get such an image I will use encoded stream and update the test case.

 

Thanks,

Jay

 

From: Brian Burkhalter
Sent: Thursday, November 09, 2017 12:52 AM
To: Jayathirth D V
Cc: Prahalad Kumar Narayanan; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

 

Hi Jay,

 

I don’t see anything wrong with using an encoded string if it is not derived from a copyrighted image: it’s better than checking an image binary into the SCM. It would be nice however to test for negative height as well in order to get full coverage.

 

Thanks,

 

Brian

 

On Nov 8, 2017, at 2:35 AM, Jayathirth D V <[hidden email]> wrote:



I would like to get inputs from others also so that we can decide to use or not to use the encoded string for image information from submitter in the JDK.

 

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Jayathirth D v

Hi Brian,

 

Submitter shared PNG image with negative IHDR height also.

And he also clarified from his side that there are no copyright issues.

 

I have updated the test case to check for negative width and negative height input.

Please find updated webrev for review:

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

 

Thanks,

Jay

 

From: Jayathirth D V
Sent: Thursday, November 09, 2017 12:38 PM
To: Brian Burkhalter
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

 

Hi Brian,

 

Thanks for your review.

The encoded string present in test case has only the critical chunks, I have checked and verified the same with Phil also offline and it looks like it doesn’t have any text chunks like iTXt, tEXt, and zTXt which may contain copyright information. I have sent mail to submitter also to ask for copyright issues, if any with the provided images. Waiting for the response from submitter. But looking into encoded stream it looks to be safe from copyright issues.

 

Regarding negative height in IHDR. I think we can’t create PNG images with negative IHDR height using ImageIO so I have asked the submitter for another sample image with negative height and non-negative width. I can get such an image I will use encoded stream and update the test case.

 

Thanks,

Jay

 

From: Brian Burkhalter
Sent: Thursday, November 09, 2017 12:52 AM
To: Jayathirth D V
Cc: Prahalad Kumar Narayanan; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

 

Hi Jay,

 

I don’t see anything wrong with using an encoded string if it is not derived from a copyrighted image: it’s better than checking an image binary into the SCM. It would be nice however to test for negative height as well in order to get full coverage.

 

Thanks,

 

Brian

 

On Nov 8, 2017, at 2:35 AM, Jayathirth D V <[hidden email]> wrote:

 

I would like to get inputs from others also so that we can decide to use or not to use the encoded string for image information from submitter in the JDK.

 

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Brian Burkhalter-2
Hi Jay,

Looks good.

Thanks,

Brian

On Nov 10, 2017, at 12:31 AM, Jayathirth D V <[hidden email]> wrote:

IHDR height also.
And he also clarified from his side that there are no copyright issues.
 
I have updated the test case to check for negative width and negative height input.
Please find updated webrev for review:

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190512: PngReader incorrectly throws IllegalArgumentException for malformed images with negative dimensions

Phil Race
+1

-phil

On 11/13/2017 08:27 AM, Brian Burkhalter wrote:
Hi Jay,

Looks good.

Thanks,

Brian

On Nov 10, 2017, at 12:31 AM, Jayathirth D V <[hidden email]> wrote:

IHDR height also.
And he also clarified from his side that there are no copyright issues.
 
I have updated the test case to check for negative width and negative height input.
Please find updated webrev for review: