8338626: ClassLoaderExt::process_jar_manifest() should allow / separator on Windows #20924

calvinccheung · 2024-09-09T20:46:45Z

On Windows, we can use '/' as the file separator in the classpath but not in the Class-Path: attribute.
This patch is to enable the use of '/' as the file separator in the Class-Path:attribute on Windows.

Passed tiers 1 - 3 testing.

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-8338626: ClassLoaderExt::process_jar_manifest() should allow / separator on Windows (Bug - P4)

Reviewers

Matias Saavedra Silva (@matias9927 - Reviewer) Review applies to 335a0aae
Ioi Lam (@iklam - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20924

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

Using diff file

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

Webrev

Link to Webrev Comment

…rator on Windows

bridgekeeper · 2024-09-09T20:47:26Z

👋 Welcome back ccheung! 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-09T20:47:32Z

@calvinccheung 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:

8338626: ClassLoaderExt::process_jar_manifest() should allow / separator on Windows

Reviewed-by: iklam, dholmes, matsaave

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 223 new commits pushed to the master branch:

5e5942a: 8339794: Open source closed choice tests Azure Pipelines continuous integration (without running tests, for now) #1
81ff91e: 8339531: Improve performance of MemorySegment::mismatch
ab9b72c: 8329035: New Data Destination instructions support
7f1dae1: 8339874: Avoid duplicate checking of trailing slash in ZipFile.getZipEntry
4d65c3e: 8339876: Move constant symbol caches to Utf8EntryImpl
0765917: 8340011: Simplify jdk.internal.classfile.impl.EntryMap
1b17e0b: 8338747: hasIncubatorModules needs to be generated when module resolution required at startup
3c40afa: 8334165: Remove serialVersionUID compatibility logic from JMX
315abdf: 8339733: C2: some nodes can have incorrect control after do_range_check()
ac3f92b: 8339731: java.desktop/share/classes/javax/swing/text/html/default.css typo in margin settings
... and 213 more: https://git.openjdk.org/jdk/compare/a461369f16a2d92ab428d14c36dd69fa5942bbc5...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-09-09T20:48:06Z

@calvinccheung 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-09T20:52:16Z

Webrevs

matias9927

LGTM! Just one note of the test

iklam · 2024-09-09T21:48:34Z

test/hotspot/jtreg/runtime/cds/appcds/ClassPathAttr.java

@@ -82,6 +83,40 @@ static void testNormalOps() throws Exception {
            output.shouldMatch("checking shared classpath entry: .*cpattr2.jar");
            output.shouldMatch("checking shared classpath entry: .*cpattr3.jar");
          });
