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

[Alan Bateman]

On Sat, 10 Jun 2023 05:16:28 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:
> 
>   minor update to the test to allow tracking the time taken to create the jars for debug purpose

Thanks for taking this one. My guess is that this issue goes back 20+ 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.

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

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

Good, this looks much better.

test/jdk/java/net/URL/HandlersPkgPrefix/LargeClasspathWithPkgPrefix.java line 44:

> 42:  * @run driver LargeClasspathWithPkgPrefix
> 43:  */
> 44: 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.

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

Marked as reviewed by alanb (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14395#pullrequestreview-1473325189
PR Review Comment: https://git.openjdk.org/jdk/pull/14395#discussion_r1225140617
PR Review Comment: https://git.openjdk.org/jdk/pull/14395#discussion_r1225140504