[OpenJDK 2D-Dev] [11] RFR JDK-8191023: PngReader throws NegativeArraySizeException when keyword length exceeds chunk size

[Sergey Bylokhov]

Looks fine.

On 25/01/2018 06:02, Jayathirth D V wrote:
> Hi Sergey & Prahalad,
> 
> Thanks for your inputs.
> 
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191023/webrev.02/
> 
> Regards,
> Jay
> 
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Thursday, January 25, 2018 3:23 PM
> To: Sergey Bylokhov; Jayathirth D V; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-8191023: PngReader throws NegativeArraySizeException when keyword length exceeds chunk size
> 
> Hello Jay
> 
> Kindly ignore the point mentioned below with ( >) when you follow-up.
> Lines 431 and 671 in webrev.01 are correct and we needn't change subtraction value.
> 
>> Correction to the subtraction value from -2 to -1 in the following lines
>>       431         int compressedProfileLength = chunkLength - keyword.length() - 2;
>>       671         int textLength = chunkLength - keyword.length() - 2;
> 
> Thanks
> Have a good day
> 
> Prahalad N.
> 
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Thursday, January 25, 2018 11:15 AM
> To: Sergey Bylokhov; Jayathirth D V; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-8191023: PngReader throws NegativeArraySizeException when keyword length exceeds chunk size
> 
> Hello Jay
> 
> Sergey has suggested a valid point- If the CRC was generated with incorrect chunk data, the approach I suggested will succeed and we would still end up in the exception within the parse_<chunk> methods. Hence going by if (...) checks is appropriate.
> 
> Few minor corrections to the webrev.01 are as follows:
> 
> . Correction to the subtraction value from -2 to -1 in the following lines
>    The original code had -2 because, one extra Unsigned byte was getting read prior to the size calculation.
>    The current changes advance the calculation by a few lines ahead of reading extra UnsignedByte.
> 
>        431         int compressedProfileLength = chunkLength - keyword.length() - 2;
>        671         int textLength = chunkLength - keyword.length() - 2;
> 
> . The result of these two operations are not the same due to the order of execution.
>    If you had checked the logic and corrected the usage, it should be fine.
> 
>        // Original code
>        518         chunkLength -= metadata.sPLT_paletteName.length() + 1;
>        // Code in the webrev
>        526         int remainingChunkLength = chunkLength -
>        527                 metadata.sPLT_paletteName.length() + 1;
> 
> . All the if conditions in the proposed fix check whether remSize < 0. I believe, we will need size "<=" 0 check.
>    Reason is that new <Type>[0] will succeed but subsequent read with stream.read* method will throw "ArrayIndexOutOfBounds: 0" exception.
> 
> Thank you
> Have a good day
> 
> Prahalad N.
> 
> 
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Thursday, January 25, 2018 5:44 AM
> To: Prahalad Kumar Narayanan; Jayathirth D V; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-8191023: PngReader throws NegativeArraySizeException when keyword length exceeds chunk size
> 
> On 22/01/2018 23:17, Prahalad Kumar Narayanan wrote:
>> My suggestion was to -
>> . 'Generate' CRC from Chunk data and compare it with the retrieved value at Line 731 'before' proceeding to process any of the chunks.
>> . In mal-formed chunks (corrupted chunk length /or chunk data), the CRC check will fail thus giving an effective way to identify a valid chunk.
>> . Many of the if (...) conditions that 've been added to parse_<Chunk> methods can be avoided with CRC check done upfront.
> 
> Is it possible that CRC will be broken/malformed as well as a chunk data?(For example if it is generated on top of incorrect data?), if yes then we should check the data itself for correct/incorrect values.
> 
> --
> Best regards, Sergey.
> 


-- 
Best regards, Sergey.