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

[Alan Bateman]

On Fri, 9 Jun 2023 15:47:54 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> 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 ...
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   newline at end of test file

src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 215:

> 213:         // this URLClassPath constructor is solely for the app classloader, for which we use
> 214:         // the system provided jar protocol handler implementation for loading jars
> 215:         // 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 says that the application class loader must use the built-in protocol handler.

src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 216:

> 214:         // the system provided jar protocol handler implementation for loading jars
> 215:         // in the classpath
> 216:         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.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/14395#discussion_r1224523768
PR Review Comment: https://git.openjdk.org/jdk/pull/14395#discussion_r1224527185