8339460: CDS error when module is located in a directory with space in the name #20987

mkartashev · 2024-09-13T08:04:58Z

The crux of the problem is that path names in CDS are saved as URIs and subsequently "converted" back to path names simply by stripping off the prefix without appropriate conversion of the rest of the name. This commit adds such a conversion.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8339460: CDS error when module is located in a directory with space in the name (Bug - P4)

Reviewers

Calvin Cheung (@calvinccheung - Reviewer)
Ioi Lam (@iklam - Reviewer) Review applies to 9cba7ab0

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20987/head:pull/20987
$ git checkout pull/20987

Update a local copy of the PR:
$ git checkout pull/20987
$ git pull https://git.openjdk.org/jdk.git pull/20987/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20987

View PR using the GUI difftool:
$ git pr show -t 20987

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20987.diff

Webrev

Link to Webrev Comment

…n the name

bridgekeeper · 2024-09-13T08:05:30Z

👋 Welcome back mkartashev! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-09-13T08:06:40Z

@mkartashev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8339460: CDS error when module is located in a directory with space in the name

Reviewed-by: ccheung, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 265 new commits pushed to the master branch:

8c8f0d8: 8339260: Move rarely used constants out of ClassFile
47c1069: 8340812: LambdaForm customization via MethodHandle::updateForm is not thread safe
66f1639: 8339271: giflib attribution correction
8f75619: 8340864: Remove unused lines related to vmClasses
84751cb: 8340831: Simplify simple validation for class definition in MethodHandles.Lookup
df1959f: 8340838: Clean up MutableCallSite to use explicit release fence instead of AtomicInteger
1b2d40a: 8340956: ProblemList 4 java/nio/channels/DatagramChannel tests on macosx-all
f7bc9ba: 8340228: Open source couple more miscellaneous AWT tests
0e0b0b0: 8340684: Reading from an input stream backed by a closed ZipFile has no test coverage
81b5f09: 8340946: Add vmTestbase/gc/memory/Nio/Nio.java and java/nio/Buffer/LimitDirectMemory.java to problem list
... and 255 more: https://git.openjdk.org/jdk/compare/a35fd3861044bdb8ddae378cb666b3d2e549a8c8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@calvinccheung, @iklam) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-09-13T08:07:16Z

@mkartashev The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-09-13T08:10:19Z

Webrevs

dfuch · 2024-09-13T16:21:30Z

Hi, could you point at the place where the URI is written? Do we know what code produced the URI?

mkartashev · 2024-09-16T10:20:15Z

The URI is coming from the Java side. When a module is defined, its location is kept as a URI. See Modules.java:

Module defineModule(ClassLoader loader, ModuleDescriptor descriptor, URI uri);

Later on, the URI is converted into a path name, see ClassLoaderExt::process_module_table():

        path = ClassLoader::skip_uri_protocol(path);
        char* path_copy = NEW_RESOURCE_ARRAY(char, strlen(path) + 1);
        strcpy(path_copy, path);
        _module_paths->append(path_copy);

That path name, untouched, is passed to os::stat() to verify the file; see ClassLoader::setup_module_search_path():

  if (os::stat(path, &st) != 0) {
    tty->print_cr("os::stat error %d (%s). CDS dump aborted (path was \"%s\").",
      errno, os::errno_name(errno), path);
    vm_exit_during_initialization();
  }

At this point, if the original URI had any escaped characters, os::stat() would fail terminating the entire JVM.
There are other code paths that also had to be fixed.

calvinccheung

Thanks for fixing this bug. I've re-assigned the bug to you. Couple of comments below.

calvinccheung · 2024-09-23T22:46:41Z

src/hotspot/share/classfile/classLoader.cpp

