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.
