8308184: Launching java with large number of jars in classpath with java.protocol.handler.pkgs system property set can lead to StackOverflowError #14395

jaikiran · 2023-06-09T14:55:50Z

Can I please get a review of this change which proposes to fix the issue noted in https://bugs.openjdk.org/browse/JDK-8308184?

When an application is launched, the app classloader internally uses a jdk.internal.loader.URLClassPath to find and load the main class being launched. The URLClassPath uses a list of classpath entries to find resources. Depending on the classpath entry, the URLClassPath will use a relevant loader. A couple of such loaders are URLClassPath$FileLoader (for loading resources from directories) and URLClassPath$JarLoader (for loading resources from jar files).

JarLoader creates instances of java.net.URL to represent the jar file being loaded. java.net.URL uses protocol specific java.net.URLStreamHandler instance to handle connections to those URLs. When constructing an instance of URL, callers can pass a protocol handler. If it is not passed then the URL class looks for protocol handlers that might have been configured by the application. The java.protocol.handler.pkgs system property is the one which allows overriding the protocol handlers (even for the jar protocol). When this property is set, the URL class triggers lookup and classloading of the protocol handler classes.

The issue that is reported is triggered when the java.protocol.handler.pkgs system property is set and the classpath has too many jar files. app classloader triggers lookup of the main class and the URLClassPath picks up the first entry in the classpath and uses a JarLoader (in this example our classpath entries have a jar file at the beginning of the list). The JarLoader instantiates a java.net.URL, which notices that the java.protocol.handler.pkgs is set, so it now triggers lookup of a (different) class using the same classloader and thus the same URLClassPath. The URLClassPath picks the next classpath entry and then calls into the URL again through the JarLoader. This sequence ends up being re-entrant calls and given the large number of classpath entries, these re-entrant calls end up with a StackOverflowError as shown in the linked JBS issue.

The commit in this PR fixes this issue by using the system provided protocol handler implementation of the jar protocol in the app classloader. This results in the URL instances created through the JarLoader to use this specific handler instance. This allows the app classloader which is responsible for loading the application's main class to reliably use the system provided protocol handler.

Do note that this doesn't in any way impact the ability of applications to override the protocol handler for jar protocol. Applications can still continue to do it by setting the java.protocol.handler.pkgs system property to configure their application specific implementation for the jar protocol (or other protocols). URL instances created by the application code will continue to use their overridden jar protocol handler. The commit in this PR merely fixes the jar protocol handler that is used by the app classloader.

A new jtreg test has been added which reproduces this issue and verifies the fix. tier testing is in progress.

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-8308184: Launching java with large number of jars in classpath with java.protocol.handler.pkgs system property set can lead to StackOverflowError (Bug - P4)

Reviewers

Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to b1260557
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14395

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

Using diff file

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

Webrev

Link to Webrev Comment

…ava.protocol.handler.pkgs system property set can lead to StackOverflowError

bridgekeeper · 2023-06-09T14:56:44Z

👋 Welcome back jpai! 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 · 2023-06-09T14:58:54Z

@jaikiran The following labels will be automatically applied to this pull request:

core-libs
net

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

AlanBateman · 2023-06-09T15:01:47Z

src/java.base/share/classes/jdk/internal/loader/URLClassPath.java

     * @param skipEmptyElements indicates if empty elements are ignored or
     *        treated as the current working directory
     *
     * @apiNote Used to create the application class path.
     */
