Hi Sergey, Thanks for pointing it out. It looks like redundant try-catch block. 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.
