[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/ <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 <philip.race at oracle.com> 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 <brian.burkhalter at oracle.com <mailto:brian.burkhalter at 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 <JAYATHIRTH.D.V at ORACLE.COM <mailto:JAYATHIRTH.D.V at 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 <brian.burkhalter at oracle.com <mailto:brian.burkhalter at 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 <JAYATHIRTH.D.V at ORACLE.COM <mailto:JAYATHIRTH.D.V at ORACLE.COM>> wrote:
>>>>>> 
>>>>>> Please review the following fix for JDK 15:
>>>>>> 
>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-6532025 <https://bugs.openjdk.java.net/browse/JDK-6532025>
>>>>>> Webrev : http://cr.openjdk.java.net/~jdv/6532025/webrev.00/ <http://cr.openjdk.java.net/%7Ejdv/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.
>>>>> 
>>>> 
>>> 
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20200306/49122480/attachment.htm>