+    if (str[index] == '%'
+            && isxdigit(str[index + 1])
+            && isxdigit(str[index + 2])) {


I think you need to bound check the index because the uri from ClassLoaderExt::process_module_table could just be a directory name, e.g. dir%, without the jar file name.

str is still zero-terminated, so if str[index] == '%' is true, then str[index + 1] at least exists and might be 0. If isxdigit(str[index + 1]) is true then str[index + 2] at least exists and might be 0, in which case isxdigit() is going to be false for it.

So if isxdigit(str[index + 1]) is false, it won't proceed to check isxdigit(str[index + 2])?
The bound check I'm thinking about is like the following:

if (index >= strlen(str) - 2) { return str[index]; }

So if isxdigit(str[index + 1]) is false, it won't proceed to check isxdigit(str[index + 2])? The bound check I'm thinking about is like the following:

That is exactly correct.

The bound check I'm thinking about is like the following:

if (index >= strlen(str) - 2) { return str[index]; }

This seems unnecessary as we already assume that the string is null-terminated in the caller, and, as I have demonstrated out earlier, null-terminated strings are quite safe to check character-by-character.

calvinccheung · 2024-09-23T22:53:22Z

test/hotspot/jtreg/runtime/cds/appcds/complexURI/ComplexURITest.java

+
+public class ComplexURITest {
+    final static String listFileName = "test-classlist.txt";
+    final static String archiveName  = "test-dynamic.jsa";


You're using the same archiveName for both static and dynamic test cases. I'd suggest declare a different archive name for the static case. E.g.

final static String staticArchiveName = "test-static.jsa"; final static String dynamicArchiveName = "test-dynamic.jsa";

Also, I think this test should be excluded from the hotspot_appcds_dynamic test group in the open/test/hotspot/jtreg/TEST.groups file.

Done. I also added the code to safeguard against IOException on Windows in the case of deleting the read-only archive created previously.

Test changes look good. I will run some testing on your patch.

iklam

Looks good. Some small nits.

iklam · 2024-09-25T00:49:45Z

src/hotspot/share/classfile/classLoader.cpp

+#ifdef _WINDOWS
+        if (decoded == '/') {
+            decoded = '\\';
+        }
+#endif


Is this necessary for this patch? If not, I think we should not include it, since it changes behavior (i.e., strcmp() on the path may start failing).

Is this necessary for this patch?

I suppose, not. Let me check that the test still passes on Windows, though.

Let me check that the test still passes on Windows, though.

The test does pass on Windows with all the recent changes.

iklam · 2024-09-25T00:50:13Z

src/hotspot/share/classfile/classLoader.cpp

+
+#ifdef _WINDOWS
+    if (uri[0] == '/') {
+        // Absolute path name on Windows does not begin with a slash


Please use 2-space indents.

Certainly. Done.

calvinccheung

I ran tiers 1 - 4 testing prior to this latest change and saw no new failures.

mkartashev · 2024-09-26T07:15:12Z

/integrate

openjdk · 2024-09-26T07:16:27Z

@mkartashev
Your change (at version 5fcaf02) is now ready to be sponsored by a Committer.

calvinccheung · 2024-09-26T15:39:48Z

/sponsor

openjdk · 2024-09-26T15:40:31Z

Going to push as commit aceae76.
Since your change was applied there have been 274 commits pushed to the master branch:

aeaa4f7: 8336895: BufferedReader doesn't read full \r\n line ending when it doesn't fit in buffer
376056c: 8336468: Reflection and MethodHandles should use more precise initializer checks
e36ce5f: 8336942: Improve test coverage for class loading elements with annotations of different retentions
95d3e9d: 8339560: Unaddressed comments during code review of JDK-8337664
47fcf5a: 8340687: Open source closed frame tests Azure Pipelines continuous integration (without running tests, for now) #1
777c20c: 8339542: compiler/codecache/CheckSegmentedCodeCache.java fails
3762ec3: 8340466: Add description for PassFailJFrame constructors
e2626db: 8340899: Remove wildcard bound in PositionWindows.positionTestWindows
10da2c2: 8340923: The class LogSelection copies uninitialized memory
8c8f0d8: 8339260: Move rarely used constants out of ClassFile
... and 264 more: https://git.openjdk.org/jdk/compare/a35fd3861044bdb8ddae378cb666b3d2e549a8c8...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-26T15:40:38Z

@calvinccheung @mkartashev Pushed as commit aceae76.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8339460: CDS error when module is located in a directory with space i…

1330cde

…n the name

openjdk bot added the rfr Pull request is ready for review label Sep 13, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 13, 2024

calvinccheung suggested changes Sep 23, 2024

View reviewed changes

Improved the test and excluded it from hotspot_appcds_dynamic

9cba7ab

iklam approved these changes Sep 25, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 25, 2024

Fixed formatting; stopped converting slash to backslash on Windows

5fcaf02

openjdk bot removed the ready Pull request is ready to be integrated label Sep 25, 2024

calvinccheung approved these changes Sep 25, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 25, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 26, 2024

openjdk bot added the integrated Pull request has been integrated label Sep 26, 2024

openjdk bot closed this Sep 26, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 26, 2024

8339460: CDS error when module is located in a directory with space in the name #20987

8339460: CDS error when module is located in a directory with space in the name #20987

Uh oh!

Conversation

mkartashev commented Sep 13, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Sep 13, 2024

Uh oh!

openjdk bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 13, 2024

Uh oh!

mlbridge bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dfuch commented Sep 13, 2024

Uh oh!

mkartashev commented Sep 16, 2024

Uh oh!

calvinccheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iklam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calvinccheung left a comment

Choose a reason for hiding this comment

Uh oh!

mkartashev commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

calvinccheung commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

Uh oh!

mkartashev commented Sep 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 13, 2024 •

edited

Loading

mlbridge bot commented Sep 13, 2024 •

edited

Loading