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 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/ > > 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. > -- Best regards, Sergey.
