[OpenJDK 2D-Dev] [15] RFR JDK-8242557: Add length limit for strings in PNGImageWriter

[Prasanta Sadhukhan]

Hi Jay,

 >> we will throw exception in case of reader also

But in the fix, only PNGWriter is changed. I did not see in existing 
PNGReader any exception being thrown for > 79 bytes. I see in reader, we 
have

private void parse_iCCP_chunk(int chunkLength) throws IOException {
         String keyword = readNullTerminatedString("ISO-8859-1", 80);
         int compressedProfileLength = chunkLength - keyword.length() - 2;
         if (compressedProfileLength <= 0) {
             throw new IIOException("iCCP chunk length is not proper");
         }

so chunkLength can be say, 100, keyWord.lenth = 79 so 
compressedProfileLength = 100-79-2=19 so we will not throw IOException. 
Am I missing something?

Regards
Prasanta

On 30-Apr-20 3:23 PM, Jayathirth D v wrote:
> Hi Prasanta,
>
> I didnt say reader will decode more than 79 bytes but user might 
> expect more than 79 bytes since we allowed more than 79 bytes write.
> While reading non-standard PNG chunks(having greater than 79 bytes 
> null terminated string) we will throw exception in case of reader 
> also. Its a stream, we will not know the length and we rely completely 
> on null termination and we expect it to be spec complaint and not more 
> than 79 bytes.
>
> Thanks,
> Jay
>
>> On 30-Apr-2020, at 2:51 PM, Prasanta Sadhukhan 
>> <prasanta.sadhukhan at oracle.com 
>> <mailto:prasanta.sadhukhan at oracle.com>> wrote:
>>
>> Hi Jay,
>>
>> But why reader will read more than 79 bytes from the chunk..It should 
>> also limit it's reading to 1st 79 bytes (if the chunks we get from 
>> non-oracle non-standard PNG writer is more than 79bytes) as per spec, no?
>>
>> Regards
>> Prasanta
>> On 30-Apr-20 2:01 PM, Jayathirth D v wrote:
>>> Hi Prasanta,
>>>
>>> Thanks for the review.
>>> If we consume only 79 bytes and continue writing the image, it will 
>>> lead to unexpected behaviour when user tries to read the image 
>>> expecting longer than 79 string data. Our reader is also tightly 
>>> spec compliant for null terminated strings after JDK-8195841 
>>> <https://bugs.openjdk.java.net/browse/JDK-8195841> and JDK-8191023 
>>> <https://bugs.openjdk.java.net/browse/JDK-8191023>. Its better to be 
>>> spec compliant than leaving things open for interpretation.
>>>
>>> Regards,
>>> Jay
>>>
>>>
>>>> On 29-Apr-2020, at 1:22 PM, Prasanta Sadhukhan 
>>>> <prasanta.sadhukhan at oracle.com 
>>>> <mailto:prasanta.sadhukhan at oracle.com>> wrote:
>>>>
>>>> Hi Jay,
>>>>
>>>> Looks good to me . Although I have a question which is, instead of 
>>>> throwing Exception, can we proceed with 1st 79 bytes of these 
>>>> chunks and continue?
>>>>
>>>> Regards
>>>> Prasanta
>>>> On 27-Apr-20 10:51 PM, Philip Race wrote:
>>>>> I reviewed http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html
>>>>> and I think you covered all the cases.
>>>>>
>>>>> I also reviewed the reader and it seems we already check only up 
>>>>> to 80 chars there.
>>>>>
>>>>> I note we assume the same max length of 80 for the language tag 
>>>>> for iTxt.
>>>>> The spec. doesn't specify a limit but I think 80 is more than 
>>>>> generous here.
>>>>>
>>>>> +1
>>>>>
>>>>> -phil.
>>>>>
>>>>> On 4/27/20, 9:59 AM, Jayathirth D v wrote:
>>>>>> Hello All,
>>>>>>
>>>>>> Please review the following fix for JDK 15:
>>>>>>
>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8242557
>>>>>> Webrev : http://cr.openjdk.java.net/~jdv/8242557/webrev.00/ 
>>>>>> <http://cr.openjdk.java.net/%7Ejdv/8242557/webrev.00/>
>>>>>>
>>>>>> Issue : PNG specification restricts length of strings in some 
>>>>>> chunks to 
>>>>>> 79(http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html ) 
>>>>>> excluding null termination. But our writer implementation has no 
>>>>>> such check.
>>>>>> Solution : Add checks in different chunks where there must be 
>>>>>> restrictions.
>>>>>>
>>>>>> Thanks,
>>>>>> Jay
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20200430/cb12b1da/attachment-0001.htm>