Re: [OpenJDK 2D-Dev] RFR: 8244113 : [TESTBUG] java/awt/font/Rotate/RotatedSyntheticBoldTest.java test comments interpreted as args.

[Philip Race]

Updated with a couple of stability fixes :
1) to use invokeAndWait() instead of invokeLater()
2) To skip fonts that don't support the text we are testing.

http://cr.openjdk.java.net/~prr/8244113.1

-phil

On 4/29/20, 11:56 AM, Philip Race wrote:

bug : https://bugs.openjdk.java.net/browse/JDK-8244113
webrev : http://cr.openjdk.java.net/~prr/8244113/

Apparently jtreg consumes text even from 2 lines later after an @run tag
as being args to pass to the program.

Test still passes everywhere after the change.

-phil.

Re: [OpenJDK 2D-Dev] RFR: 8221305: java/awt/FontMetrics/MaxAdvanceIsMax.java fails on MacOS + Solaris

[Philip Race]

This test is called MaxAdvance *is max*. My emphasis.
Not "all values returned by getWidths() are less than or equal to max 
advance.
The latter is simply the inadequate implementation to partially test the 
assertion

and which was already proving to be too much. The only way to make that test
pass is to make changes to what max advance reports and I am saying that
we should not do that. It would be artificial and wrong.

-phil.



On 4/30/20, 9:23 AM, Sergey Bylokhov wrote:

On 4/29/20 3:58 pm, Philip Race wrote:
the advance of 'm' is a commonly used proxy for the design width of a 
latin font.
But I agree it is also not really a great estimate once you consider 
any international text.


Anyway I am not sure where you are headed with that and the
discussion of charWidth() or charWidths().

I am not trying to "fix the world" here, I am just removing a test
that it is pointless to maintain.


Then probably we can report a bug, to state that we have some issues 
here?

From my point of view, the test is mostly fine, it does not check some
real corner cases and compound glyphs but only the simple chars, which 
should
work. If it does not work means all code where we try to predict the 
size of
the text components is broken, and it looks like there is no way to 
fix that.

Re: [OpenJDK 2D-Dev] RFR: 8221305: java/awt/FontMetrics/MaxAdvanceIsMax.java fails on MacOS + Solaris

[Sergey Bylokhov]

On 4/29/20 3:58 pm, Philip Race wrote:

the advance of 'm' is a commonly used proxy for the design width of a latin 
font.
But I agree it is also not really a great estimate once you consider any 
international text.

Anyway I am not sure where you are headed with that and the
discussion of charWidth() or charWidths().

I am not trying to "fix the world" here, I am just removing a test
that it is pointless to maintain.


Then probably we can report a bug, to state that we have some issues here?
From my point of view, the test is mostly fine, it does not check some
real corner cases and compound glyphs but only the simple chars, which should
work. If it does not work means all code where we try to predict the size of
the text components is broken, and it looks like there is no way to fix that.


--
Best regards, Sergey.

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 
 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 
> 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 
> 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  and 
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 
> 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/ 



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

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

[Jayathirth D v]

Hi Prasanta,

Thats why I pointed to 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 
>  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 
>>> 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 
  and 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 
> 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/ 
>>>  
>>> 
>>> Issue : PNG specification restricts length of string

Re: [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 
> 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 
 and 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 
> 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/ 



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

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

[Jayathirth D v]

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 
>  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 
>>  and 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 
>>> 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/ 
>  
> 
> 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
>> 

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

[Prasanta Sadhukhan]

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 
 and 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 
> 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/ 



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

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

[Jayathirth D v]

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  
and 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 
>  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/ 
>>>  
>>> 
>>> 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