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

[Andrew Hughes]

----- 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.

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
OpenJDK 9 for a new webrev. Once that is posted, I can commit
the change.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222

PGP Key: rsa4096/248BDC07 (hkp://keys.gnupg.net)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07