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/ 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 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/ > > 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; 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; 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 in >> http://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 >> 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/> >>>>>> >>>>>> 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.
