8329013: StackOverflowError when starting Apache Tomcat with signed jar #18534

coffeys · 2024-03-28T15:55:12Z

Calling extra logging calls during initialization of Logger libraries can cause recursion leading to StackOverflowError
This patch adds logic to the EventHelper class to detect recursion and prevent it.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8329013: StackOverflowError when starting Apache Tomcat with signed jar (Bug - P4)

Reviewers

Daniel Fuchs (@dfuch - Reviewer)
Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18534/head:pull/18534
$ git checkout pull/18534

Update a local copy of the PR:
$ git checkout pull/18534
$ git pull https://git.openjdk.org/jdk.git pull/18534/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18534

View PR using the GUI difftool:
$ git pr show -t 18534

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18534.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-28T15:56:00Z

👋 Welcome back coffeys! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-03-28T15:56:28Z

@coffeys This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8329013: StackOverflowError when starting Apache Tomcat with signed jar

Reviewed-by: dfuchs, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 169 new commits pushed to the master branch:

dd5d7d0: 8327002: (fs) java/nio/file/Files/CopyMoveVariations.java should be able to test across file systems
6ae1cf1: 8329368: Generational ZGC: Remove the unnecessary friend classes in ZAllocator
7eb78e3: 8320676: Manual printer tests have no Pass/Fail buttons, instructions close set 1
5ac067f: 8329289: Unify SetupJdkExecutable and SetupJdkLibrary
5ae849d: 8329292: Fix missing cleanups in java.management and jdk.management
ed821cb: 8324807: Manual printer tests have no Pass/Fail buttons, instructions close set 2
5cf457b: 8329320: Simplify awt/print/PageFormat/NullPaper.java test
8b934aa: 8329358: Generational ZGC: Remove the unused method ZPointer::set_remset_bits
3b582df: 8051959: Add thread and timestamp options to java.security.debug system property
816638e: 8329352: Remove dead code in splashscreen_sys.c
... and 159 more: https://git.openjdk.org/jdk/compare/d3f3011d56267360d65841da3550eca79cf1575b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-03-28T15:57:05Z

@coffeys The following labels will be automatically applied to this pull request:

core-libs
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-03-28T15:59:46Z

Webrevs

01: Full - Incremental (90c8467a)
00: Full (196634ec)

jaikiran · 2024-04-02T09:50:58Z

Hello Sean, from what I understand of this proposed fix, what we are attempting here is to prevent a re-entrant call into System.getLogger()? Is my understanding correct? The linked JBS issue has a stacktrace from Java 8 and doesn't match the current mainline code so it isn't fully clear to me what's triggering the re-entrancy and StackOverFlowError. I tried to reproduce this with the jtreg test that's part of this PR, without including your proposed fix, but that didn't reproduce the StackOverFlowError but the test seems to have uncovered some kind of deadlock in running the shutdown hook that the LogManager installs in the Runtime. I will check that issue separately.

coffeys · 2024-04-02T10:27:00Z

Thanks for taking a look at this Jai. Without the src patch, the test should create an SOE. Have a look at the resulting jtr file[1]. jtreg complains about a timeout due to some other lock up issue. I figured that since the test shouldn't fail, then it shouldn't be an issue. Probably worth logging another issue to track that.

The EventHelper class can be called to log crypto events - if these events occur during logging initialization, then recursive calls can cause problems. This patch should fix up that area and block recursive calls (by returning false in the 2nd recursive call into same method)

