Hi Sergey, My Observations:
There are many places in createImageInputStream() and createImageOutputStream() where it will return null for stream. 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. 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. 3) When we don't support the provider and theRegistry.getServiceProviders() return iterator with size zero. 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. In the test case we are trying to following third scenario to get stream as null to verify the code changes. 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.
