Re: [OpenJDK 2D-Dev] [15] RFR JDK-6532025: GIF reader throws misleading exception with truncated images

[Brian Burkhalter]

+1

> On Mar 6, 2020, at 8:01 AM, Phil Race  wrote:
> 
> +1
> 
> -Phil.
> 
> On 3/5/20 7:47 PM, Jayathirth D v wrote:
>> Hi Phil,
>> 
>> Thanks for your inputs.
>> 
>> Yes we don’t need extra “left > 0” check, I have removed it and I have 
>> updated the test case to print “Test passed”.
>> 
>> Please find updated webrev for review:
>> http://cr.openjdk.java.net/~jdv/6532025/webrev.01/ 
>>  
>> 
>> I ran all imageio jtreg tests and there are no failures with updated change.
>> 
>> Thanks,
>> Jay
>> 
>>> On 06-Mar-2020, at 8:22 AM, Philip Race >> > wrote:
>>> 
>>> I don't understand why you need to recheck that left > 0.
>>> Nothing can change it between the while loop check and your check
>>> 
>>>  while (left > 0) {
>>>  int nbytes = stream.read(block, off, left);
>>> +if (nbytes == -1 && left > 0) {
>>> +throw new IIOException("Invalid block length 
>>> for " +
>>> +"LZW encoded image data");
>>> +}
>>>  off += nbytes;
>>>  left -= nbytes;
>>>  }
>>> 
>>> Also in the test since you are printing
>>>   51 } catch (IIOException e) {
>>>   52 // do nothing we expect IIOException but we should not
>>>   53 // throw IndexOutOfBoundsException
>>>   54 System.out.println(e.toString());
>>>   55 System.out.println("Caught IIOException ignore it");
>>> 
>>> Maybe add "Test passed" to be extra clear !
>>> 
>>> -phil.
>>> 
>>> On 3/5/20, 6:15 PM, Jayathirth D v wrote:
 
 Hi Brian,
 
 Thanks for the clarification and approval.
 
 @Others : Need one more review please take a look.
 
 Regards,
 Jay 
 
> On 06-Mar-2020, at 2:05 AM, Brian Burkhalter  > wrote:
> 
> Hi Jay,
> 
> I think the overall change is fine.
> 
> Regards,
> 
> Brian
> 
>> On Mar 5, 2020, at 9:18 AM, Jayathirth D v > > wrote:
>> 
>> Hello Brian,
>> 
>> Thanks for the review. GIF stream in regression test case would match 
>> warn.gif stream behaviour that’s why it going into 
>> GIFImageReader.getCode().
>> 
>> Are you okay with overall webrev.00 patch or have you just approved 
>> GIFImageReader change? Please clarify.
>> 
>> Regards,
>> Jay
>> 
>>> On 05-Mar-2020, at 10:20 PM, Brian Burkhalter 
>>> mailto:brian.burkhal...@oracle.com>> 
>>> wrote:
>>> 
>>> Hello Jay,
>>> 
>>> The source fix looks OK to me. I get the same exception as in the bug 
>>> description when I run the test against my unpatched local JDK 15 build.
>>> 
>>> Thanks,
>>> 
>>> Brian
>>> 
 On Mar 5, 2020, at 2:12 AM, Jayathirth D v >>> > wrote:
 
 Please review the following fix for JDK 15:
 
 Bug : https://bugs.openjdk.java.net/browse/JDK-6532025 
 
 Webrev : http://cr.openjdk.java.net/~jdv/6532025/webrev.00/ 
  
 
 Root cause : When we have truncated GIF images, stream.read() returns 
 -1 but GIFImageReader doesn’t handle such conditions properly and 
 continues to read input stream data.
 Solution : Handle cases where we reach EOF and throw appropriate 
 exception.
>>> 
>> 
> 
 
>> 
> 

Re: [OpenJDK 2D-Dev] [15] RFR JDK-6532025: GIF reader throws misleading exception with truncated images

[Phil Race]

+1

-Phil.

On 3/5/20 7:47 PM, Jayathirth D v wrote:

Hi Phil,

Thanks for your inputs.

Yes we don’t need extra “left > 0” check, I have removed it and I have 
updated the test case to print “Test passed”.


Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6532025/webrev.01/

I ran all imageio jtreg tests and there are no failures with updated 
change.


Thanks,
Jay

On 06-Mar-2020, at 8:22 AM, Philip Race > wrote:


I don't understand why you need to recheck that left > 0.
Nothing can change it between the while loop check and your check

 while (left > 0) {
 int nbytes = stream.read(block, off, left);
+    if (nbytes == -1 && left > 0) {
+    throw new IIOException("Invalid block 
length for " +

+    "LZW encoded image data");
+    }
 off += nbytes;
 left -= nbytes;
 }

Also in the test since you are printing
   51 } catch (IIOException e) {
   52 // do nothing we expect IIOException but we should not
   53 // throw IndexOutOfBoundsException
   54 System.out.println(e.toString());
   55 System.out.println("Caught IIOException ignore it");

Maybe add "Test passed" to be extra clear !

-phil.

On 3/5/20, 6:15 PM, Jayathirth D v wrote:

Hi Brian,

Thanks for the clarification and approval.

@Others : Need one more review please take a look.

Regards,
Jay

On 06-Mar-2020, at 2:05 AM, Brian Burkhalter 
mailto:brian.burkhal...@oracle.com>> 
wrote:


Hi Jay,

I think the overall change is fine.

Regards,

Brian

On Mar 5, 2020, at 9:18 AM, Jayathirth D v 
mailto:jayathirth@oracle.com>> wrote:


Hello Brian,

Thanks for the review. GIF stream in regression test case would 
match warn.gif stream behaviour that’s why it going into 
GIFImageReader.getCode().


Are you okay with overall webrev.00 patch or have you just 
approved GIFImageReader change? Please clarify.


Regards,
Jay

On 05-Mar-2020, at 10:20 PM, Brian Burkhalter 
> wrote:


Hello Jay,

The source fix looks OK to me. I get the same exception as in the 
bug description when I run the test against my unpatched local 
JDK 15 build.


Thanks,

Brian

On Mar 5, 2020, at 2:12 AM, Jayathirth D v 
mailto:jayathirth@oracle.com>> 
wrote:


Please review the following fix for JDK 15:

Bug : https://bugs.openjdk.java.net/browse/JDK-6532025
Webrev : http://cr.openjdk.java.net/~jdv/6532025/webrev.00/ 



Root cause : When we have truncated GIF images, stream.read() 
returns -1 but GIFImageReader doesn’t handle such conditions 
properly and continues to read input stream data.
Solution : Handle cases where we reach EOF and throw appropriate 
exception.

Re: [OpenJDK 2D-Dev] [15] RFR JDK-6532025: GIF reader throws misleading exception with truncated images

[Jayathirth D v]

Hi Phil,

Thanks for your inputs.

Yes we don’t need extra “left > 0” check, I have removed it and I have updated 
the test case to print “Test passed”.

Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6532025/webrev.01/ 
 

I ran all imageio jtreg tests and there are no failures with updated change.

Thanks,
Jay

> On 06-Mar-2020, at 8:22 AM, Philip Race  wrote:
> 
> I don't understand why you need to recheck that left > 0.
> Nothing can change it between the while loop check and your check
> 
>  while (left > 0) {
>  int nbytes = stream.read(block, off, left);
> +if (nbytes == -1 && left > 0) {
> +throw new IIOException("Invalid block length for 
> " +
> +"LZW encoded image data");
> +}
>  off += nbytes;
>  left -= nbytes;
>  }
> 
> Also in the test since you are printing
>   51 } catch (IIOException e) {
>   52 // do nothing we expect IIOException but we should not
>   53 // throw IndexOutOfBoundsException
>   54 System.out.println(e.toString());
>   55 System.out.println("Caught IIOException ignore it");
> 
> Maybe add "Test passed" to be extra clear !
> 
> -phil.
> 
> On 3/5/20, 6:15 PM, Jayathirth D v wrote:
>> 
>> Hi Brian,
>> 
>> Thanks for the clarification and approval.
>> 
>> @Others : Need one more review please take a look.
>> 
>> Regards,
>> Jay 
>> 
>>> On 06-Mar-2020, at 2:05 AM, Brian Burkhalter >> > wrote:
>>> 
>>> Hi Jay,
>>> 
>>> I think the overall change is fine.
>>> 
>>> Regards,
>>> 
>>> Brian
>>> 
 On Mar 5, 2020, at 9:18 AM, Jayathirth D v >>> > wrote:
 
 Hello Brian,
 
 Thanks for the review. GIF stream in regression test case would match 
 warn.gif stream behaviour that’s why it going into 
 GIFImageReader.getCode().
 
 Are you okay with overall webrev.00 patch or have you just approved 
 GIFImageReader change? Please clarify.
 
 Regards,
 Jay
 
> On 05-Mar-2020, at 10:20 PM, Brian Burkhalter 
> mailto:brian.burkhal...@oracle.com>> wrote:
> 
> Hello Jay,
> 
> The source fix looks OK to me. I get the same exception as in the bug 
> description when I run the test against my unpatched local JDK 15 build.
> 
> Thanks,
> 
> Brian
> 
>> On Mar 5, 2020, at 2:12 AM, Jayathirth D v > > wrote:
>> 
>> Please review the following fix for JDK 15:
>> 
>> Bug : https://bugs.openjdk.java.net/browse/JDK-6532025 
>> 
>> Webrev : http://cr.openjdk.java.net/~jdv/6532025/webrev.00/ 
>>  
>> 
>> Root cause : When we have truncated GIF images, stream.read() returns -1 
>> but GIFImageReader doesn’t handle such conditions properly and continues 
>> to read input stream data.
>> Solution : Handle cases where we reach EOF and throw appropriate 
>> exception.
> 
 
>>> 
>> 

Re: [OpenJDK 2D-Dev] [15] RFR JDK-6532025: GIF reader throws misleading exception with truncated images

[Philip Race]

I don't understand why you need to recheck that left > 0.
Nothing can change it between the while loop check and your check

 while (left > 0) {
 int nbytes = stream.read(block, off, left);
+if (nbytes == -1 && left > 0) {
+throw new IIOException("Invalid block 
length for " +

+"LZW encoded image data");
+}
 off += nbytes;
 left -= nbytes;
 }

Also in the test since you are printing

  51 } catch (IIOException e) {
  52 // do nothing we expect IIOException but we should not
  53 // throw IndexOutOfBoundsException
  54 System.out.println(e.toString());
  55 System.out.println("Caught IIOException ignore it");


Maybe add "Test passed" to be extra clear !

-phil.

On 3/5/20, 6:15 PM, Jayathirth D v wrote:

Hi Brian,

Thanks for the clarification and approval.

@Others : Need one more review please take a look.

Regards,
Jay

On 06-Mar-2020, at 2:05 AM, Brian Burkhalter 
mailto:brian.burkhal...@oracle.com>> wrote:


Hi Jay,

I think the overall change is fine.

Regards,

Brian

On Mar 5, 2020, at 9:18 AM, Jayathirth D v 
mailto:jayathirth@oracle.com>> wrote:


Hello Brian,

Thanks for the review. GIF stream in regression test case would 
match warn.gif stream behaviour that’s why it going into 
GIFImageReader.getCode().


Are you okay with overall webrev.00 patch or have you just approved 
GIFImageReader change? Please clarify.


Regards,
Jay

On 05-Mar-2020, at 10:20 PM, Brian Burkhalter 
mailto:brian.burkhal...@oracle.com>> 
wrote:


Hello Jay,

The source fix looks OK to me. I get the same exception as in the 
bug description when I run the test against my unpatched local JDK 
15 build.


Thanks,

Brian

On Mar 5, 2020, at 2:12 AM, Jayathirth D v 
mailto:jayathirth@oracle.com>> wrote:


Please review the following fix for JDK 15:

Bug : https://bugs.openjdk.java.net/browse/JDK-6532025
Webrev : http://cr.openjdk.java.net/~jdv/6532025/webrev.00/ 



Root cause : When we have truncated GIF images, stream.read() 
returns -1 but GIFImageReader doesn’t handle such conditions 
properly and continues to read input stream data.
Solution : Handle cases where we reach EOF and throw appropriate 
exception.

Re: [OpenJDK 2D-Dev] [15] RFR JDK-6532025: GIF reader throws misleading exception with truncated images

[Jayathirth D v]

Hi Brian,

Thanks for the clarification and approval.

@Others : Need one more review please take a look.

Regards,
Jay 

> On 06-Mar-2020, at 2:05 AM, Brian Burkhalter  
> wrote:
> 
> Hi Jay,
> 
> I think the overall change is fine.
> 
> Regards,
> 
> Brian
> 
>> On Mar 5, 2020, at 9:18 AM, Jayathirth D v > > wrote:
>> 
>> Hello Brian,
>> 
>> Thanks for the review. GIF stream in regression test case would match 
>> warn.gif stream behaviour that’s why it going into GIFImageReader.getCode().
>> 
>> Are you okay with overall webrev.00 patch or have you just approved 
>> GIFImageReader change? Please clarify.
>> 
>> Regards,
>> Jay
>> 
>>> On 05-Mar-2020, at 10:20 PM, Brian Burkhalter >> > wrote:
>>> 
>>> Hello Jay,
>>> 
>>> The source fix looks OK to me. I get the same exception as in the bug 
>>> description when I run the test against my unpatched local JDK 15 build.
>>> 
>>> Thanks,
>>> 
>>> Brian
>>> 
 On Mar 5, 2020, at 2:12 AM, Jayathirth D v >>> > wrote:
 
 Please review the following fix for JDK 15:
 
 Bug : https://bugs.openjdk.java.net/browse/JDK-6532025 
 
 Webrev : http://cr.openjdk.java.net/~jdv/6532025/webrev.00/ 
  
 
 Root cause : When we have truncated GIF images, stream.read() returns -1 
 but GIFImageReader doesn’t handle such conditions properly and continues 
 to read input stream data.
 Solution : Handle cases where we reach EOF and throw appropriate exception.
>>> 
>> 
> 

Re: [OpenJDK 2D-Dev] [15] RFR JDK-6532025: GIF reader throws misleading exception with truncated images

[Brian Burkhalter]

Hi Jay,

I think the overall change is fine.

Regards,

Brian

> On Mar 5, 2020, at 9:18 AM, Jayathirth D v  wrote:
> 
> Hello Brian,
> 
> Thanks for the review. GIF stream in regression test case would match 
> warn.gif stream behaviour that’s why it going into GIFImageReader.getCode().
> 
> Are you okay with overall webrev.00 patch or have you just approved 
> GIFImageReader change? Please clarify.
> 
> Regards,
> Jay
> 
>> On 05-Mar-2020, at 10:20 PM, Brian Burkhalter > > wrote:
>> 
>> Hello Jay,
>> 
>> The source fix looks OK to me. I get the same exception as in the bug 
>> description when I run the test against my unpatched local JDK 15 build.
>> 
>> Thanks,
>> 
>> Brian
>> 
>>> On Mar 5, 2020, at 2:12 AM, Jayathirth D v >> > wrote:
>>> 
>>> Please review the following fix for JDK 15:
>>> 
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-6532025 
>>> 
>>> Webrev : http://cr.openjdk.java.net/~jdv/6532025/webrev.00/ 
>>>  
>>> 
>>> Root cause : When we have truncated GIF images, stream.read() returns -1 
>>> but GIFImageReader doesn’t handle such conditions properly and continues to 
>>> read input stream data.
>>> Solution : Handle cases where we reach EOF and throw appropriate exception.
>> 
> 

Re: [OpenJDK 2D-Dev] [15] RFR JDK-6532025: GIF reader throws misleading exception with truncated images

[Jayathirth D v]

Hello Brian,

Thanks for the review. GIF stream in regression test case would match warn.gif 
stream behaviour that’s why it going into GIFImageReader.getCode().

Are you okay with overall webrev.00 patch or have you just approved 
GIFImageReader change? Please clarify.

Regards,
Jay

> On 05-Mar-2020, at 10:20 PM, Brian Burkhalter  
> wrote:
> 
> Hello Jay,
> 
> The source fix looks OK to me. I get the same exception as in the bug 
> description when I run the test against my unpatched local JDK 15 build.
> 
> Thanks,
> 
> Brian
> 
>> On Mar 5, 2020, at 2:12 AM, Jayathirth D v > > wrote:
>> 
>> Please review the following fix for JDK 15:
>> 
>> Bug : https://bugs.openjdk.java.net/browse/JDK-6532025 
>> 
>> Webrev : http://cr.openjdk.java.net/~jdv/6532025/webrev.00/ 
>>  
>> 
>> Root cause : When we have truncated GIF images, stream.read() returns -1 but 
>> GIFImageReader doesn’t handle such conditions properly and continues to read 
>> input stream data.
>> Solution : Handle cases where we reach EOF and throw appropriate exception.
> 

Re: [OpenJDK 2D-Dev] [15] RFR JDK-6532025: GIF reader throws misleading exception with truncated images

[Brian Burkhalter]

Hello Jay,

The source fix looks OK to me. I get the same exception as in the bug 
description when I run the test against my unpatched local JDK 15 build.

Thanks,

Brian

> On Mar 5, 2020, at 2:12 AM, Jayathirth D v  wrote:
> 
> Please review the following fix for JDK 15:
> 
> Bug : https://bugs.openjdk.java.net/browse/JDK-6532025 
> 
> Webrev : http://cr.openjdk.java.net/~jdv/6532025/webrev.00/ 
>  
> 
> Root cause : When we have truncated GIF images, stream.read() returns -1 but 
> GIFImageReader doesn’t handle such conditions properly and continues to read 
> input stream data.
> Solution : Handle cases where we reach EOF and throw appropriate exception.