-    URLClassPath(String cp, boolean skipEmptyElements) {
+    URLClassPath(String cp, URLStreamHandler jarHandler, boolean skipEmptyElements) {


This is a special constructor for the application class path so shouldn't have the jarHandler parameter. This should allow you to drop the changes to ClassLoaders.

Thank you Alan for the suggestion. I've updated the PR accordingly. The test continues to pass.

mlbridge · 2023-06-09T15:06:02Z

Webrevs

…to the URLClassPath

AlanBateman · 2023-06-09T16:39:12Z

src/java.base/share/classes/jdk/internal/loader/URLClassPath.java

-        this.jarHandler = null;
+        // this URLClassPath constructor is solely for the app classloader, for which we use
+        // the system provided jar protocol handler implementation for loading jars
+        // in the classpath


The apiNote on the constructor makes it clear that the constructor is for the application class path. Also just to say that the term used in the URL docs is "built-in protocol handler". Combined then it might be clearer if the comment at L213 says that the application class loader uses the built-in protocol handler to avoid protocol handler lookup when opening JAR files on the class path.

AlanBateman · 2023-06-09T16:42:21Z

src/java.base/share/classes/jdk/internal/loader/URLClassPath.java

+        // this URLClassPath constructor is solely for the app classloader, for which we use
+        // the system provided jar protocol handler implementation for loading jars
+        // in the classpath
+        this.jarHandler = new Handler();


I wouldn't normally suggest dropping an import and using a qualified class name but "Handler" is a very generic class name, replacing it with "new sun.net.www.protocol.jar.Handler()" might be clearer. Another approach would be to move the registry/lookup of the built-in protocol handler from URL to a support class so it could be used in several places but that that might be more trouble that it is worth so what you have is okay.

I wouldn't normally suggest dropping an import and using a qualified class name but "Handler" is a very generic class name, replacing it with "new sun.net.www.protocol.jar.Handler()" might be clearer.

I agree. I have now updated the PR to use the fully qualified name at the call site and also adjusted the code comment as recommended.

mlchung

Using the built-in jar protocol handler looks right. I like Alan's suggestion to use the fully qualified name in the source to make it clearer.

mlchung · 2023-06-09T17:40:58Z

test/jdk/java/net/URL/HandlersPkgPrefix/LargeClasspathWithPkgPrefix.java

+
+    // javac -d <destDir> <javaFile>
+    private static void compile(Path javaFile, Path destDir) throws Exception {
+        String javacPath = JDKToolFinder.getJDKTool("javac");


FYI. jdk.test.lib.compiler.CompilerUtils can be used to compile classes in process.

Thank you Mandy, this is useful, I wasn't aware of this one. I have now updated the test to use this utility class.

openjdk · 2023-06-09T17:47:32Z

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

8308184: Launching java with large number of jars in classpath with java.protocol.handler.pkgs system property set can lead to StackOverflowError

Reviewed-by: mchung, alanb

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

b94b679: 8309627: Incorrect sorting of DirtyCardQueue buffers
aace3dc: 8309760: ProblemList serviceability/jvmti/vthread/FollowReferences/VThreadStackRefTest.java#default with ZGC
80edd5c: 8294985: SSLEngine throws IAE during parsing of X500Principal
bdd81b3: 8304885: Reuse stale data to improve DNS resolver resiliency
beec734: 8309692: Fix -Wconversion warnings in javaClasses
7d82479: 8309142: Refactor test/langtools/tools/javac/versions/Versions.java
f5ec93e: 8309745: Problem list open client tests failing on Ubuntu_23.04
cee5724: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
7d6f97d: 8309673: Refactor ref_at methods in SA ConstantPool
7a970b2: 8309310: Update --release 21 symbol information for JDK 21 build 26
... and 3 more: https://git.openjdk.org/jdk/compare/a48bcf367120fc7cde88b19097dabe9c86c90bb7...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.

…he jars for debug purpose

AlanBateman · 2023-06-10T06:16:22Z

test/jdk/java/net/URL/HandlersPkgPrefix/LargeClasspathWithPkgPrefix.java

+ *        jdk.test.lib.process.ProcessTools
+ * @run driver LargeClasspathWithPkgPrefix
+ */
+public class LargeClasspathWithPkgPrefix {


test/jdk/java/net/URL/HandlersPkgPrefix is an unusual location for the test, maybe it could move to test/jdk/sun/misc/URLClassPath so it's the same location as the other tests for URLClassPath. Separately (not suggesting it for this PR) is that test directory should probably be renamed to jdk/internal/loader as the URLClassPath moved in JDK 9.

AlanBateman · 2023-06-10T06:16:37Z

src/java.base/share/classes/jdk/internal/loader/URLClassPath.java

-        this.jarHandler = null;
+        // the application class loader uses the built-in protocol handler to avoid protocol
+        // handler lookup when opening JAR files on the class path.
+        this.jarHandler = new sun.net.www.protocol.jar.Handler();


Good, this looks much better.

AlanBateman

Thanks for taking this one. My guess is that this issue goes back 20+ years but not noticed because it's running with the system property to specify another location for protocol handlers is probably rare, and it seems a scanning of a large large class path to trigger it.

I assume you'll see the comment about moving the test to be with the other tests for URLClassPath.

jaikiran · 2023-06-10T13:40:30Z

Hello Alan,

Thanks for taking this one. My guess is that this issue goes back 20+ years but not noticed because it's running with the system property to specify another location for protocol handlers is probably rare, and it seems a scanning of a large large class path to trigger it.

When I started looking into this issue, I was curious why this wasn't reported in Java 8 since the reporter was merely switching Java version from 8 to a newer version. When I look into Java 8 code, I see that the sun.misc.Launcher in Java 8 specifically deals with this case by using the built-in protocol handler and uses the bootstrap classloader for loading that hander, while launching the application's main class:
https://github.com/openjdk/jdk8u-dev/blob/master/jdk/src/share/classes/sun/misc/Launcher.java#L53
https://github.com/openjdk/jdk8u-dev/blob/master/jdk/src/share/classes/sun/misc/Launcher.java#L512

I think that explains why it isn't seen in that version.

jaikiran · 2023-06-10T13:42:52Z

I assume you'll see the comment about moving the test to be with the other tests for URLClassPath.

I have updated the PR to change the location of this new test class. Additionally I've filed https://bugs.openjdk.org/browse/JDK-8309763 to move the sun/misc/URLClassPath tests to jdk/internal/loader directory.

AlanBateman · 2023-06-10T18:58:27Z

When I started looking into this issue, I was curious why this wasn't reported in Java 8 since the reporter was merely switching Java version from 8 to a newer version. When I look into Java 8 code, I see that the sun.misc.Launcher in Java 8 specifically deals with this case by using the built-in protocol handler

Thanks for the digging into that, I was curious why this hasn't come up before now. I suppose part of it is that it's rare to run with this system property to override or add URL protocol handlers. There are a number of JDK 1.0/1.1 era APIs that used this approach for pluggability. In time it may be possible to deprecate this mechanism, newer code use services and URLStreamHandlerProvider.

Anyway, it's good that you've tracked this down, thanks for spending time on it.

jaikiran · 2023-06-12T09:44:23Z

Thank you Alan and Mandy for the reviews. tier1, tier2, tier3 testing came back fine. I'll go ahead and merge this.

jaikiran · 2023-06-12T09:44:52Z

/integrate

openjdk · 2023-06-12T09:45:10Z

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

4d47069: 4516654: Metalworks Demo: Window title not displayed fully in Low Vision Theme
408cadb: 8309467: Pattern dominance should be adjusted
6c3e621: 8308749: C2 failed: regular loops only (counted loop inside infinite loop)
f5cbe53: 8027711: Unify wildcarding syntax for CompileCommand and CompileOnly
4d66d97: 8309549: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java fails on AIX
3981297: 8309703: AIX build fails after JDK-8280982
16c3d53: 8308603: Removing do_pending_ref/enclosing_ref from MetaspaceClosure
b94b679: 8309627: Incorrect sorting of DirtyCardQueue buffers
aace3dc: 8309760: ProblemList serviceability/jvmti/vthread/FollowReferences/VThreadStackRefTest.java#default with ZGC
80edd5c: 8294985: SSLEngine throws IAE during parsing of X500Principal
... and 10 more: https://git.openjdk.org/jdk/compare/a48bcf367120fc7cde88b19097dabe9c86c90bb7...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-12T09:45:22Z

@jaikiran Pushed as commit 268ec61.

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

8308184: Launching java with large number of jars in classpath with j…

418dd36

…ava.protocol.handler.pkgs system property set can lead to StackOverflowError

openjdk bot added the rfr label Jun 9, 2023

openjdk bot added core-libs net labels Jun 9, 2023

AlanBateman reviewed Jun 9, 2023

View reviewed changes

jaikiran added 2 commits June 9, 2023 20:39

Alan's review suggestion - no need to pass jarHandler as a parameter …

2fdb567

…to the URLClassPath

newline at end of test file

b126055

AlanBateman reviewed Jun 9, 2023

View reviewed changes

mlchung approved these changes Jun 9, 2023

View reviewed changes

openjdk bot added the ready label Jun 9, 2023

jaikiran added 4 commits June 10, 2023 06:37

minor code comment change and use fully qualified classname at call site

f34f820

Mandy's suggestion - use CompilerUtils for in-process compilation

50abea6

minor comment adjustment on test method

39d7ea2

minor update to the test to allow tracking the time taken to create t…

362ef5c

…he jars for debug purpose

AlanBateman reviewed Jun 10, 2023

View reviewed changes

AlanBateman approved these changes Jun 10, 2023

View reviewed changes

move the test to test/jdk/sun/misc/URLClassPath directory

5a57fe8

AlanBateman approved these changes Jun 10, 2023

View reviewed changes

openjdk bot added the integrated label Jun 12, 2023

openjdk bot closed this Jun 12, 2023

openjdk bot removed ready rfr labels Jun 12, 2023

jaikiran deleted the 8308184 branch June 12, 2023 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8308184: Launching java with large number of jars in classpath with java.protocol.handler.pkgs system property set can lead to StackOverflowError #14395

8308184: Launching java with large number of jars in classpath with java.protocol.handler.pkgs system property set can lead to StackOverflowError #14395

jaikiran commented Jun 9, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 9, 2023

openjdk bot commented Jun 9, 2023

AlanBateman Jun 9, 2023 •

edited

Loading

jaikiran Jun 9, 2023

mlbridge bot commented Jun 9, 2023 •

edited

Loading

AlanBateman Jun 9, 2023 •

edited

Loading

AlanBateman Jun 9, 2023

jaikiran Jun 10, 2023

mlchung left a comment

mlchung Jun 9, 2023

jaikiran Jun 10, 2023

openjdk bot commented Jun 9, 2023 •

edited

Loading

AlanBateman Jun 10, 2023

AlanBateman Jun 10, 2023

AlanBateman left a comment •

edited

Loading

jaikiran commented Jun 10, 2023

jaikiran commented Jun 10, 2023

AlanBateman commented Jun 10, 2023

jaikiran commented Jun 12, 2023 •

edited

Loading

jaikiran commented Jun 12, 2023

openjdk bot commented Jun 12, 2023

openjdk bot commented Jun 12, 2023

8308184: Launching java with large number of jars in classpath with java.protocol.handler.pkgs system property set can lead to StackOverflowError #14395

8308184: Launching java with large number of jars in classpath with java.protocol.handler.pkgs system property set can lead to StackOverflowError #14395

Conversation

jaikiran commented Jun 9, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 9, 2023

openjdk bot commented Jun 9, 2023

AlanBateman Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

jaikiran Jun 9, 2023

Choose a reason for hiding this comment

mlbridge bot commented Jun 9, 2023 • edited Loading

Webrevs

AlanBateman Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

AlanBateman Jun 9, 2023

Choose a reason for hiding this comment

jaikiran Jun 10, 2023

Choose a reason for hiding this comment

mlchung left a comment

Choose a reason for hiding this comment

mlchung Jun 9, 2023

Choose a reason for hiding this comment

jaikiran Jun 10, 2023

Choose a reason for hiding this comment

openjdk bot commented Jun 9, 2023 • edited Loading

AlanBateman Jun 10, 2023

Choose a reason for hiding this comment

AlanBateman Jun 10, 2023

Choose a reason for hiding this comment

AlanBateman left a comment • edited Loading

Choose a reason for hiding this comment

jaikiran commented Jun 10, 2023

jaikiran commented Jun 10, 2023

AlanBateman commented Jun 10, 2023

jaikiran commented Jun 12, 2023 • edited Loading

jaikiran commented Jun 12, 2023

openjdk bot commented Jun 12, 2023

openjdk bot commented Jun 12, 2023

jaikiran commented Jun 9, 2023 •

edited by openjdk bot

Loading

AlanBateman Jun 9, 2023 •

edited

Loading

mlbridge bot commented Jun 9, 2023 •

edited

Loading

AlanBateman Jun 9, 2023 •

edited

Loading

openjdk bot commented Jun 9, 2023 •

edited

Loading

AlanBateman left a comment •

edited

Loading

jaikiran commented Jun 12, 2023 •

edited

Loading