[OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt CRC for IEND chunk throws IIOException

Tue Nov 20 10:16:53 UTC 2018

Thanks for the clarification Jay.
The changes look fine.

Krishna

-----Original Message-----
From: Jayathirth D V 
Sent: Tuesday, November 20, 2018 3:37 PM
To: Krishna Addepalli <krishna.addepalli at oracle.com>; 2d-dev at openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt CRC for IEND chunk throws IIOException

Hi Krishna,

Thanks for the review.

I have added detail comment mentioning why we don’t read CRC data for IEND chunk.
We can add debug/warning info mentioning that CRC data is not present/ corrupt, but after IEND chunk type there can be any length of stream data. In the stream present for this bug we have 3 byte data and we are throwing IIOException. If some image has stream data of more than 4 byte after IEND chunk type, in that case also we would assume the data present as CRC for IEND chunk and try to validate it. Basically there is no way we can verify that stream data present after IEND chunk type is CRC for IEND chunk. So it's better if stop reading metadata as soon as we hit IEND chunk type, reading more metadata and trying to validate it is not needed.

Also as I have added in JBS many other decoders(Ubuntu GIMP, IrfanView, Windows paint, Mac Preview, Ubuntu Image viewer) also work this way and they are not worried about CRC for IEND chunk.

Thanks,
Jay

-----Original Message-----
From: Krishna Addepalli
Sent: Tuesday, November 20, 2018 2:32 PM
To: Jayathirth D V; 2d-dev at openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt CRC for IEND chunk throws IIOException

Hi Jay,

While the exception is suppressed with your fix, I think it makes sense to add some logging of this fact, so that it could aid in debugging.

Thanks,
Krishna

-----Original Message-----
From: Jayathirth D V
Sent: Friday, November 16, 2018 12:30 PM
To: 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt CRC for IEND chunk throws IIOException

Thanks Sergey for the approval.
Need one more review.
Please review the webrev :
http://cr.openjdk.java.net/~jdv/8211422/webrev.01/ 

Regards,
Jay

-----Original Message-----
From: Sergey Bylokhov
Sent: Thursday, November 15, 2018 10:58 PM
To: Jayathirth D V; 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt CRC for IEND chunk throws IIOException

Looks fine.

On 14/11/2018 04:35, Jayathirth D V wrote:
> Hi Sergey,
> 
> Thanks for the review.
> 
> I have updated the code to not move the changes out of switch. Apart from that I have refined comments to make it clear why we are not reading CRC for IEND chunk. Also chunkCRC value needs to be initialized because of this update, initialized value of chunkCRC will not be used anywhere in the logic.
> 
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8211422/webrev.01/
> 
> Thanks,
> Jay
> 
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Wednesday, November 14, 2018 4:31 AM
> To: Jayathirth D V; 2d-dev at openjdk.java.net
> Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with 
> corrupt CRC for IEND chunk throws IIOException
> 
> Hi, Jay.
> 
> Probably it will be better to not to change the code inside switch, and only add the check below around of reading CRC:
>     "if (chunkType != IEND_TYPE) {"
> 
> The current fix may throw "Invalid chunk length" when seek/flush the data for IEND_TYPE, which is not correct.
> 
> 
> On 12/11/2018 20:22, Jayathirth D V wrote:
>> Hello All,
>>
>> Gentle reminder for review.
>>
>> Thanks,
>>
>> Jay
>>
>> *From:*Jayathirth Rao
>> *Sent:* Tuesday, October 23, 2018 7:09 PM
>> *To:* 2d-dev at openjdk.java.net
>> *Subject:* [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with 
>> corrupt CRC for IEND chunk throws IIOException
>>
>> Hello All,
>>
>> Please review the following fix in JDK12:
>>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8211422
>>
>> Webrev: http://cr.openjdk.java.net/~jdv/8211422/webrev.00/
>>
>> Issue : When we try to read PNG image with corrupt/no 4 byte CRC data for IEND chunk we throw IIOException. We see this issue only after JDK-8164971 <https://bugs.openjdk.java.net/browse/JDK-8164971>.
>>
>> Root cause : In JDK-8164971 <https://bugs.openjdk.java.net/browse/JDK-8164971> fix we made changes to continue reading metadata until we reach IEND chunk. Before JDK-8164971 <https://bugs.openjdk.java.net/browse/JDK-8164971> change we used to stop reading metadata as soon as we hit first IDAT chunk. According to PNG spec there can be more than one IDAT chunk/ more headers after IDAT chunk. So we need to read metadata until we hit IEND chunk. But in case of this bug we have IEND chunk but it has corrupt/no CRC chunk, so we throw IIOException(According to PNG spec every PNG chunk should contain 4 byte CRC). But lot of other decoders accept these kind of images which has no CRC chunk for IEND chunk.
>>
>> Solution : There is no need to verify CRC for IEND chunk(Chunk data length for IEND is 0). Stop reading metadata once we hit Chunk type info for IEND chunk.
>>
>> Thanks,
>>
>> Jay
>>
> 
> 
> --
> Best regards, Sergey.
> 

--
Best regards, Sergey.