[1] (pasting a better SOE stacktrace (first one was partial removal of src patch)

at java.base/jdk.internal.logger.LazyLoggers.getLogger(LazyLoggers.java:435) at java.base/java.lang.System.getLogger(System.java:1831) at java.base/jdk.internal.event.EventHelper.isLoggingSecurity(EventHelper.java:166) at RecursiveEventHelper.getProperty(RecursiveEventHelper.java:56) at java.logging/java.util.logging.LogManager.getLevelProperty(LogManager.java:2340) at java.logging/java.util.logging.LogManager$LoggerContext.addLocalLogger(LogManager.java:802) at java.logging/java.util.logging.LogManager$LoggerContext.ensureDefaultLogger(LogManager.java:752) at java.logging/java.util.logging.LogManager$LoggerContext.ensureAllDefaultLoggers(LogManager.java:716) at java.logging/java.util.logging.LogManager$LoggerContext.addLocalLogger(LogManager.java:774) at java.logging/java.util.logging.LogManager$LoggerContext.addLocalLogger(LogManager.java:758) at java.logging/java.util.logging.LogManager$SystemLoggerContext.demandLogger(LogManager.java:939) at java.logging/java.util.logging.LogManager.demandSystemLogger(LogManager.java:559) at java.logging/java.util.logging.LogManager$LoggingProviderAccess.demandLoggerFor(LogManager.java:2727) at java.logging/sun.util.logging.internal.LoggingProviderImpl.demandJULLoggerFor(LoggingProviderImpl.java:412) at java.logging/sun.util.logging.internal.LoggingProviderImpl.demandLoggerFor(LoggingProviderImpl.java:438) at java.base/jdk.internal.logger.DefaultLoggerFinder.getLogger(DefaultLoggerFinder.java:159) at java.base/jdk.internal.logger.LazyLoggers.getLoggerFromFinder(LazyLoggers.java:410) at java.base/jdk.internal.logger.LazyLoggers.getLazyLogger(LazyLoggers.java:465) at java.base/jdk.internal.logger.LazyLoggers.getLogger(LazyLoggers.java:435) at java.base/java.lang.System.getLogger(System.java:1831) at java.base/jdk.internal.event.EventHelper.isLoggingSecurity(EventHelper.java:166) STATUS:Failed.main' threw exception: java.lang.StackOverflowError
`

jaikiran · 2024-04-02T11:16:55Z

You are right, turns out the jtr file did have the StackOverFlowError from the test introduced in this PR - I just didn't check it given the other issue.

Now that I look at the stacktrace and the complex checks we are adding, I am starting to wonder if these re-entrancy checks should actually be added within the jdk.internal.logger implementation code/classes, instead of at these call sites. As far as I can see, the jdk.internal.event.EventHelper isn't doing anything out of the ordinary and the same issue could probably end up in some other parts during the logging initialization.

dfuch

The code changes LGTM. Some comments on the test.

dfuch · 2024-04-02T13:34:42Z

test/jdk/jdk/security/logging/RecursiveEventHelper.java

+        EventHelper.isLoggingSecurity();
+        return super.getProperty(p);
+    }
+}


add newline?

dfuch · 2024-04-02T13:38:00Z

test/jdk/jdk/security/logging/RecursiveEventHelper.java

+        // opening of a signed jar during System logger initialization triggered
+        // a recursive call (via EventHelper.isLoggingSecurity) back into
+        // logger API
+        EventHelper.isLoggingSecurity();


As an additional check you could set a static volatile boolean to true here and double check that it's been set at the end of the main method,

In fact it doesn't need to be volatile since it all happens in the main thread - but that's not a big issue.

coffeys · 2024-04-02T15:01:15Z

@jaikiran I did look at adding logic in the Logger libraries also to correct this issue. Significant work was done in that area last year via JDK-8314263

I guess the question is what sort of Logger and LoggerFinder would you return in cases where recursion is detected ? Extra issues might arise from the System.getLogger method being public and widely used. Additional issues might stem from the fact that Logger libraries can be plugged in and returned Logger might be stored as a static variable etc.

It seemed safer to catch the issue at the EventHelper level - a library that's confined to internal JDK use.

jaikiran · 2024-04-02T15:20:13Z

I guess the question is what sort of Logger and LoggerFinder would you return in cases where recursion is detected ? Extra issues might arise from the System.getLogger method being public and widely used. Additional issues might stem from the fact that Logger libraries can be plugged in and returned Logger might be stored as a static variable etc.

Agreed - a lot of questions will need to be answered before attempting any change in that code. I don't have answers to any of those though :)

Given that System.getLogger() is public API and from the looks of it, any application code can trigger this issue without a JDK internal class involvement (I'm guessing if you modify the test in this PR to replace the call to EventHelper.isLoggingSecurity(); with var foo = System.getLogger("bar"); the StackOverFlowError issue will reproduce?), I think the real issue will continue to exist after this change.

Having said that, I didn't mean to stop you from doing this change in EventHelper. My question was merely an observation. Your change in the current form looks OK to me.

coffeys · 2024-04-02T17:31:35Z

Thanks for the reviews. Yes - your suggestion would be another example of how recursion could occur with the System Logger Jai. I'll have a look to see if some sort of Noop Logger could be returned if recursion is detected at System Logger end.

/integrate

openjdk · 2024-04-02T17:32:33Z

Going to push as commit 925d829.
Since your change was applied there have been 169 commits pushed to the master branch:

dd5d7d0: 8327002: (fs) java/nio/file/Files/CopyMoveVariations.java should be able to test across file systems
6ae1cf1: 8329368: Generational ZGC: Remove the unnecessary friend classes in ZAllocator
7eb78e3: 8320676: Manual printer tests have no Pass/Fail buttons, instructions close set 1
5ac067f: 8329289: Unify SetupJdkExecutable and SetupJdkLibrary
5ae849d: 8329292: Fix missing cleanups in java.management and jdk.management
ed821cb: 8324807: Manual printer tests have no Pass/Fail buttons, instructions close set 2
5cf457b: 8329320: Simplify awt/print/PageFormat/NullPaper.java test
8b934aa: 8329358: Generational ZGC: Remove the unused method ZPointer::set_remset_bits
3b582df: 8051959: Add thread and timestamp options to java.security.debug system property
816638e: 8329352: Remove dead code in splashscreen_sys.c
... and 159 more: https://git.openjdk.org/jdk/compare/d3f3011d56267360d65841da3550eca79cf1575b...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-02T17:32:40Z

@coffeys Pushed as commit 925d829.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8329013

196634e

openjdk bot changed the title ~~8329013~~ 8329013: StackOverflowError when starting Apache Tomcat with signed jar Mar 28, 2024

openjdk bot added the rfr Pull request is ready for review label Mar 28, 2024

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Mar 28, 2024

dfuch reviewed Apr 2, 2024

View reviewed changes

incorporate testcase feedback from Daniel

90c8467

jaikiran approved these changes Apr 2, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 2, 2024

dfuch approved these changes Apr 2, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 2, 2024

openjdk bot closed this Apr 2, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8329013: StackOverflowError when starting Apache Tomcat with signed jar #18534

8329013: StackOverflowError when starting Apache Tomcat with signed jar #18534

coffeys commented Mar 28, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 28, 2024

openjdk bot commented Mar 28, 2024 •

edited

Loading

openjdk bot commented Mar 28, 2024

mlbridge bot commented Mar 28, 2024 •

edited

Loading

jaikiran commented Apr 2, 2024 •

edited

Loading

coffeys commented Apr 2, 2024 •

edited

Loading

jaikiran commented Apr 2, 2024 •

edited

Loading

dfuch left a comment

dfuch Apr 2, 2024

dfuch Apr 2, 2024

dfuch Apr 2, 2024

coffeys commented Apr 2, 2024

jaikiran commented Apr 2, 2024

coffeys commented Apr 2, 2024

openjdk bot commented Apr 2, 2024

openjdk bot commented Apr 2, 2024

8329013: StackOverflowError when starting Apache Tomcat with signed jar #18534

8329013: StackOverflowError when starting Apache Tomcat with signed jar #18534

Conversation

coffeys commented Mar 28, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 28, 2024

openjdk bot commented Mar 28, 2024 • edited Loading

openjdk bot commented Mar 28, 2024

mlbridge bot commented Mar 28, 2024 • edited Loading

Webrevs

jaikiran commented Apr 2, 2024 • edited Loading

coffeys commented Apr 2, 2024 • edited Loading

jaikiran commented Apr 2, 2024 • edited Loading

dfuch left a comment

Choose a reason for hiding this comment

dfuch Apr 2, 2024

Choose a reason for hiding this comment

dfuch Apr 2, 2024

Choose a reason for hiding this comment

dfuch Apr 2, 2024

Choose a reason for hiding this comment

coffeys commented Apr 2, 2024

jaikiran commented Apr 2, 2024

coffeys commented Apr 2, 2024

openjdk bot commented Apr 2, 2024

openjdk bot commented Apr 2, 2024

coffeys commented Mar 28, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 28, 2024 •

edited

Loading

mlbridge bot commented Mar 28, 2024 •

edited

Loading

jaikiran commented Apr 2, 2024 •

edited

Loading

coffeys commented Apr 2, 2024 •

edited

Loading

jaikiran commented Apr 2, 2024 •

edited

Loading