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.
