RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v4]

[Daniel Fuchs]

On Mon, 28 Aug 2023 17:29:16 GMT, Sean Coffey <coffeys at openjdk.org> wrote:

>> Recursive initialization calls possible during loading of LoggerFinder service.  
>> 
>> This fix detects the recursive call and returns a temporary LoggerFinder that is backed by a lazy logger. Automated test case developed to simulate loading of an external LoggerFinder service while also having other threads poke System.getLogger during this framework initialization.
>
> Sean Coffey has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Tidy up SignedLoggerFinderTest.java
>  - Code contribution from Daniel included. New tests. Fix up extra service call issues

test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java line 58:

> 56:      * of this run). Running test in signed and unsigned jar mode for sanity coverage.
> 57:      * The current bug only manifests when jars are signed.
> 58:      */

Suggestion:

    /**
     * This test triggers recursion by calling `System.getLogger` in the class init
     * of a custom LoggerFinder. Without the fix, this is expected to throw 
     * java.lang.NoClassDefFoundError: Could not initialize class jdk.internal.logger.LoggerFinderLoader$ErrorPolicy
     * caused by: java.lang.StackOverflowError
     *
     */

test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java line 65:

> 63:         logs.stream().map(SimpleLogRecord::of).forEach(System.out::println);
> 64:         logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check);
> 65:         assertEquals(String.valueOf(logs.size()), String.valueOf(2));*/

Why is this code commented? Is it an oversight?

test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/RecursiveLoadingTest.java line 55:

> 53:      * of this run). Running test in signed and unsigned jar mode for sanity coverage.
> 54:      * The current bug only manifests when jars are signed.
> 55:      */

Suggestion:

    /**
     * This test triggers recursion by calling `System.getLogger` in the class init
     * of a custom LoggerFinder. Without the fix, this is expected to throw 
     * java.lang.NoClassDefFoundError: Could not initialize class jdk.internal.logger.LoggerFinderLoader$ErrorPolicy
     * caused by: java.lang.StackOverflowError
     *
     */

test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/RecursiveLoadingTest.java line 61:

> 59:         logs.stream().map(SimpleLogRecord::of).forEach(System.out::println);
> 60:         logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check);
> 61:         assertEquals(String.valueOf(logs.size()), String.valueOf(2));

See suggested changes to SimpleLoggerFinder
Suggestion:

        assertEquals(String.valueOf(logs.size()), String.valueOf(3));

test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java line 28:

> 26: import java.lang.*;
> 27: import java.util.*;
> 28: import java.util.concurrent.CopyOnWriteArrayList;

Suggestion:

import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.ConcurrentHashMap;

test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java line 49:

> 47:     }
> 48: 
> 49:     public static final CopyOnWriteArrayList<Object> LOGS = new CopyOnWriteArrayList<>();

Suggest to move the declaration before the static block above.

test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java line 50:

> 48: 
> 49:     public static final CopyOnWriteArrayList<Object> LOGS = new CopyOnWriteArrayList<>();
> 50: 

Suggestion:


    private final Map<String, SimpleLogger> loggers = new ConcurrentHashMap<>();

    public SimpleLoggerFinder() {
        System.getLogger("dummy")
                .log(System.Logger.Level.INFO,
                        "Logger finder service created");
    }

test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java line 53:

> 51:     @Override
> 52:     public System.Logger getLogger(String name, Module module) {
> 53:         return new SimpleLogger(name);

Suggestion:

        return loggers.computeIfAbsent(name, SimpleLogger::new);

We're now getting the "dummy" logger twice, and since its constructor has side effects on its underlying jul logger implementation, we don't want to create two loggers for the same name - as that would cause two identical SimpleHandler instances to be added to the same underlying logger, making the log message appear twice in the LOGS list.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307771237
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307725024
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307773997
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307773194
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307776466
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307774748
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307775886
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307777399