Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt CRC for IEND chunk throws IIOException
Thanks Sergey for the approval.
Need one more review.
Please review the webrev :
http://cr.openjdk.java.net/~jdv/8211422/webrev.01/
Regards,
Jay
-Original Message-
From: Sergey Bylokhov
Sent: Thursday, November 15, 2018 10:58 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt
CRC for IEND chunk throws IIOException
Looks fine.
On 14/11/2018 04:35, Jayathirth D V wrote:
> Hi Sergey,
>
> Thanks for the review.
>
> I have updated the code to not move the changes out of switch. Apart from
> that I have refined comments to make it clear why we are not reading CRC for
> IEND chunk. Also chunkCRC value needs to be initialized because of this
> update, initialized value of chunkCRC will not be used anywhere in the logic.
>
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8211422/webrev.01/
>
> Thanks,
> Jay
>
> -Original Message-
> From: Sergey Bylokhov
> Sent: Wednesday, November 14, 2018 4:31 AM
> To: Jayathirth D V; 2d-dev@openjdk.java.net
> Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with
> corrupt CRC for IEND chunk throws IIOException
>
> Hi, Jay.
>
> Probably it will be better to not to change the code inside switch, and only
> add the check below around of reading CRC:
> "if (chunkType != IEND_TYPE) {"
>
> The current fix may throw "Invalid chunk length" when seek/flush the data for
> IEND_TYPE, which is not correct.
>
>
> On 12/11/2018 20:22, Jayathirth D V wrote:
>> Hello All,
>>
>> Gentle reminder for review.
>>
>> Thanks,
>>
>> Jay
>>
>> *From:*Jayathirth Rao
>> *Sent:* Tuesday, October 23, 2018 7:09 PM
>> *To:* 2d-dev@openjdk.java.net
>> *Subject:* [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with
>> corrupt CRC for IEND chunk throws IIOException
>>
>> Hello All,
>>
>> Please review the following fix in JDK12:
>>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8211422
>>
>> Webrev: http://cr.openjdk.java.net/~jdv/8211422/webrev.00/
>>
>> Issue : When we try to read PNG image with corrupt/no 4 byte CRC data for
>> IEND chunk we throw IIOException. We see this issue only after JDK-8164971
>> .
>>
>> Root cause : In JDK-8164971
>> fix we made changes to
>> continue reading metadata until we reach IEND chunk. Before JDK-8164971
>> change we used to stop
>> reading metadata as soon as we hit first IDAT chunk. According to PNG spec
>> there can be more than one IDAT chunk/ more headers after IDAT chunk. So we
>> need to read metadata until we hit IEND chunk. But in case of this bug we
>> have IEND chunk but it has corrupt/no CRC chunk, so we throw
>> IIOException(According to PNG spec every PNG chunk should contain 4 byte
>> CRC). But lot of other decoders accept these kind of images which has no CRC
>> chunk for IEND chunk.
>>
>> Solution : There is no need to verify CRC for IEND chunk(Chunk data length
>> for IEND is 0). Stop reading metadata once we hit Chunk type info for IEND
>> chunk.
>>
>> Thanks,
>>
>> Jay
>>
>
>
> --
> Best regards, Sergey.
>
--
Best regards, Sergey.
[OpenJDK 2D-Dev] RFR: 8130264 : change the mechanism by which JDK loads the platform-specific PrinterJob implementation
bug: http://cr.openjdk.java.net/~prr/8130264/ webrev: http://cr.openjdk.java.net/~prr/8130264/ Currently java launcher code embeds the name of the java.desktop module's PrinterJob implementation class for each platform in a system property which is later read by the java.desktop code to use to reflectively locate the class and instantiate it. This fix removes that entirely from the launcher code and the desktop module now looks it up internally via a simple platform proxy class. This builds on all platforms and we rely on existing printing tests to verify that we can still locate the implementation class. The new regression test just verifies the system property name space is no longer polluted. I didn't find any test (apart from this new one) that references it. Since that system property has been around for a long time I am thinking I should file a CSR to document its removal .. unless there is a concensus it is not necessary. -phil.
Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt CRC for IEND chunk throws IIOException
Looks fine.
On 14/11/2018 04:35, Jayathirth D V wrote:
Hi Sergey,
Thanks for the review.
I have updated the code to not move the changes out of switch. Apart from that
I have refined comments to make it clear why we are not reading CRC for IEND
chunk. Also chunkCRC value needs to be initialized because of this update,
initialized value of chunkCRC will not be used anywhere in the logic.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8211422/webrev.01/
Thanks,
Jay
-Original Message-
From: Sergey Bylokhov
Sent: Wednesday, November 14, 2018 4:31 AM
To: Jayathirth D V; 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt
CRC for IEND chunk throws IIOException
Hi, Jay.
Probably it will be better to not to change the code inside switch, and only
add the check below around of reading CRC:
"if (chunkType != IEND_TYPE) {"
The current fix may throw "Invalid chunk length" when seek/flush the data for
IEND_TYPE, which is not correct.
On 12/11/2018 20:22, Jayathirth D V wrote:
Hello All,
Gentle reminder for review.
Thanks,
Jay
*From:*Jayathirth Rao
*Sent:* Tuesday, October 23, 2018 7:09 PM
*To:* 2d-dev@openjdk.java.net
*Subject:* [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with
corrupt CRC for IEND chunk throws IIOException
Hello All,
Please review the following fix in JDK12:
Bug : https://bugs.openjdk.java.net/browse/JDK-8211422
Webrev: http://cr.openjdk.java.net/~jdv/8211422/webrev.00/
Issue : When we try to read PNG image with corrupt/no 4 byte CRC data for IEND chunk
we throw IIOException. We see this issue only after JDK-8164971
.
Root cause : In JDK-8164971 fix we
made changes to continue reading metadata until we reach IEND chunk. Before JDK-8164971
change we used to stop reading
metadata as soon as we hit first IDAT chunk. According to PNG spec there can be more than
one IDAT chunk/ more headers after IDAT chunk. So we need to read metadata until we hit
IEND chunk. But in case of this bug we have IEND chunk but it has corrupt/no CRC chunk, so
we throw IIOException(According to PNG spec every PNG chunk should contain 4 byte CRC). But
lot of other decoders accept these kind of images which has no CRC chunk for IEND chunk.
Solution : There is no need to verify CRC for IEND chunk(Chunk data length for
IEND is 0). Stop reading metadata once we hit Chunk type info for IEND chunk.
Thanks,
Jay
--
Best regards, Sergey.
--
Best regards, Sergey.
Re: [OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
Looks ok to me. Thanks for handling the configure changes! /Erik On 2018-11-15 08:06, Volker Simonis wrote: Hi, can I please have a review for the following small change: http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/ https://bugs.openjdk.java.net/browse/JDK-8213944 Change JDK-8210863 removed the Xrandr.h/randr.h headers from the OpenJDK sources but forgot to add a configure check for the Xrandr extension which is now a build dependency. The change also broke the AIX build. AIX never supported Xrandr, but that was only detected at runtime, when the JDK was unable to dynamically load libXrand.so. Now, without Xrandr.h/randr.h in the source tree any more, we have to conditionally compile some parts of src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c such that it doesn't require the definitions and declarations from Xrandr.h/randr.h any more. Thank you and best regards, Volker
Re: [OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
PS I am not sure why xrandr headers would not be available for AIX. They are a standard part of the xdistribution. If true, think what you are going to have to do is add a --with-xrandr-include option and provide it that way. -phil. On 11/15/18, 8:55 AM, Philip Race wrote: Hmm. I don't like the ifdefs. Xrandr is a requirement for the build. If its not there at runtime that's OK. -phil. On 11/15/18, 8:06 AM, Volker Simonis wrote: Hi, can I please have a review for the following small change: http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/ https://bugs.openjdk.java.net/browse/JDK-8213944 Change JDK-8210863 removed the Xrandr.h/randr.h headers from the OpenJDK sources but forgot to add a configure check for the Xrandr extension which is now a build dependency. The change also broke the AIX build. AIX never supported Xrandr, but that was only detected at runtime, when the JDK was unable to dynamically load libXrand.so. Now, without Xrandr.h/randr.h in the source tree any more, we have to conditionally compile some parts of src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c such that it doesn't require the definitions and declarations from Xrandr.h/randr.h any more. Thank you and best regards, Volker
Re: [OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
Hmm. I don't like the ifdefs. Xrandr is a requirement for the build. If its not there at runtime that's OK. -phil. On 11/15/18, 8:06 AM, Volker Simonis wrote: Hi, can I please have a review for the following small change: http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/ https://bugs.openjdk.java.net/browse/JDK-8213944 Change JDK-8210863 removed the Xrandr.h/randr.h headers from the OpenJDK sources but forgot to add a configure check for the Xrandr extension which is now a build dependency. The change also broke the AIX build. AIX never supported Xrandr, but that was only detected at runtime, when the JDK was unable to dynamically load libXrand.so. Now, without Xrandr.h/randr.h in the source tree any more, we have to conditionally compile some parts of src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c such that it doesn't require the definitions and declarations from Xrandr.h/randr.h any more. Thank you and best regards, Volker
Re: [OpenJDK 2D-Dev] RFR: 8210863: Remove Xrandr include files from JDK sources
Sorry, I'm a little late to the game, but this change broke the AIX build. It also misses a configure check for Xrandr which now became a build dependency. I've opened "8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it" and sent around a RFR on this [1] list for the trivial fix. Would be nice if you could have a look at it and review it. Thank you and best regards, Volker [1] http://mail.openjdk.java.net/pipermail/2d-dev/2018-November/009636.html On Wed, Oct 31, 2018 at 8:18 PM Phil Race wrote: > > Erik has an updated devkit that is a pre-requisite for that. It is already > pushed and I tested with his patch which will make the new devkit the > default. > > So mach5 is green once Erik pushes his devkit fix which I have tested > to make sure we always have the header files on otherwise "minimally" > configured linux build systems. > > Ergo, this will go in only once he has pushed his fix : > https://bugs.openjdk.java.net/browse/JDK-8210837 > > -phil. > > On 10/31/18 12:02 PM, Sergey Bylokhov wrote: > > Looks fine, I assume mach5 is green. > > > > On 31/10/2018 11:57, Phil Race wrote: > >> Bug: https://bugs.openjdk.java.net/browse/JDK-8210863 > >> Webrev: http://cr.openjdk.java.net/~prr/8210863/ > >> > >> JDK imported Xrandr headers into it's source a really long time ago > >> because > >> old build systems did not have it. We no longer have that problem, so > >> we can > >> remove these. > >> > >> I am not changing the code to remove the dynamic loading of the > >> functions and > >> change it to compile time link against them. > >> We can probably do that too at a later time, but the immediate goal > >> is to > >> remove these source files since they are imported 3rd party sources. > >> > >> -phil. > >> > >> > > > > >
[OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
Hi, can I please have a review for the following small change: http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/ https://bugs.openjdk.java.net/browse/JDK-8213944 Change JDK-8210863 removed the Xrandr.h/randr.h headers from the OpenJDK sources but forgot to add a configure check for the Xrandr extension which is now a build dependency. The change also broke the AIX build. AIX never supported Xrandr, but that was only detected at runtime, when the JDK was unable to dynamically load libXrand.so. Now, without Xrandr.h/randr.h in the source tree any more, we have to conditionally compile some parts of src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c such that it doesn't require the definitions and declarations from Xrandr.h/randr.h any more. Thank you and best regards, Volker
