Skip to content

8365467: Fix multiple issues in ExplodedImage #26757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+344 −80
Open

8365467: Fix multiple issues in ExplodedImage #26757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 74 additions & 76 deletions src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@
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;
Expand All @@ -38,6 +36,7 @@
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;
Expand All @@ -56,16 +55,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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to make this class actually testable and avoid just using the static field from SystemImage.

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid unnecessary assumptions about file systems.

String str = modulesDir.getFileSystem().getSeparator();
separator = str.equals("/") ? null : str;
modulesDirAttrs = Files.readAttributes(modulesDir, BasicFileAttributes.class);
initNodes();
Expand Down Expand Up @@ -94,6 +92,11 @@ private final class PathNode extends Node {
this.children = children;
}

@Override
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discovered this method was missing during testing. I think this logic is what's needed here, but I would like someone to just double check.

public boolean isResource() {
return link == null && !getFileAttributes().isDirectory();
}

@Override
public boolean isDirectory() {
return children != null ||
Expand Down Expand Up @@ -126,7 +129,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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using static field.

String pName = MODULES + nativeSlashToFrontSlash(p.toString());
Node node = findNode(pName);
if (node != null) { // findNode may choose to hide certain files!
Expand All @@ -152,7 +155,7 @@ public long size() {
}

@Override
public void close() throws IOException {
public synchronized void close() throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nodes map is always accessed synchronised except here, so by synchronizing this we can stop using ConcurrentHashMap.

nodes.clear();
}

Expand All @@ -161,70 +164,68 @@ 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is simply wrong. The lookup of 'packages/java.lang/java.base/java/lang' should fail, since there is no node for it. It's the wrapping file system's job to test for symbolic links in the path and resolve this sort of thing.
Only /packages, /packages/xxx and /packages/xxx/yyy nodes need to exist in SystemImage.

// 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.
*/
Node createModulesNode(String name, Path path) {
assert !nodes.containsKey(name) : "Node must not already exist: " + name;
assert name.startsWith(MODULES) && name.length() > MODULES.length() : "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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silently ignoring IOException was both a risk, and a possible performance issue, since it was being used for normal code flow whenever a non-existent node was being asked for. Now, an exception here is unconditionally a problem, since the given path does exist.

}
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.
*/
Path underlyingModulesPath(String name) {
if (name.startsWith(MODULES) && name.length() > MODULES.length()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra length test avoids issues when "/modules/" is given, since that should be invalid but otherwise gets turned into a path to the parent dir.

Path path = modulesDir.resolve(frontSlashToNativeSlash(name.substring(MODULES.length())));
return Files.exists(path) ? path : null;
}
return null;
}
Expand All @@ -249,24 +250,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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens once per module, so it's where we create the root dirs (see below).

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);
}
});
}
Expand All @@ -275,8 +273,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()));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed because there's already "modulesDir" elsewhere.

nodes.put(modulesRootNode.getName(), modulesRootNode);

// create children under "/packages"
List<Node> packagesChildren = new ArrayList<>(packageToModules.size());
Expand All @@ -285,7 +283,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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to call code that "creates" the module directory node here, we did it above, and here we can just guarantee it's in the cache.

PathNode linkNode = new PathNode(PACKAGES + pkgName + "/" + moduleName, moduleNode);
nodes.put(linkNode.getName(), linkNode);
moduleLinkNodes.add(linkNode);
Expand All @@ -295,13 +293,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);
}
Expand Down
8 changes: 4 additions & 4 deletions src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,13 @@ void close() throws IOException {
return new ExplodedImage(explodedModulesDir);
}

static final String RUNTIME_HOME;
private static final String RUNTIME_HOME;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hiding these prevents unwanted use by other classes (which would make them effectively untestable).

// "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;
Expand Down
30 changes: 30 additions & 0 deletions test/jdk/jdk/internal/jrtfs/whitebox/ExplodedImageTestDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure if a class declaration is needed here. It seems to work fine, but I've seen a case where there wasn't one (but that felt odd). Happy to remove if not wanted.

4 changes: 4 additions & 0 deletions test/jdk/jdk/internal/jrtfs/whitebox/TEST.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
modules = \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cargo-culted without a true understand of what's going on. This seems to be what's needed for module access.

java.base/jdk.internal.jimage \
java.base/jdk.internal.jrtfs
bootclasspath.dirs=.
Loading