+
+      // Test handling of forward slash ('/') file separator for Class-Path attribute on Windows.
+      if (Platform.isWindows()) {


Does the new test work for other platforms as well? If so, I think we should remove this check.

It does. Since other platforms have '/' as the file separator, running it again would be wasting of time.

That makes sense.

AlanBateman · 2024-09-10T06:17:17Z

src/hotspot/share/classfile/classLoaderExt.cpp

+    if (dir_tail == nullptr) {
+      dir_tail = strrchr(dir_name, '/');
+    }
+#endif


The value of the Class-Path attribute is a sequence of relative URLs, not file paths, so I wouldn't expect to see any usage of os::file_separator() here, even on Unix systems. Some further work may be needed here to align with how the Class-Path attribute is treated by the application class loader.

Given these are required to be relative URLs [1] and given the path component of a URL only uses '/' as a separator [2], then it would seem the use of os::file_separator is wrong and it should just be hardcoded to '/' - and further it means the current code has never been correct on Windows. ??

[1] https://docs.oracle.com/en/java/javase/21/docs/specs/jar/jar.html#class-path-attribute
[2] https://en.wikipedia.org/wiki/URL

Consider java -cp dir1/A.jar ... where the manifiest of A.jar contains:
Class-Path: a.jar
The dir_name in the above corresponds to dir1/A.jar. The strrchr is trying to locate the separator between the directory and jar name. Later in the same function, it will combine the directory name (in this case dir1) with the names from each of the entry from the attribute and create a class path entry. In the example, it will create a class path entry of dir1/a.jar.
Before the patch, on Windows, it only works with '\' (the default file separator). e.g. java -cp dir1\A.jar ... .

Also, the modified function is only called during CDS dump time for setting up the shared class paths for the app class loader.

To me it sounds like the logic should be always using / to locate and then use file_seperator to rewrite to a classpath filesystem entry.

Just to repeat what I said previously, the value of Class-Path attribute is a sequence of relative URLs. The code in ClassLoaderExt::process_jar_manifest will work for some cases but in general, the appending used in this function can't be worked to combine a file path and URL path.

Alan, could you show us some cases where ClassLoaderExt::process_jar_manifest() will fail. IIUC, the failures won't be Windows-specific (i.e., orthogonal to the \ vs / issues in this bug) but rather will happen on all platforms.

Can you try examples like file:lib.jar, file:/d/lib.jar, /d/lib.jar for starters. Once you bring Windows file paths into the picture then it's a world of hurt. There is big other topic around escaping where Class-Path is completely broken and we haven't figure out how to fix these issues in a compatibility way.

Sorry, I didn't notice last few comments until after pushing the fix. I think if there's any other shortcoming of the function in question, it should be addressed via different bug/RFE.
After a casual search for Class-Path attribute, I found: https://docs.oracle.com/javase/8/docs/technotes/guides/jar/jar.html#classpath
It says the following:
Currently, the URLs must be relative to the code base of the JAR file for security reasons. Thus, remote optional packages will originate from the same code base as the application.

It says the following: Currently, the URLs must be relative to the code base of the JAR file for security reasons. Thus, remote optional packages will originate from the same code base as the application.

That's right but there are a lot of issues here that date back to JDK 1.2 when the feature was added. We also have a long standing issue that tooling and plugins in the eco system are putting all manner of values into the attribute value, which has constrained us from fixing some of the fundamental issues. The summary is that the resolving the value against the base URL in the code here isn't the same as that done by the application class loader.

iklam · 2024-09-11T07:36:48Z

src/hotspot/share/classfile/classLoaderExt.cpp

+#ifdef _WINDOWS
+    // On Windows, we also support forward slash as the file separator for Class-Path: attribute.
+    if (dir_tail == nullptr) {
+      dir_tail = strrchr(dir_name, '/');


There's still a bug in the current patch. If the input is

java -cp a\b/c.jar

The current patch gets "a" as the directory name, but it should get "a\b" instead.

I've pushed an updated fix to address this above. Also added a test.

dholmes-ora · 2024-09-12T00:38:44Z

src/hotspot/share/classfile/classLoaderExt.cpp

@@ -213,6 +213,15 @@ void ClassLoaderExt::process_jar_manifest(JavaThread* current, ClassPathEntry* e
    char sep = os::file_separator()[0];
    const char* dir_name = entry->name();
    const char* dir_tail = strrchr(dir_name, sep);
+#ifdef _WINDOWS
+    // On Windows, we also support forward slash as the file separator when locating entries in the Class-Path: attribute.


Suggested change

// On Windows, we also support forward slash as the file separator when locating entries in the Class-Path: attribute.

// On Windows, we also support forward slash as the file separator when locating entries in the classpath entry.

The forward slash is not being used in relation to any Class-path attribute here.

Updated the comment.
Also updated the test so it can be run more reliably.

dholmes-ora · 2024-09-12T06:11:40Z

test/hotspot/jtreg/runtime/cds/appcds/ClassPathAttr.java

@@ -82,6 +83,75 @@ static void testNormalOps() throws Exception {
            output.shouldMatch("checking shared classpath entry: .*cpattr2.jar");
            output.shouldMatch("checking shared classpath entry: .*cpattr3.jar");
          });
+
+      // Test handling of forward slash ('/') file separator for Class-Path attribute on Windows.


Again this comment is wrong/misleading - the / is in the classpath entry

Updated the comment.

dholmes-ora

Okay. Thanks

iklam

LGTM

calvinccheung · 2024-09-13T14:58:32Z

Thanks @matias9927, @iklam, @dholmes-ora, @AlanBateman for the review.

/integrate

openjdk · 2024-09-13T14:59:35Z

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

3e0da58: 8333843: Provide guidelines on MemorySegment to read strings with known lengths
3c4d15b: 8334301: Errors in jpackage man page
4d01178: 8339927: Man page update for deprecating jhsdb debugd for removal
bd44cf8: 8330302: strace004 can still fail
8a4ea09: 8336492: Regression in lambda serialization
358ff19: 8339727: Open source several AWT focus tests - series 1
0c36177: 8340089: Simplify SegmentBulkOperations::powerOfProperty
bacd046: 8321010: RISC-V: C2 RoundVF
5709c37: 8340081: Test java/foreign/TestLinker.java failed failed: missing permission java.lang.foreign.native.threshold.power.fill
b88ff9c: 8339849: Enumerate opto and C1 stubs, generate enums, names, fields and generator calls
... and 224 more: https://git.openjdk.org/jdk/compare/a461369f16a2d92ab428d14c36dd69fa5942bbc5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-13T14:59:42Z

@calvinccheung Pushed as commit 89ca89c.

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

8338626: CDS handling of JAR Class-Path attribute should allow / sepa…

335a0aa

…rator on Windows

calvinccheung marked this pull request as ready for review September 9, 2024 20:48

openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org rfr Pull request is ready for review labels Sep 9, 2024

matias9927 approved these changes Sep 9, 2024

View reviewed changes

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

iklam reviewed Sep 9, 2024

View reviewed changes

iklam approved these changes Sep 9, 2024

View reviewed changes

AlanBateman reviewed Sep 10, 2024

View reviewed changes

iklam suggested changes Sep 11, 2024

View reviewed changes

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

calvinccheung changed the title ~~8338626: CDS handling of JAR Class-Path attribute should allow / separator on Windows~~ 8338626: ClassLoaderExt::process_jar_manifest() should allow / separator on Windows Sep 12, 2024

@iklam comment

f0ecad8

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Sep 12, 2024

dholmes-ora suggested changes Sep 12, 2024

View reviewed changes

@dholmes-ora comment and test update

9d6cf3a

dholmes-ora reviewed Sep 12, 2024

View reviewed changes

update comment in the testcase

1cfbe56

dholmes-ora approved these changes Sep 13, 2024

View reviewed changes

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

iklam approved these changes Sep 13, 2024

View reviewed changes

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

openjdk bot closed this Sep 13, 2024

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

calvinccheung deleted the 8338626-cp-attr-Windows branch September 13, 2024 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8338626: ClassLoaderExt::process_jar_manifest() should allow / separator on Windows #20924

8338626: ClassLoaderExt::process_jar_manifest() should allow / separator on Windows #20924

calvinccheung commented Sep 9, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 9, 2024

openjdk bot commented Sep 9, 2024 •

edited

Loading

openjdk bot commented Sep 9, 2024

mlbridge bot commented Sep 9, 2024 •

edited

Loading

matias9927 left a comment

iklam Sep 9, 2024

calvinccheung Sep 9, 2024

iklam Sep 9, 2024

AlanBateman Sep 10, 2024 •

edited

Loading

dholmes-ora Sep 10, 2024

calvinccheung Sep 10, 2024

calvinccheung Sep 10, 2024

dholmes-ora Sep 11, 2024

AlanBateman Sep 13, 2024

iklam Sep 13, 2024

AlanBateman Sep 13, 2024

calvinccheung Sep 13, 2024

AlanBateman Sep 15, 2024 •

edited

Loading

iklam Sep 11, 2024

calvinccheung Sep 12, 2024

dholmes-ora Sep 12, 2024

calvinccheung Sep 12, 2024

dholmes-ora Sep 12, 2024

calvinccheung Sep 12, 2024

dholmes-ora left a comment

iklam left a comment

calvinccheung commented Sep 13, 2024

openjdk bot commented Sep 13, 2024

openjdk bot commented Sep 13, 2024

	// On Windows, we also support forward slash as the file separator when locating entries in the Class-Path: attribute.
	// On Windows, we also support forward slash as the file separator when locating entries in the classpath entry.

8338626: ClassLoaderExt::process_jar_manifest() should allow / separator on Windows #20924

8338626: ClassLoaderExt::process_jar_manifest() should allow / separator on Windows #20924

Conversation

calvinccheung commented Sep 9, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 9, 2024

openjdk bot commented Sep 9, 2024 • edited Loading

openjdk bot commented Sep 9, 2024

mlbridge bot commented Sep 9, 2024 • edited Loading

Webrevs

matias9927 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

iklam left a comment

Choose a reason for hiding this comment

calvinccheung commented Sep 13, 2024

openjdk bot commented Sep 13, 2024

openjdk bot commented Sep 13, 2024

calvinccheung commented Sep 9, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 9, 2024 •

edited

Loading

mlbridge bot commented Sep 9, 2024 •

edited

Loading

AlanBateman Sep 10, 2024 •

edited

Loading

AlanBateman Sep 15, 2024 •

edited

Loading