-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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.
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
8308184: Launching java with large number of jars in classpath with java.protocol.handler.pkgs system property set can lead to StackOverflowError #14395
Conversation
…ava.protocol.handler.pkgs system property set can lead to StackOverflowError
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
* @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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Alan for the suggestion. I've updated the PR accordingly. The test continues to pass.
Webrevs
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
// javac -d <destDir> <javaFile> | ||
private static void compile(Path javaFile, Path destDir) throws Exception { | ||
String javacPath = JDKToolFinder.getJDKTool("javac"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI. jdk.test.lib.compiler.CompilerUtils
can be used to compile classes in process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Mandy, this is useful, I wasn't aware of this one. I have now updated the test to use this utility class.
@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:
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
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 |
…he jars for debug purpose
* jdk.test.lib.process.ProcessTools | ||
* @run driver LargeClasspathWithPkgPrefix | ||
*/ | ||
public class LargeClasspathWithPkgPrefix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, this looks much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Hello Alan,
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 I think that explains why it isn't seen in that version. |
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 |
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. |
Thank you Alan and Mandy for the reviews. tier1, tier2, tier3 testing came back fine. I'll go ahead and merge this. |
/integrate |
Going to push as commit 268ec61.
Your commit was automatically rebased without conflicts. |
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 ajdk.internal.loader.URLClassPath
to find and load the main class being launched. TheURLClassPath
uses a list of classpath entries to find resources. Depending on the classpath entry, theURLClassPath
will use a relevant loader. A couple of such loaders areURLClassPath$FileLoader
(for loading resources from directories) andURLClassPath$JarLoader
(for loading resources from jar files).JarLoader
creates instances ofjava.net.URL
to represent the jar file being loaded.java.net.URL
uses protocol specificjava.net.URLStreamHandler
instance to handle connections to those URLs. When constructing an instance ofURL
, callers can pass a protocol handler. If it is not passed then theURL
class looks for protocol handlers that might have been configured by the application. Thejava.protocol.handler.pkgs
system property is the one which allows overriding the protocol handlers (even for thejar
protocol). When this property is set, theURL
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 theURLClassPath
picks up the first entry in the classpath and uses aJarLoader
(in this example our classpath entries have a jar file at the beginning of the list). TheJarLoader
instantiates ajava.net.URL
, which notices that thejava.protocol.handler.pkgs
is set, so it now triggers lookup of a (different) class using the same classloader and thus the sameURLClassPath
. TheURLClassPath
picks the next classpath entry and then calls into theURL
again through theJarLoader
. This sequence ends up being re-entrant calls and given the large number of classpath entries, these re-entrant calls end up with aStackOverflowError
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 theapp
classloader. This results in theURL
instances created through theJarLoader
to use this specific handler instance. This allows theapp
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 thejava.protocol.handler.pkgs
system property to configure their application specific implementation for thejar
protocol (or other protocols).URL
instances created by the application code will continue to use their overriddenjar
protocol handler. The commit in this PR merely fixes the jar protocol handler that is used by theapp
classloader.A new jtreg test has been added which reproduces this issue and verifies the fix. tier testing is in progress.
Progress
Issue
Reviewers
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