RFR: 8175010: ImageReader is not thread-safe

[Peter Levart]

Hi Claes,

Reading the https://bugs.openjdk.java.net/browse/JDK-8174817 and then 
the change that was just pushed, I can't seem to figure out what was the 
problem with original code. I can't find evidence for claims in 
https://bugs.openjdk.java.net/browse/JDK-8175010 . Is the problem 
publication of ImageReader via ImageReaderFactory? That can't be it, 
since ImageReaderFactory is using ConcurrentHashMap which ensures 
happens before relationships.

Is there any place else where ImageReader.open(Path) is called and then 
the instance unsafely published to other threads? The only place I could 
find is in jdk.internal.jrtfs.SystemImage.open():

     static SystemImage open() throws IOException {
         if (modulesImageExists) {
             // open a .jimage and build directory structure
             final ImageReader image = ImageReader.open(moduleImageFile);
             image.getRootDirectory();
             return new SystemImage() {
                 @Override
                 Node findNode(String path) throws IOException {
                     return image.findNode(path);
                 }
                 @Override
                 byte[] getResource(Node node) throws IOException {
                     return image.getResource(node);
                 }
                 @Override
                 void close() throws IOException {
                     image.close();
                 }
             };
         }

...here the final 'image' local variable is captured by anonymous inner 
SystemImage subclass into a synthetic final field, so this final field 
ensures that ImageReader is published safely as a delegate of SystemImage.

ImageReader.open(Path) factory method delegates to 
ImageReader.SharedImageReader.open(Path, ByteOrder) which creates a new 
instance of ImageReader encapsulating a SharedImageReader object:

           public static ImageReader open(Path imagePath, ByteOrder 
byteOrder) throws IOException {
             Objects.requireNonNull(imagePath);
             Objects.requireNonNull(byteOrder);

             synchronized (OPEN_FILES) {
                 SharedImageReader reader = OPEN_FILES.get(imagePath);

                 if (reader == null) {
                     // Will fail with an IOException if wrong byteOrder.
                     reader =  new SharedImageReader(imagePath, byteOrder);
                     OPEN_FILES.put(imagePath, reader);
                 } else if (reader.getByteOrder() != byteOrder) {
                     throw new IOException("\"" + reader.getName() + "\" 
is not an image file");
                 }

                 ImageReader image = new ImageReader(reader); // <<-- HERE
                 reader.openers.add(image);

                 return image;
             }
         }

...the ImageReader returned from this method is not safe to publish via 
data race, but as far as I can see, there is no such publishing going 
on. So am I right in that all this patch solves is it eliminates a 
possibility of NPE when ImageReader is close()-d concurrently with being 
used from some other thread. If such NPE was observed, it means that 
close() is being called concurrently with access and there are still 
races possible which can cause undesired effects. For example: calling 
ImageReader.close() while using it concurrently may close underlying 
SharedImageReader and then after closing, access it.

So is there a concurrent ImageReader.close() possible? The only place I 
see ImageReader.close() being invoked is from SystemImage.close() in the 
anonymous inner class implementation which wraps ImageReader. 
SystemImage.close() is only being invoked from JrtFileSystem.cleanup(), 
which is called from JrtFileSystem.close() and JrtFileSystem.finalize().

The following is theoretically possible:

FileSystem fs = FileSystems.newFileSystem(URI.create("jrt:/"), ...);

Path p = fs.getPath(...);
FileSystemProvider provider = fs.provider();
InputStream is = provider.newInputStream(p, ...);
// 'fs' and 'p' (which has a reference to 'fs') may be found finalizable 
and be finalized while the call to obtain content of input stream is in 
progress

For this to be prevented, the following method in JrtFileSystem:

     // returns the content of the file resource specified by the path
     byte[] getFileContent(JrtPath path) throws IOException {
         Node node = checkNode(path);
         if (node.isDirectory()) {
             throw new FileSystemException(path + " is a directory");
         }
         //assert node.isResource() : "resource node expected here";
         return image.getResource(node);
     }

...would have to be changed to:

     byte[] getFileContent(JrtPath path) throws IOException {
         Node node = checkNode(path);
         if (node.isDirectory()) {
             throw new FileSystemException(path + " is a directory");
         }
         //assert node.isResource() : "resource node expected here";
         byte[] content = image.getResource(node);
         Reference.reachabilityFence(this);
         return content;
     }


I don't claim that this is what causes the problems and I haven't found 
any such usages, but this is theoretically possible.

Regards, Peter

On 02/15/2017 02:22 PM, Claes Redestad wrote:
> Hi,
>
> a few intermittent but rare test failures[1] that has appeared
> since the latest jake integration, and since one of the changes
> in there was to make initialization of the system ImageReader
> lazy there appears to be cases where ImageReaders are not
> safely published:
>
> - Ensure ImageReader::open is called only once per Path in
> ImageReaderFactory by using CHM.computeIfAbsent
> - Ensure ImageReader.reader is safely published to a
> final field and signal close using a volatile boolean instead
>
> webrev: http://cr.openjdk.java.net/~redestad/8175010/webrev.02/
> bug: https://bugs.openjdk.java.net/browse/JDK-8175010
>
> Testing shows no issues (which admittedly doesn't mean we're
> actually solving the root cause for JDK-8174817), and performance
> numbers from adding a volatile read indicate that any overhead
> is lost in the noise on ImageReader-heavy workloads.
>
> Thanks!
>
> /Claes
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8174817