RFR: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest [v2]

[Christian Stein]

On Mon, 26 Aug 2024 03:47:49 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-8338445?
>> 
>> The JDK internal class `jdk.internal.loader.URLClassPath` is used by classloader implementations in the JDK (for example by the `java.net.URLClassLoader` and the `jdk.internal.loader.ClassLoaders$AppClassLoader`). The implementation in `URLClassPath` constructs internal `Loader`s for each URL that is in the classpath of that instance. `Loader` implementations hold on to underlying resources from which the classpath resources are served by the `URLClassPath`.
>> 
>> When constructing a `Loader`, the `URLClassPath` allows the loader implementation to parse a local classpath for that specific `Loader`. For example, the `JarLoader` (an internal class of the JDK) will parse `Class-Path` attribute in the jar file's manifest to construct additional URLs that will be included in the classpath through which resources will be served by `URLClassPath`. While parsing the local classpath, if the `Loader` instance runs into any `IOException` or a `SecurityException`, the `URLClassPath` will ignore that specific instance of the `Loader` and will not hold on to it as a classpath element.
>> 
>> As noted earlier, `Loader` instances may hold onto underlying resources. When the `URLClassPath` ignores such `Loader`(s) and doesn't call `close()` on them, then that results in potential resource leaks. The linked JBS issue demonstrates a case where the `JarFile` held by the `JarLoader` doesn't get closed (until GC garbage collects the `JarLoader` and thus the `JarFile`), when the `URLClassPath` ignores that `JarLoader` due to an `IOException` when the `JarLoader` is parsing the `Class-Path` value from the jar's manifest.
>> 
>> The commit in this PR addresses the issue by calling `close()` on such `Loader`s that get rejected by the `URLClassPath` when either an `IOException` or a `SecurityException` gets thrown when constructing the `Loader` or parsing the local classpath.
>> 
>> A new jtreg test has been introduced which consistently reproduces this issue (on Windows) and verifies the fix. Additionally tier1, tier2 and tier3 testing has completed with this change without any failures.
>
> Jaikiran Pai has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - revert to old code comment
>  - use "JAR file" consistently in test's code comments
>  - rename closeLoaderIgnoreEx to closeQuietly

Simplify test data provision by replacing `MethodSource` with `ValueSource`.

> `@ValueSource` is one of the simplest possible sources. It lets you specify a single array of literal values and can only be used for providing a single argument per parameterized test invocation.

Find more details about "[Sources of Arguments](https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests-sources)" in JUnit's User-Guide.

test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 38:

> 36: import org.junit.jupiter.params.ParameterizedTest;
> 37: import org.junit.jupiter.params.provider.Arguments;
> 38: import org.junit.jupiter.params.provider.MethodSource;

Suggestion:

import org.junit.jupiter.params.provider.ValueSource;

test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 75:

> 73:                 Arguments.of("lib4.jar C:\\bar\\foo\\world/hello.jar")
> 74:         );
> 75:     }

Suggestion:

test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 84:

> 82:      */
> 83:     @ParameterizedTest
> 84:     @MethodSource("malformedClassPaths")

Suggestion:

    @ValueSource(strings={
        "C:\\foo\\bar\\hello/world.jar lib2.jar",
        "C:/hello/world/foo.jar",
        "lib4.jar C:\\bar\\foo\\world/hello.jar"
    })

test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 114:

> 112:                 Arguments.of("lib10.jar")
> 113:         );
> 114:     }

Suggestion:

test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 124:

> 122:      */
> 123:     @ParameterizedTest
> 124:     @MethodSource("missingButParsableClassPaths")

Suggestion:

    @ValueSource(strings={"/home/me/hello/world.jar lib9.jar", "lib10.jar"})

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

Changes requested by cstein (Committer).

PR Review: https://git.openjdk.org/jdk/pull/20691#pullrequestreview-2262297032
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732237021
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732236791
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732236240
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732237567
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732238441