Hi Sergey, It will be pushed after removing the redundant code. Please find the webrev: http://cr.openjdk.java.net/~jdv/8044289/webrev.07/
Thanks, Jay -----Original Message----- From: Sergey Bylokhov Sent: Tuesday, April 05, 2016 8:05 PM To: Jayathirth D V Cc: 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In ImageIO.read() and write() NPE is not handled properly for stream On 30.03.16 9:48, Jayathirth D V wrote: > Thanks for pointing it out. It looks like redundant try-catch block. There is small code clean up is possible after your latest change: 1587 ImageOutputStream stream = null; 1588 stream = createImageOutputStream(output); I guess such code is redundant too. > I have removed these blocks in write() methods so that we are handling "cant > create cache file" exception in common way in all read() and write() methods. > Also I have updated test case description to hold true for only those read() > and write() methods where we are using createImageXXXStream() for readability. > > Please find updated webrev for review: > http://cr.openjdk.java.net/~jdv/8044289/webrev.06/ > > After CCC is done we will push above change to repo. > > Thanks, > Jay > > -----Original Message----- > From: Sergey Bylokhov > Sent: Tuesday, March 29, 2016 6:11 PM > To: Philip Race; Jayathirth D V > Cc: 2d-dev@openjdk.java.net > Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In > ImageIO.read() and write() NPE is not handled properly for stream > > I am not sure what is the reason of this code? > 1546 try { > 1547 output.delete(); > 1548 stream = createImageOutputStream(output); > 1549 } catch (IOException e) { > 1550 throw e; > 1551 } > > On 29.03.16 1:12, Philip Race wrote: >> This all looks fine to me. Although no new Exception types are thrown >> I think we need a CCC anyway, so please prepare and get that approved >> before pushing. >> >> -phil. >> >> On 3/28/16, 3:30 AM, Jayathirth D V wrote: >>> >>> Hi Phil, >>> >>> After discussion with Sergey we have come to conclusion that Sergey >>> is okay with throwing exception or returning null/false. >>> >>> So I have modified the webrev to throw exception. >>> >>> Also in write() I have modified the catch block to just throw the >>> caught exception without adding message since it will create >>> confusion between the case where we are not able to create cache and >>> when >>> createImageOutputStream() returns null. >>> >>> Regarding catch block of createImageOutputStream() only when we >>> usecache is true we use FileCacheImageOutputStream() and it throws >>> IOException so I think there is no need to add extra check before >>> throwing IOException. >>> >>> Please find updated webrev for review : >>> >>> http://cr.openjdk.java.net/~jdv/8044289/webrev.04/ >>> <http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.04/> >>> >>> Thanks, >>> >>> Jay >>> >>> *From:*Jayathirth D V >>> *Sent:* Thursday, March 24, 2016 3:52 PM >>> *To:* Philip Race >>> *Cc:* Sergey Bylokhov; 2d-dev@openjdk.java.net >>> *Subject:* RE: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In >>> ImageIO.read() and write() NPE is not handled properly for stream >>> >>> Hi Phil, >>> >>> We can change the code to something like : >>> >>> 1522 * @return <code>false</code> if no appropriate writer is >>> found or >>> 1523 * when needed stream is null. >>> >>> 1545 try { >>> 1546 output.delete(); >>> 1547 stream = createImageOutputStream(output); >>> 1548 } catch (IOException e) { >>> 1549 *throw e;* >>> 1550 } >>> >>> In this case we will be throwing IOException when we are not able to >>> create cache file and stream is null. >>> >>> And return null/false when createImageXXXStream returns null. Also >>> we can add check before throwing IIOException from >>> createImageXXXStream for usecache. >>> >>> 350 >>> 351 boolean usecache = getUseCache() && hasCachePermission(); >>> 352 >>> 353 while (iter.hasNext()) { >>> 354 ImageInputStreamSpi spi = iter.next(); >>> 355 if (spi.getInputClass().isInstance(input)) { >>> 356 try { >>> 357 return spi.createInputStreamInstance(input, >>> 358 usecache, >>> 359 >>> getCacheDirectory()); >>> 360 } catch (IOException e) { >>> 361 *if (usecache)* >>> 362 throw new IIOException("Can't create cache >>> file!", e); >>> 363 } >>> 364 } >>> 365 } >>> >>> This will be one of the approach. >>> >>> Or we can throw IOException in all the cases with additional check >>> in createImageXXXStream for usecache. This will be like throwing >>> IOException when createImageXXXStream returns null/false or it >>> throws IIOException when it cant create cache file. This will be >>> another approach. Please let us know your inputs. >>> >>> Thanks, >>> >>> Jay >>> >>> *From:*Philip Race >>> *Sent:* Thursday, March 24, 2016 5:10 AM >>> *To:* Jayathirth D V >>> *Cc:* Sergey Bylokhov; 2d-dev@openjdk.java.net >>> <mailto:2d-dev@openjdk.java.net> >>> *Subject:* Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In >>> ImageIO.read() and write() NPE is not handled properly for stream >>> >>> I don't think this is ready and we need to discuss whether to rework it. >>> >>> In general I think I prefer IIO rather than null when we have a stream >>> problem. >>> >>> >>> For one thing you have this change :- >>> >>> 1522 * @return <code>false</code> if no appropriate writer is >>> found or >>> 1523 * not able to create required ImageOutputStream. >>> yet the implementation says :- >>> 1545 try { >>> 1546 output.delete(); >>> 1547 stream = createImageOutputStream(output); >>> 1548 } catch (IOException e) { >>> 1549 throw new IIOException("Can't create output stream!", e); >>> 1550 } >>> So whilst "null" would mean we cannot find an IOS SPI it isn't the >>> only reason we fail to create the IOS A null return seems to get >>> passed through to doWrite() which probably throws IIOE And I think >>> that the implementation is actually right here where it throws an >>> exception. >>> The ability to create an image input or output stream could be >>> because of some kind of problem opening the file, or with the IIS or >>> IOS implementations - like they were de-registered. I think in many >>> if not all of these cases IOE - or IIOE is what we want. >>> Also since we call createImageInputStream() - which is public - we >>> need to look at that, and what it does, which is also something to consider. >>> If it returns null, that seems to mean that no suitable SPI is >>> registered which can only happen if they are de-registered. For the >>> methods where we use createImageInputStream() I think it is fair to >>> turn that into IIOE BTW I note that the code there says :- >>> 350 >>> 351 boolean usecache = getUseCache() && hasCachePermission(); >>> 352 >>> 353 while (iter.hasNext()) { >>> 354 ImageInputStreamSpi spi = iter.next(); >>> 355 if (spi.getInputClass().isInstance(input)) { >>> 356 try { >>> 357 return spi.createInputStreamInstance(input, >>> 358 usecache, >>> 359 >>> getCacheDirectory()); >>> 360 } catch (IOException e) { >>> 361 throw new IIOException("Can't create cache >>> file!", e); >>> 362 } >>> 363 } >>> 364 } >>> So far I can see that means it will claim any Exception that is >>> generated is because it could not create the cache file without any proof >>> of that and even if "useCache" is false. >>> It seems to imply to me that the author was not considering things >>> like someone passing a closed InputStream .. probably we ought to be >>> more vague in this case or look more deeply at what we can tell from >>> whether it is IIOE or IOE. >>> -phil. >>> >>> On 3/22/16, 10:49 AM, Jayathirth D V wrote: >>> >>> Thanks for the review Sergey. >>> >>> Can I get +1 for this please? >>> >>> -Jay >>> >>> -----Original Message----- >>> >>> From: Sergey Bylokhov >>> >>> Sent: Tuesday, March 22, 2016 9:52 PM >>> >>> To: Jayathirth D V; Philip Race >>> >>> Cc:2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net> >>> >>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : >>> In >>> ImageIO.read() and write() NPE is not handled properly for stream >>> >>> This fix looks fine to me. >>> >>> At least it made all code work in a similar way. But probably someone >>> will have other opinion? >>> >>> On 22.03.16 12:34, Jayathirth D V wrote: >>> >>> Hi Sergey, >>> >>> I have unified changes related to ImageIO.read() and >>> ImageIO.write(). >>> >>> In case of read() we will be returning null when >>> createImageInputStream() returns null. >>> >>> In case of write() we will be returning false when >>> createImageOutputStream() returns null. >>> >>> Please find updated webrev for review: >>> >>> http://cr.openjdk.java.net/~jdv/8044289/webrev.03/ >>> <http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.03/> >>> >>> Thanks, >>> >>> Jay >>> >>> -----Original Message----- >>> >>> From: Sergey Bylokhov >>> >>> Sent: Tuesday, March 22, 2016 7:39 AM >>> >>> To: Jayathirth D V; Philip Race >>> >>> Cc:2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net> >>> >>> Subject: Re: [OpenJDK 2D-Dev] Review Request for >>> JDK-8044289 >>> : In >>> >>> ImageIO.read() and write() NPE is not handled properly for >>> stream >>> >>> Hi, Jay. >>> >>> Since we decided to update the specification we get an additional >>> flexibility in the fix, because we can update the "@exception IOException" >>> part of javadoc and describe that it can be thrown if ImageXXXStream cannot >>> be created. >>> >>> I don't see a big differences of both approaches in case of >>> write() methods, but in case of read(which has the similar issue but was >>> skipped in the fix) there is a difference. >>> >>> Note that the read(File) unlike other methods throws the >>> unspecified >>> >>> IIOException exception if ImageXXXStream cannot be created. >>> We can >>> >>> think about unification of read/write methods in ImageIO. >>> Hmm but even >>> >>> then >>> >>> read() in some cases will return null and in some cases throw an >>> exception.... >>> >>> and createImageInputStream/createImageOutputStream() will look >>> alien as they always return null not an exception. >>> >>> Another possible unification is to update javadoc for all >>> methods, >>> >>> that they will return null if related providers are not >>> found(in this >>> >>> case "throw new IIOException("Can't create an ImageInputStream!"))" >>> >>> should be removed from read(File); >>> >>> On 21.03.16 10:26, Jayathirth D V wrote: >>> >>> Hi Sergey, >>> >>> For the second approach I have created webrev for review. >>> >>> Please review updated webrev : >>> >>> http://cr.openjdk.java.net/~jdv/8044289/webrev.02/ >>> <http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.02/> >>> >>> Thanks, >>> >>> Jay >>> >>> -----Original Message----- >>> >>> From: Jayathirth D V >>> >>> Sent: Friday, March 18, 2016 2:23 PM >>> >>> To: Sergey Bylokhov >>> >>> Cc:2d-dev@openjdk.java.net >>> <mailto:2d-dev@openjdk.java.net>; Philip Race >>> >>> Subject: RE: [OpenJDK 2D-Dev] Review Request for >>> JDK-8044289 : In >>> >>> ImageIO.read() and write() NPE is not handled properly >>> for stream >>> >>> Hi Sergey, >>> >>> Thanks for your inputs. >>> >>> Indeed all the null values returned by >>> createImageInputStream() and createImageInputStream() are valid only. >>> >>> I went through the code more and found that there is no need >>> for fix in ImageIO.read() methods as null stream is handled in >>> read(ImageInputStream stream) which is called by all the remaining read() >>> API's through IAE. So i can remove the changes made in ImageIO.read() >>> functions. Also istream is closed in finally block of read(url). >>> >>> For ImageIO.write(File) & ImageIO.write(OutputStream) NPE we >>> have two approaches for fix: >>> >>> 1) Keep the present changes of throwing IOException when >>> stream is null and change the throw clause of write() spec mentioning: >>> >>> "IOException - if an error occurs during writing or not >>> able to create required ImageOutputStream" >>> >>> 2) Remove the present changes and handle null in doWrite() >>> method by returning false when stream is null & add code in finally block >>> to close the stream only when stream is not null. Along with this we >>> can also change Returns clause of spec to mention that write() might return >>> false when we are not able to create ImageOutputStream. >>> >>> >>> >>> private static boolean doWrite(RenderedImage >>> im, ImageWriter writer, >>> >>> >>> ImageOutputStream output) throws IOException { >>> >>> if (writer == null) { >>> >>> return false; >>> >>> } >>> >>> + if (output == null) { >>> >>> + return false; >>> >>> + } >>> >>> >>> >>> try { >>> >>> return doWrite(im, >>> writer, stream); >>> >>> } finally { >>> >>> + if (stream != null) >>> >>> stream.close(); >>> >>> } >>> >>> >>> >>> Returns : >>> >>> false if no appropriate writer is found or not >>> able to create required ImageOutputStream. >>> >>> Please let me know your inputs. >>> >>> Thanks, >>> >>> Jay >>> >>> -----Original Message----- >>> >>> From: Sergey Bylokhov >>> >>> Sent: Friday, March 18, 2016 12:24 AM >>> >>> To: Jayathirth D V >>> >>> Cc:2d-dev@openjdk.java.net >>> <mailto:2d-dev@openjdk.java.net>; Philip Race >>> >>> Subject: Re: [OpenJDK 2D-Dev] Review Request for >>> JDK-8044289 : In >>> >>> ImageIO.read() and write() NPE is not handled properly >>> for stream >>> >>> On 17.03.16 21:14, Jayathirth D V wrote: >>> >>> There are many places in createImageInputStream() and >>> createImageOutputStream() where it will return null for stream. >>> >>> But when why you change the write/read methods? They should >>> take null into account and work according to specification. If some bogus >>> null returns from createImageInputStream/createImageOutputStream then it >>> should be fixed in these methods. >>> >>> So for NPE described in bug to occur there can be multiple >>> paths: >>> >>> 1) One of the path described in bug is when >>> theRegistry.getServiceProviders() throws IAE and we return null. >>> >>> This is the case when we have no installed spi and should >>> return null/false. >>> >>> 2) When we pass non-existent file path to ImageIO (as >>> described >>> inhttp://stackoverflow.com/questions/11153200/with-imageio-write-api-call-i-get-nullpointerexception >>> ). Here we catch FileNotFoundException but still we return stream as null >>> at the end which will cause NPE further. >>> >>> This is same thing described in the bug(about stream.close()). >>> The write method call createImageOutputStream() and got >>> FileNotFoundException. >>> >>> This exception should be propagated to the user, but it is >>> replaced in the finally block, because we try to close the stream=null. So >>> the fix for this particular case is to check the stream to null in finally, >>> No? >>> >>> 3) When we don't support the provider and >>> theRegistry.getServiceProviders() return iterator with size zero. >>> >>> This means that our providers do not support this case, and we >>> should return null/false. >>> >>> For us to follow first scenario we need access to >>> categoryMap of IIORegistry and delete the ImageInputStreamSpi/ >>> ImageOutputStreamSpi but I was not able to find a way to tamper it from >>> test case. If we follow second case FileNotFoundException will be thrown by >>> default eventhough we handle NPE. >>> >>> But you change the code and we now contradicts the >>> specification, in your case the test should expect false/null from >>> write()/read(). >>> >>> In the test case we are trying to following third scenario >>> to get stream as null to verify the code changes. >>> >>> To catch the bogus null we should try to use another approach, >>> or change the spec. But it seems all null values above are correct, we >>> should be ready for them. >>> >>> As part of collective fix we are throwing IOException >>> which will indicate that we are not able to create required stream. >>> >>> Thanks, >>> >>> Jay >>> >>> -----Original Message----- >>> >>> From: Sergey Bylokhov >>> >>> Sent: Thursday, March 17, 2016 7:27 PM >>> >>> To: Phil Race >>> >>> Cc: Jayathirth D V;2d-dev@openjdk.java.net >>> <mailto:2d-dev@openjdk.java.net> >>> >>> Subject: Re: [OpenJDK 2D-Dev] Review Request for >>> JDK-8044289 : In >>> >>> ImageIO.read() and write() NPE is not handled >>> properly for stream >>> >>> On 17.03.16 0:36, Phil Race wrote: >>> >>> I don't think that is the actual reason here >>> >>> But the test doing exactly this thing -> deregister >>> all spi => in >>> >>> this case according to specification all related >>> >>> >>> methods(read/write/creatImageInputStream/creatImageOutputStream) should >>> return null or false. >>> >>> - it seems likely that there >>> >>> was some kind of internal error or bad SPI causing >>> this. >>> >>> So throwing an exception seems more appropriate. >>> >>> the problem was in some of our methods which do not expect >>> null in some places(like stream.close() from the bug description). >>> >>> And also the bug was originally solely about write(..). >>> >>> -phil. >>> >>> On 03/16/2016 01:35 PM, Sergey Bylokhov wrote: >>> >>> As far as I understand the >>> createImageInputStream() returns null >>> >>> is this stream is unsupported by current >>> plugins via spi. Probably >>> >>> the >>> >>> read() method should return null too? >>> >>> /** >>> >>> * Returns a {@code BufferedImage} >>> as the result of decoding >>> >>> * a supplied {@code InputStream} >>> with an {@code ImageReader} >>> >>> * chosen automatically from among those >>> currently registered. >>> >>> * The {@code InputStream} is >>> wrapped in an >>> >>> * {@code ImageInputStream}. If no >>> registered >>> >>> * {@code ImageReader} claims to be >>> able to read the >>> >>> * resulting stream, {@code null} is >>> returned. >>> >>> * >>> >>> ....... >>> >>> * @return a {@code BufferedImage} >>> containing the decoded >>> >>> * contents of the input, or {@code null}. >>> >>> */ >>> >>> public static BufferedImage >>> read(InputStream input) throws >>> >>> IOException >>> >>> On 16.03.16 20:29, Philip Race wrote: >>> >>> The test writes out into "test.src". >>> >>> I believe that you should treat that as a >>> "read-only" location. >>> >>> Write to a tempfile (File.createTempFile()) or >>> TESTCLASSES. >>> >>> -phil. >>> >>> On 3/15/16, 10:50 PM, Jayathirth D V wrote: >>> >>> Hi Phil,All >>> >>> _Please review the following fix in >>> JDK9:_ >>> >>> __ >>> >>> Bug >>> :https://bugs.openjdk.java.net/browse/JDK-8044289 >>> >>> Webrev >>> :http://cr.openjdk.java.net/~jdv/8044289/webrev.00/ >>> >>> <http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.00/> >>> >>> >>> <http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.00/> >>> >>> <http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.00/> >>> >>> Issue : When >>> createImageInputStream() or createImageOutputStream >>> >>> returns null in ImageIO.read() or >>> ImageIO.write() we are seeing >>> >>> NPE as it stream is used further without >>> null check. >>> >>> Root cause : "stream" is used >>> without checking whether it is >>> >>> null or not. >>> >>> Solution : Handle null pointer of stream >>> and throw IIOException. >>> >>> Test case contains all possible >>> scenarios where >>> >>> createImageInputStream() or >>> createImageOutputStream can return null. >>> >>> Thanks, >>> >>> Jay >>> >>> -- >>> >>> Best regards, Sergey. >>> >>> -- >>> >>> Best regards, Sergey. >>> >>> -- >>> >>> Best regards, Sergey. >>> >>> -- >>> >>> Best regards, Sergey. >>> > > > -- > Best regards, Sergey. > -- Best regards, Sergey.
