8365467: Issues with jrtfs implementation for exploded run-time images

Reviewed-by: rriggs, sundar

Author: david-beaumont

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -27,17 +27,15 @@
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.nio.file.DirectoryStream;
-import java.nio.file.FileSystem;
 import java.nio.file.FileSystemException;
-import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.stream.Stream;
 
 import jdk.internal.jimage.ImageReader.Node;
@@ -56,16 +54,15 @@ class ExplodedImage extends SystemImage {
 
     private static final String MODULES = "/modules/";
     private static final String PACKAGES = "/packages/";
-    private static final int PACKAGES_LEN = PACKAGES.length();
 
-    private final FileSystem defaultFS;
+    private final Path modulesDir;
     private final String separator;
-    private final Map<String, PathNode> nodes = Collections.synchronizedMap(new HashMap<>());
+    private final Map<String, PathNode> nodes = new HashMap<>();
     private final BasicFileAttributes modulesDirAttrs;
 
     ExplodedImage(Path modulesDir) throws IOException {
-        defaultFS = FileSystems.getDefault();
-        String str = defaultFS.getSeparator();
+        this.modulesDir = modulesDir;
+        String str = modulesDir.getFileSystem().getSeparator();
         separator = str.equals("/") ? null : str;
         modulesDirAttrs = Files.readAttributes(modulesDir, BasicFileAttributes.class);
         initNodes();
@@ -79,21 +76,26 @@ private final class PathNode extends Node {
         private PathNode link;
         private List<Node> children;
 
-        PathNode(String name, Path path, BasicFileAttributes attrs) {  // path
+        private PathNode(String name, Path path, BasicFileAttributes attrs) {  // path
             super(name, attrs);
             this.path = path;
         }
 
-        PathNode(String name, Node link) {              // link
+        private PathNode(String name, Node link) {              // link
             super(name, link.getFileAttributes());
             this.link = (PathNode)link;
         }
 
-        PathNode(String name, List<Node> children) {    // dir
+        private PathNode(String name, List<Node> children) {    // dir
             super(name, modulesDirAttrs);
             this.children = children;
         }
 
+        @Override
+        public boolean isResource() {
+            return link == null && !getFileAttributes().isDirectory();
+        }
+
         @Override
         public boolean isDirectory() {
             return children != null ||
@@ -112,7 +114,7 @@ public PathNode resolveLink(boolean recursive) {
             return recursive && link.isLink() ? link.resolveLink(true) : link;
         }
 
-        byte[] getContent() throws IOException {
+        private byte[] getContent() throws IOException {
             if (!getFileAttributes().isRegularFile())
                 throw new FileSystemException(getName() + " is not file");
             return Files.readAllBytes(path);
@@ -126,7 +128,7 @@ public Stream<String> getChildNames() {
                 List<Node> list = new ArrayList<>();
                 try (DirectoryStream<Path> stream = Files.newDirectoryStream(path)) {
                     for (Path p : stream) {
-                        p = explodedModulesDir.relativize(p);
+                        p = modulesDir.relativize(p);
                         String pName = MODULES + nativeSlashToFrontSlash(p.toString());
                         Node node = findNode(pName);
                         if (node != null) {  // findNode may choose to hide certain files!
@@ -152,7 +154,7 @@ public long size() {
     }
 
     @Override
-    public void close() throws IOException {
+    public synchronized void close() throws IOException {
         nodes.clear();
     }
 
@@ -161,74 +163,78 @@ public byte[] getResource(Node node) throws IOException {
         return ((PathNode)node).getContent();
     }
 
-    // find Node for the given Path
     @Override
-    public synchronized Node findNode(String str) {
-        Node node = findModulesNode(str);
+    public synchronized Node findNode(String name) {
+        PathNode node = nodes.get(name);
         if (node != null) {
             return node;
         }
-        // lazily created for paths like /packages/<package>/<module>/xyz
-        // For example /packages/java.lang/java.base/java/lang/
-        if (str.startsWith(PACKAGES)) {
-            // pkgEndIdx marks end of <package> part
-            int pkgEndIdx = str.indexOf('/', PACKAGES_LEN);
-            if (pkgEndIdx != -1) {
-                // modEndIdx marks end of <module> part
-                int modEndIdx = str.indexOf('/', pkgEndIdx + 1);
-                if (modEndIdx != -1) {
-                    // make sure we have such module link!
-                    // ie., /packages/<package>/<module> is valid
-                    Node linkNode = nodes.get(str.substring(0, modEndIdx));
-                    if (linkNode == null || !linkNode.isLink()) {
-                        return null;
-                    }
-                    // map to "/modules/zyz" path and return that node
-                    // For example, "/modules/java.base/java/lang" for
-                    // "/packages/java.lang/java.base/java/lang".
-                    String mod = MODULES + str.substring(pkgEndIdx + 1);
-                    return findModulesNode(mod);
-                }
-            }
+        // If null, this was not the name of "/modules/..." node, and since all
+        // "/packages/..." nodes were created and cached in advance, the name
+        // cannot reference a valid node.
+        Path path = underlyingModulesPath(name);
+        if (path == null) {
+            return null;
         }
-        return null;
+        // This can still return null for hidden files.
+        return createModulesNode(name, path);
     }
 
-    // find a Node for a path that starts like "/modules/..."
-    Node findModulesNode(String str) {
-        PathNode node = nodes.get(str);
-        if (node != null) {
-            return node;
-        }
-        // lazily created "/modules/xyz/abc/" Node
-        // This is mapped to default file system path "<JDK_MODULES_DIR>/xyz/abc"
-        Path p = underlyingPath(str);
-        if (p != null) {
-            try {
-                BasicFileAttributes attrs = Files.readAttributes(p, BasicFileAttributes.class);
-                if (attrs.isRegularFile()) {
-                    Path f = p.getFileName();
-                    if (f.toString().startsWith("_the."))
-                        return null;
+    /**
+     * Lazily creates and caches a {@code Node} for the given "/modules/..." name
+     * and corresponding path to a file or directory.
+     *
+     * @param name a resource or directory node name, of the form "/modules/...".
+     * @param path the path of a file for a resource or directory.
+     * @return the newly created and cached node, or {@code null} if the given
+     *     path references a file which must be hidden in the node hierarchy.
+     */
+    private Node createModulesNode(String name, Path path) {
+        assert !nodes.containsKey(name) : "Node must not already exist: " + name;
+        assert isNonEmptyModulesPath(name) : "Invalid modules name: " + name;
+
+        try {
+            // We only know if we're creating a resource of directory when we
+            // look up file attributes, and we only do that once. Thus, we can
+            // only reject "marker files" here, rather than by inspecting the
+            // given name string, since it doesn't apply to directories.
+            BasicFileAttributes attrs = Files.readAttributes(path, BasicFileAttributes.class);
+            if (attrs.isRegularFile()) {
+                Path f = path.getFileName();
+                if (f.toString().startsWith("_the.")) {
+                    return null;
                 }
-                node = new PathNode(str, p, attrs);
-                nodes.put(str, node);
-                return node;
-            } catch (IOException x) {
-                // does not exists or unable to determine
+            } else if (!attrs.isDirectory()) {
+                return null;
             }
+            PathNode node = new PathNode(name, path, attrs);
+            nodes.put(name, node);
+            return node;
+        } catch (IOException x) {
+            // Since the path reference a file, any errors should not be ignored.
+            throw new UncheckedIOException(x);
         }
-        return null;
     }
 
-    Path underlyingPath(String str) {
-        if (str.startsWith(MODULES)) {
-            str = frontSlashToNativeSlash(str.substring("/modules".length()));
-            return defaultFS.getPath(explodedModulesDir.toString(), str);
+    /**
+     * Returns the expected file path for name in the "/modules/..." namespace,
+     * or {@code null} if the name is not in the "/modules/..." namespace or the
+     * path does not reference a file.
+     */
+    private Path underlyingModulesPath(String name) {
+        if (isNonEmptyModulesPath(name)) {
+            Path path = modulesDir.resolve(frontSlashToNativeSlash(name.substring(MODULES.length())));
+            return Files.exists(path) ? path : null;
         }
         return null;
     }
 
+    private static boolean isNonEmptyModulesPath(String name) {
+        // Don't just check the prefix, there must be something after it too
+        // (otherwise you end up with an empty string after trimming).
+        return name.startsWith(MODULES) && name.length() > MODULES.length();
+    }
+
     // convert "/" to platform path separator
     private String frontSlashToNativeSlash(String str) {
         return separator == null ? str : str.replace("/", separator);
@@ -249,24 +255,21 @@ private void initNodes() throws IOException {
         // same package prefix may exist in multiple modules. This Map
         // is filled by walking "jdk modules" directory recursively!
         Map<String, List<String>> packageToModules = new HashMap<>();
-        try (DirectoryStream<Path> stream = Files.newDirectoryStream(explodedModulesDir)) {
+        try (DirectoryStream<Path> stream = Files.newDirectoryStream(modulesDir)) {
             for (Path module : stream) {
                 if (Files.isDirectory(module)) {
                     String moduleName = module.getFileName().toString();
                     // make sure "/modules/<moduleName>" is created
-                    findModulesNode(MODULES + moduleName);
+                    Objects.requireNonNull(createModulesNode(MODULES + moduleName, module));
                     try (Stream<Path> contentsStream = Files.walk(module)) {
                         contentsStream.filter(Files::isDirectory).forEach((p) -> {
                             p = module.relativize(p);
                             String pkgName = slashesToDots(p.toString());
                             // skip META-INF and empty strings
                             if (!pkgName.isEmpty() && !pkgName.startsWith("META-INF")) {
-                                List<String> moduleNames = packageToModules.get(pkgName);
-                                if (moduleNames == null) {
-                                    moduleNames = new ArrayList<>();
-                                    packageToModules.put(pkgName, moduleNames);
-                                }
-                                moduleNames.add(moduleName);
+                                packageToModules
+                                        .computeIfAbsent(pkgName, k -> new ArrayList<>())
+                                        .add(moduleName);
                             }
                         });
                     }
@@ -275,8 +278,8 @@ private void initNodes() throws IOException {
         }
         // create "/modules" directory
         // "nodes" map contains only /modules/<foo> nodes only so far and so add all as children of /modules
-        PathNode modulesDir = new PathNode("/modules", new ArrayList<>(nodes.values()));
-        nodes.put(modulesDir.getName(), modulesDir);
+        PathNode modulesRootNode = new PathNode("/modules", new ArrayList<>(nodes.values()));
+        nodes.put(modulesRootNode.getName(), modulesRootNode);
 
         // create children under "/packages"
         List<Node> packagesChildren = new ArrayList<>(packageToModules.size());
@@ -285,7 +288,7 @@ private void initNodes() throws IOException {
             List<String> moduleNameList = entry.getValue();
             List<Node> moduleLinkNodes = new ArrayList<>(moduleNameList.size());
             for (String moduleName : moduleNameList) {
-                Node moduleNode = findModulesNode(MODULES + moduleName);
+                Node moduleNode = Objects.requireNonNull(nodes.get(MODULES + moduleName));
                 PathNode linkNode = new PathNode(PACKAGES + pkgName + "/" + moduleName, moduleNode);
                 nodes.put(linkNode.getName(), linkNode);
                 moduleLinkNodes.add(linkNode);
@@ -295,13 +298,13 @@ private void initNodes() throws IOException {
             packagesChildren.add(pkgDir);
         }
         // "/packages" dir
-        PathNode packagesDir = new PathNode("/packages", packagesChildren);
-        nodes.put(packagesDir.getName(), packagesDir);
+        PathNode packagesRootNode = new PathNode("/packages", packagesChildren);
+        nodes.put(packagesRootNode.getName(), packagesRootNode);
 
         // finally "/" dir!
         List<Node> rootChildren = new ArrayList<>();
-        rootChildren.add(packagesDir);
-        rootChildren.add(modulesDir);
+        rootChildren.add(packagesRootNode);
+        rootChildren.add(modulesRootNode);
         PathNode root = new PathNode("/", rootChildren);
         nodes.put(root.getName(), root);
     }

src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java

@@ -78,13 +78,13 @@ void close() throws IOException {
         return new ExplodedImage(explodedModulesDir);
     }
 
-    static final String RUNTIME_HOME;
+    private static final String RUNTIME_HOME;
     // "modules" jimage file Path
-    static final Path moduleImageFile;
+    private static final Path moduleImageFile;
     // "modules" jimage exists or not?
-    static final boolean modulesImageExists;
+    private static final boolean modulesImageExists;
     // <JAVA_HOME>/modules directory Path
-    static final Path explodedModulesDir;
+    private static final Path explodedModulesDir;
 
     static {
         PrivilegedAction<String> pa = SystemImage::findHome;

test/jdk/jdk/internal/jrtfs/whitebox/ExplodedImageTestDriver.java

@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @summary Whitebox tests for ExplodedImage to ensure compatibility with ImageReader.
+ * @modules java.base/jdk.internal.jrtfs java.base/jdk.internal.jimage
+ * @run junit/othervm java.base/jdk.internal.jrtfs.ExplodedImageTest
+ */
+public class ExplodedImageTestDriver {}

test/jdk/jdk/internal/jrtfs/whitebox/TEST.properties

@@ -0,0 +1,4 @@
+modules = \
+    java.base/jdk.internal.jimage \
+    java.base/jdk.internal.jrtfs
+bootclasspath.dirs=.