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

[Prasanta Sadhukhan]

OK. Thanks for the clarification. +1

Regards
Prasanta
On 30-Apr-20 4:47 PM, Jayathirth D v wrote:

Hi Prasanta,

Thats why I pointed to JDK-8195841 <https://bugs.openjdk.java.net/browse/JDK-8195841> where PNGImageReader.readNullTerminatedString() is changed. Second argument that is getting passed to readNullTerminatedString() is maximum length including null termination. While reading the string in readNullTerminatedString() we exit as soon as we hit null termination, it is not necessary to read 80 bytes.

So in case where we have read 80 bytes(where we have corrupted PNG) and last byte is not null termination we throw IIOException.

private String readNullTerminatedString(String charset, int maxLen) throws IOException {

        ByteArrayOutputStream baos = new ByteArrayOutputStream();
        int b = 0;
        int count = 0;
        while ((maxLen > count++) && ((b = stream.read()) != 0)) {
            if (b == -1) throw new EOFException();
            baos.write(b);
        }
        if (b != 0) {
            throw new IIOException("Found non null terminated string");
        }
        return new String(baos.toByteArray(), charset);
    }

So overall in case of reader we throw exception when we have longer than 79 bytes strings and we follow same approach in writer as per specification.

Thanks,
Jay

On 30-Apr-2020, at 3:35 PM, Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com <mailto:prasanta.sadhuk...@oracle.com>> wrote:

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.sadhuk...@oracle.com <mailto:prasanta.sadhuk...@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.sadhuk...@oracle.com <mailto:prasanta.sadhuk...@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