RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError

[Daniel Fuchs]

On Wed, 23 Aug 2023 15:41: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.

src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 230:

> 228:     // The accessor in which this logger is temporarily set.
> 229:     final LazyLoggerAccessor holder;
> 230:     final BooleanSupplier isLoadingThread;

Suggestion:

    // tests whether the logger is invoked by the loading thread before
    // the LoggerFinder is loaded; can be null;
    final BooleanSupplier isLoadingThread;

src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 234:

> 232:     boolean isLoadingThread() {
> 233:         return isLoadingThread != null && isLoadingThread.getAsBoolean();
> 234:     }

Suggestion:

    // returns true if the logger is invoked by the loading thread before the
    // LoggerFinder service is loaded
    boolean isLoadingThread() {
        return isLoadingThread != null && isLoadingThread.getAsBoolean();
    }

src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 948:

> 946:     //      and the LogManager has not been initialized yet.
> 947:     public static boolean useLazyLoggers() {
> 948:         if (!BootstrapLogger.isBooted() ||

Suggestion:

        // Note: avoid triggering the initialization of the DetectBackend class
        //   while holding the BotstrapLogger class monitor
        if (!BootstrapLogger.isBooted() ||

src/java.base/share/classes/jdk/internal/logger/LazyLoggers.java line 425:

> 423:      */
> 424:     public static final Logger getLogger(String name, Module module) {
> 425:         BootstrapLogger.detectBackend();

Suggestion:

        // triggers detection of the backend
        BootstrapLogger.detectBackend();

src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 76:

> 74: 
> 75:     // Return the loaded LoggerFinder, or load it if not already loaded.
> 76:     static volatile Thread loadingThread;

Suggestion:

    // record the loadingThread while loading the backend
    static volatile Thread loadingThread;
    // Return the loaded LoggerFinder, or load it if not already loaded.

src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 83:

> 81:             Thread currentThread = Thread.currentThread();
> 82:             if (loadingThread == currentThread) {
> 83:                 return new TemporaryLoggerFinder();

Suggestion:

                // recursive ttempt to load the backend while loading the backend
                // use a temporary logger finder that returns special BootsrtapLoggers
                // which will wait until loading is finished
                return new TemporaryLoggerFinder();

src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 98:

> 96:     }
> 97: 
> 98:     static boolean isLoadingThread() {

Suggestion:

    // returns true if called by the thread that loads the LoggerFinder, while
    // loading the LoggerFinder. 
    static boolean isLoadingThread() {

src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 140:

> 138:     }
> 139: 
> 140:     static class TemporaryLoggerFinder extends LoggerFinder {

Suggestion:

    private static final class TemporaryLoggerFinder extends LoggerFinder {

src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 145:

> 143:                     @Override
> 144:                     public Logger apply(String name, Module module) {
> 145:                         return LazyLoggers.getLoggerFromFinder(name, module);

A better idea might be to:
Suggestion:

                        return LazyLoggers.getLogger(name, module);

src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 154:

> 152:             }
> 153:         };
> 154: 

Suggestion:

        private static final TemporaryLoggerFinder INSTANCE = new TemporaryLoggerFinder();

test/jdk/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.java line 161:

> 159:             Logger testLogger = Logger.getLogger("jdk.event.security");
> 160:             assertEquals(testLogger.getClass().getName(), "java.util.logging.Logger");
> 161:             testComplete = true;

I believe these lines should be after `Security.setProperty("test_2", "test");`, to make sure the logger is loaded by the module that uses it. Also I was expecting some kind of checks to verify that the expected messages are indeed logged.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303306422
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303310327
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303314153
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303315890
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303317596
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303320330
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303323768
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303327924
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303324768
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303326813
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303342480