[OpenJDK 2D-Dev] <AWT Dev> [rfc]Stream doesn't reset mark in finally

[Jiri Vanek]

On 12/02/2015 10:26 PM, Andrew Hughes wrote:
> ----- Original Message -----
>> On 11/25/2015 06:53 PM, Andrew Hughes wrote:
>>> ----- Original Message -----
>>>> On 11/18/2015 06:17 PM, Jiri Vanek wrote:
>>>>> On 11/12/2015 02:24 PM, Sergey Bylokhov wrote:
>>>>>> Hi, Jiri.
>>>>>> This is a valid point, did you file a new CR for this issue?
>>>>
>>>> ping?
>>>>
>>>>>>
>>>>>
>>>>> Hello!
>>>>>
>>>>> here it is:
>>>>> https://jvanek.fedorapeople.org/oracle/jdk8/webrevs/resetInTryFinally/v1/
>>>>>
>>>>> Patch:
>>>>> https://jvanek.fedorapeople.org/oracle/jdk8/webrevs/resetInTryFinally/v1/webrev/
>>>>> and reprodcuer
>>>>> https://jvanek.fedorapeople.org/oracle/jdk8/webrevs/resetInTryFinally/v1/MarkTryFinallyReproducer.java
>>>>>
>>>>>
>>>>> Reproducer can be easily turned to jtreg if wonted.
>>>>>
>>>>> The patch is for 8, but is valid also for 9, except the path to file.
>>>>> I will adapt it to 9 once you are ok with it (or you may on your own,
>>>>> depends on you)
>>>>>
>>>>> J.
>>>>>
>>>>>
>>>>>> On 09.11.15 15:45, Alexander Scherbatiy wrote:
>>>>>>> On 11/7/2015 11:38 AM, Jiri Vanek wrote:
>>>>>>>> Hello!
>>>>>>>>
>>>>>>>> Looking to imageIO.java (if this is bad thread, please redirect me!)
>>>>>>>
>>>>>>>        2d-dev alias should be also the right place to ask image related
>>>>>>> questions in AWT.
>>>>>>>
>>>>>>>       Thanks,
>>>>>>>       Alexandr.
>>>>>>>
>>>>>>>> when reading images
>>>>>>>> http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/javax/imageio/ImageIO.java#l543
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> and ending at:
>>>>>>>>
>>>>>>>>                   // Perform mark/reset as a defensive measure
>>>>>>>>                   // even though plug-ins are supposed to take
>>>>>>>>                   // care of it.
>>>>>>>>                   boolean canDecode = false;
>>>>>>>>                   if (stream != null) {
>>>>>>>>                       stream.mark();
>>>>>>>>                   }
>>>>>>>>                   canDecode = spi.canDecodeInput(input);
>>>>>>>>                   if (stream != null) {
>>>>>>>>                       stream.reset();
>>>>>>>>                   }
>>>>>>>>                   return canDecode;
>>>>>>>>
>>>>>>>> I'm wondering, why  stream.reset(); is not in finaly  block:
>>>>>>>>
>>>>>>>> // Perform mark/reset as a defensive measure
>>>>>>>> // even though plug-ins are supposed to take
>>>>>>>> // care of it.
>>>>>>>> boolean canDecode = false;
>>>>>>>> if (stream != null) {
>>>>>>>> stream.mark();
>>>>>>>> }
>>>>>>>> try{
>>>>>>>>       canDecode = spi.canDecodeInput(input);
>>>>>>>> } finally {
>>>>>>>>       if (stream != null) {
>>>>>>>>           stream.reset();
>>>>>>>>       }
>>>>>>>> }
>>>>>>>> return canDecode;
>>>>>>>>
>>>>>>>>
>>>>>>>> Eg png and bmp decoders can are throwing IIOException when header is
>>>>>>>> corrutped. That pretty definitely sure killer of  stream mark stacks.
>>>>>>>> You yourselves write  : //Perform mark/reset as a defensive measure
>>>>>>>> even though plug-ins are supposed to take care of it.
>>>>>>>> So if densive, then try/finaly please.
>>>>>>>>
>>>>>>>> I did not check if this changed in 9 but if not....Please. This is
>>>>>>>> makig work on custom image plugins, for image formats which just wraps
>>>>>>>> another bmp/png and are expecting corrupted headers preatty hards.
>>>>>>>>
>>>>>>>>
>>>>>>>> J.
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>> I've filed https://bugs.openjdk.java.net/browse/JDK-8144071 for this.
>>
>> Here you go!
>>
>> https://jvanek.fedorapeople.org/oracle/jdk9/webrevs/resetInTryFinally/v2/
>>>
>>> The change looks good to me. I can confirm the reproducer fails
>>> on OpenJDK 6, 7 and 8:
>>>
>>> Exception in thread "main" java.lang.RuntimeException: exeption from call
>>> to canDecodeInput in ImageIO corrupted stack in ImageInputStream
>>> 	at MarkTryFinallyReproducer.main(MarkTryFinallyReproducer.java:317)
>>>
>>> I suggest the test case is converted to jtreg and the patch to
>> done
>>
>>> OpenJDK 9 for a new webrev. Once that is posted, I can commit
>>
>> done
>>> the change.
>>>
>>
>> TYVM!
>>
>>
>> I run all jtregs, and looked ok.
>>    J.
>>
>>
>
> Thanks. I cleaned up the test case formatting a little and
> pushed the change:
>
> http://hg.openjdk.java.net/jdk9/client/jdk/rev/284925b520f1
>
> Once it's made it to the master jdk9 repository and had time
> to soak there, you probably want to suggest it for backport to 8u.
>


Thank you!

J.