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

[Andrew Hughes]

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

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