Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458
+1 Thanks, Krishna -Original Message- From: Jayathirth D V Sent: Tuesday, November 20, 2018 1:59 PM To: 2d-dev <2d-dev@openjdk.java.net> Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Thanks Sergey for the review. I need one more review. Please review the latest webrev: http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ Thanks, Jay -Original Message- From: Sergey Bylokhov Sent: Saturday, November 17, 2018 3:35 AM To: Jayathirth D V; 2d-dev Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Looks fine. On 16/11/2018 02:16, Jayathirth D V wrote: > Hi Sergey, > > As discussed offline I did more analysis on whether we can use common > variable to determine number of bands. Since we have "outputSampleSize.length > - 1" and "inputBands + 1" kind of things. > > Actually scale array will be used on input data(ps[]), so we should use input > bands value to create and use scale array. Before JDK-6788458 there was no > difference between input bands and output bands so we didn't see any issue. > But after JDK-6788458 number of bands data is different between input and > output data for PNG_COLOR_RGB/GRAY having tRNS chunk. > > So I have made change to use inputBands data for creation and use of scale > array. Ran complete imageio jtreg and JCK tests there are no failures. > Please find updated webrev for review: > http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ > > Thanks, > Jay > > -Original Message- > From: Jayathirth D V > Sent: Wednesday, November 14, 2018 1:09 PM > To: Sergey Bylokhov; 2d-dev > Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: > ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 > > Hi Sergey, > > Thanks for the review. > > As you pointed out, yes the AIOOB is happening for ps[b]. I have updated the > analysis in JBS bug also. > > Basically the calculation of numBands is not proper because we take numBands > value from destination image. This destination image will have extra alpha > channel for Gray or RGB input data(ps[]) which will throw AIOOB. > > So we need to update the logic of how we calculate numBands different for PNG > Gray/RGB image havng tRNS chunk. Fortunately, webrev.00 is actually doing > this job. > > Regarding whether we need to change scale array logic : We expect first 3 > channel to be RGB and first channel to be Gray for PNG_COLOR_RGB and > PNG_COLOR_GRAY respectively. So just updating numBands information will > create proper scale array. So there is no need to change the scale array > logic. > > History JDK-6788458 : Toolkit was able to show transparent color information > for RGB/Gray PNG image when it has tRNS chunk, but ImageIO didn't support it. > To use tRNS data and show transparent color in output image we needed to add > extra alpha channel for PNG RGB/Gray image with tRNS chunk. But fix present > in JDK-6788458 didn't handle the case where bitDepth adjustment is needed and > we are using band information from output image(having extra alpha channel) > on input image which has no alpha channel. Change in numBands logic for this > bug fixes that issue. > > > Thanks, > Jay > > -Original Message----- > From: Sergey Bylokhov > Sent: Wednesday, November 14, 2018 4:07 AM > To: Jayathirth D V; 2d-dev > Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: > ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 > > Hi, Jay. > > Can you please provide some more detail about this bug. > >> Root cause : In JDK-6788458 we are adding extra alpha channel for >> destination whenever we have tRNS chunk. But the number of bands in bitDepth >> scale array was not changed when we have tRNS chunk. This is causing >> ArrayIndexOutOfBoundsException for scale array. > > As far as I understand the AIOOB is occurred when we access ps[b] at line > 1308 not when we access the scale array, because the scale array is created > as "scale = new int[numBands][]". So maybe numBands should depends on the > passRow? or the creation of scale[][xxx] should be updated? > BTW this code uses +1/-1 in a lot of places already, and it is not always > clear why. > > -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458
Thanks Sergey for the review. I need one more review. Please review the latest webrev: http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ Thanks, Jay -Original Message- From: Sergey Bylokhov Sent: Saturday, November 17, 2018 3:35 AM To: Jayathirth D V; 2d-dev Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Looks fine. On 16/11/2018 02:16, Jayathirth D V wrote: > Hi Sergey, > > As discussed offline I did more analysis on whether we can use common > variable to determine number of bands. Since we have "outputSampleSize.length > - 1" and "inputBands + 1" kind of things. > > Actually scale array will be used on input data(ps[]), so we should use input > bands value to create and use scale array. Before JDK-6788458 there was no > difference between input bands and output bands so we didn't see any issue. > But after JDK-6788458 number of bands data is different between input and > output data for PNG_COLOR_RGB/GRAY having tRNS chunk. > > So I have made change to use inputBands data for creation and use of scale > array. Ran complete imageio jtreg and JCK tests there are no failures. > Please find updated webrev for review: > http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ > > Thanks, > Jay > > -Original Message- > From: Jayathirth D V > Sent: Wednesday, November 14, 2018 1:09 PM > To: Sergey Bylokhov; 2d-dev > Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: > ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 > > Hi Sergey, > > Thanks for the review. > > As you pointed out, yes the AIOOB is happening for ps[b]. I have updated the > analysis in JBS bug also. > > Basically the calculation of numBands is not proper because we take numBands > value from destination image. This destination image will have extra alpha > channel for Gray or RGB input data(ps[]) which will throw AIOOB. > > So we need to update the logic of how we calculate numBands different for PNG > Gray/RGB image havng tRNS chunk. Fortunately, webrev.00 is actually doing > this job. > > Regarding whether we need to change scale array logic : We expect first 3 > channel to be RGB and first channel to be Gray for PNG_COLOR_RGB and > PNG_COLOR_GRAY respectively. So just updating numBands information will > create proper scale array. So there is no need to change the scale array > logic. > > History JDK-6788458 : Toolkit was able to show transparent color information > for RGB/Gray PNG image when it has tRNS chunk, but ImageIO didn't support it. > To use tRNS data and show transparent color in output image we needed to add > extra alpha channel for PNG RGB/Gray image with tRNS chunk. But fix present > in JDK-6788458 didn't handle the case where bitDepth adjustment is needed and > we are using band information from output image(having extra alpha channel) > on input image which has no alpha channel. Change in numBands logic for this > bug fixes that issue. > > > Thanks, > Jay > > -Original Message----- > From: Sergey Bylokhov > Sent: Wednesday, November 14, 2018 4:07 AM > To: Jayathirth D V; 2d-dev > Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: > ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 > > Hi, Jay. > > Can you please provide some more detail about this bug. > >> Root cause : In JDK-6788458 we are adding extra alpha channel for >> destination whenever we have tRNS chunk. But the number of bands in bitDepth >> scale array was not changed when we have tRNS chunk. This is causing >> ArrayIndexOutOfBoundsException for scale array. > > As far as I understand the AIOOB is occurred when we access ps[b] at line > 1308 not when we access the scale array, because the scale array is created > as "scale = new int[numBands][]". So maybe numBands should depends on the > passRow? or the creation of scale[][xxx] should be updated? > BTW this code uses +1/-1 in a lot of places already, and it is not always > clear why. > > -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458
Looks fine. On 16/11/2018 02:16, Jayathirth D V wrote: Hi Sergey, As discussed offline I did more analysis on whether we can use common variable to determine number of bands. Since we have "outputSampleSize.length - 1" and "inputBands + 1" kind of things. Actually scale array will be used on input data(ps[]), so we should use input bands value to create and use scale array. Before JDK-6788458 there was no difference between input bands and output bands so we didn't see any issue. But after JDK-6788458 number of bands data is different between input and output data for PNG_COLOR_RGB/GRAY having tRNS chunk. So I have made change to use inputBands data for creation and use of scale array. Ran complete imageio jtreg and JCK tests there are no failures. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ Thanks, Jay -Original Message- From: Jayathirth D V Sent: Wednesday, November 14, 2018 1:09 PM To: Sergey Bylokhov; 2d-dev Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Hi Sergey, Thanks for the review. As you pointed out, yes the AIOOB is happening for ps[b]. I have updated the analysis in JBS bug also. Basically the calculation of numBands is not proper because we take numBands value from destination image. This destination image will have extra alpha channel for Gray or RGB input data(ps[]) which will throw AIOOB. So we need to update the logic of how we calculate numBands different for PNG Gray/RGB image havng tRNS chunk. Fortunately, webrev.00 is actually doing this job. Regarding whether we need to change scale array logic : We expect first 3 channel to be RGB and first channel to be Gray for PNG_COLOR_RGB and PNG_COLOR_GRAY respectively. So just updating numBands information will create proper scale array. So there is no need to change the scale array logic. History JDK-6788458 : Toolkit was able to show transparent color information for RGB/Gray PNG image when it has tRNS chunk, but ImageIO didn't support it. To use tRNS data and show transparent color in output image we needed to add extra alpha channel for PNG RGB/Gray image with tRNS chunk. But fix present in JDK-6788458 didn't handle the case where bitDepth adjustment is needed and we are using band information from output image(having extra alpha channel) on input image which has no alpha channel. Change in numBands logic for this bug fixes that issue. Thanks, Jay -Original Message- From: Sergey Bylokhov Sent: Wednesday, November 14, 2018 4:07 AM To: Jayathirth D V; 2d-dev Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Hi, Jay. Can you please provide some more detail about this bug. Root cause : In JDK-6788458 we are adding extra alpha channel for destination whenever we have tRNS chunk. But the number of bands in bitDepth scale array was not changed when we have tRNS chunk. This is causing ArrayIndexOutOfBoundsException for scale array. As far as I understand the AIOOB is occurred when we access ps[b] at line 1308 not when we access the scale array, because the scale array is created as "scale = new int[numBands][]". So maybe numBands should depends on the passRow? or the creation of scale[][xxx] should be updated? BTW this code uses +1/-1 in a lot of places already, and it is not always clear why. -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458
Hi Sergey, As discussed offline I did more analysis on whether we can use common variable to determine number of bands. Since we have "outputSampleSize.length - 1" and "inputBands + 1" kind of things. Actually scale array will be used on input data(ps[]), so we should use input bands value to create and use scale array. Before JDK-6788458 there was no difference between input bands and output bands so we didn't see any issue. But after JDK-6788458 number of bands data is different between input and output data for PNG_COLOR_RGB/GRAY having tRNS chunk. So I have made change to use inputBands data for creation and use of scale array. Ran complete imageio jtreg and JCK tests there are no failures. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ Thanks, Jay -Original Message- From: Jayathirth D V Sent: Wednesday, November 14, 2018 1:09 PM To: Sergey Bylokhov; 2d-dev Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Hi Sergey, Thanks for the review. As you pointed out, yes the AIOOB is happening for ps[b]. I have updated the analysis in JBS bug also. Basically the calculation of numBands is not proper because we take numBands value from destination image. This destination image will have extra alpha channel for Gray or RGB input data(ps[]) which will throw AIOOB. So we need to update the logic of how we calculate numBands different for PNG Gray/RGB image havng tRNS chunk. Fortunately, webrev.00 is actually doing this job. Regarding whether we need to change scale array logic : We expect first 3 channel to be RGB and first channel to be Gray for PNG_COLOR_RGB and PNG_COLOR_GRAY respectively. So just updating numBands information will create proper scale array. So there is no need to change the scale array logic. History JDK-6788458 : Toolkit was able to show transparent color information for RGB/Gray PNG image when it has tRNS chunk, but ImageIO didn't support it. To use tRNS data and show transparent color in output image we needed to add extra alpha channel for PNG RGB/Gray image with tRNS chunk. But fix present in JDK-6788458 didn't handle the case where bitDepth adjustment is needed and we are using band information from output image(having extra alpha channel) on input image which has no alpha channel. Change in numBands logic for this bug fixes that issue. Thanks, Jay -Original Message- From: Sergey Bylokhov Sent: Wednesday, November 14, 2018 4:07 AM To: Jayathirth D V; 2d-dev Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Hi, Jay. Can you please provide some more detail about this bug. > Root cause : In JDK-6788458 we are adding extra alpha channel for destination > whenever we have tRNS chunk. But the number of bands in bitDepth scale array > was not changed when we have tRNS chunk. This is causing > ArrayIndexOutOfBoundsException for scale array. As far as I understand the AIOOB is occurred when we access ps[b] at line 1308 not when we access the scale array, because the scale array is created as "scale = new int[numBands][]". So maybe numBands should depends on the passRow? or the creation of scale[][xxx] should be updated? BTW this code uses +1/-1 in a lot of places already, and it is not always clear why. -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458
Hi, Jay. Can you please provide some more detail about this bug. Root cause : In JDK-6788458 we are adding extra alpha channel for destination whenever we have tRNS chunk. But the number of bands in bitDepth scale array was not changed when we have tRNS chunk. This is causing ArrayIndexOutOfBoundsException for scale array. As far as I understand the AIOOB is occurred when we access ps[b] at line 1308 not when we access the scale array, because the scale array is created as "scale = new int[numBands][]". So maybe numBands should depends on the passRow? or the creation of scale[][xxx] should be updated? BTW this code uses +1/-1 in a lot of places already, and it is not always clear why. -- Best regards, Sergey.
[OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458
Hello All, Please review the following fix in JDK12: Bug : https://bugs.openjdk.java.net/browse/JDK-8211795 Webrev: http://cr.openjdk.java.net/~jdv/8211795/webrev.00/ Issue : When we read PNG image having tRNS chunk and it needs bitDepth adjustment then we throw ArrayIndexOutOfBoundsException Root cause : In JDK-6788458 we are adding extra alpha channel for destination whenever we have tRNS chunk. But the number of bands in bitDepth scale array was not changed when we have tRNS chunk. This is causing ArrayIndexOutOfBoundsException for scale array. Solution : Whenever we have tRNS chunk use extra alpha band for bitDepth scale array. Thanks, Jay
