You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "joepembe (via GitHub)" <gi...@apache.org> on 2025/09/18 23:13:51 UTC
[PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
joepembe opened a new pull request, #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934
The `ThrowableStackTraceRenderer` class can throw a `NullPointerException` if the suppressed exceptions associated with the `Throwable` it is rendering are being concurrently mutated. This happens because `ThrowableStackTraceRenderer` invokes `Throwable#getSuppressed()` twice: once in `ThrowableStackTraceRenderer.Context.Metadata#populateMetadata()`, and a second time in `ThrowableStackTraceRenderer#renderThrowable()`, ahead of invoking `ThrowableStackTraceRenderer#renderSuppressed()`. If a racing thread manages to add a new suppressed exception to the being-logged exception between these two invocations, then the `Map<Throwable, Context.Metadata>` constructed by `populateMetadata()` will contain no mapping for the newly-added suppression, and as a result the dereference performed on line 168 explodes.
Note: the unit test I am adding here requires the ability to mock `Throwable#getSuppressed()`. Since this method is `final`, I had to add a dependency on `mockito-inline`, which then required that I fix up some unrelated unit tests that had been relying on the lack of support for mocking `final` methods.
This fixes #3929
> [!IMPORTANT]
> Base your changes on `2.x` branch if you are targeting Log4j 2; use `main` otherwise.
## Checklist
Before we can review and merge your changes, please go through the checklist below. If you're still working on some items, feel free to submit your pull request as a draft—our CI will help guide you through the remaining steps.
### ✅ Required checks
- [X] **License**: I confirm that my changes are submitted under the [Apache License, Version 2.0](https://apache.org/licenses/LICENSE-2.0).
- [X] **Commit signatures**: All commits are signed and verifiable. (See [GitHub Docs on Commit Signature Verification](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification)).
- [X] **Code formatting**: The code is formatted according to the project’s style guide.
<details>
<summary>How to check and fix formatting</summary>
- To **check** formatting: `./mvnw spotless:check`
- To **fix** formatting: `./mvnw spotless:apply`
See [the build instructions](https://logging.apache.org/log4j/2.x/development.html#building) for details.
</details>
- [X] **Build & Test**: I verified that the project builds and all unit tests pass.
<details>
<summary>How to build the project</summary>
Run: `./mvnw verify`
See [the build instructions](https://logging.apache.org/log4j/2.x/development.html#building) for details.
</details>
### 🧪 Tests (select one)
- [X] I have added or updated tests to cover my changes.
- [ ] No additional tests are needed for this change.
### 📝 Changelog (select one)
- [X] I added a changelog entry in `src/changelog/.2.x.x`. (See [Changelog Entry File Guide](https://logging.apache.org/log4j/tools/log4j-changelog.html#changelog-entry-file)).
- [ ] This is a trivial change and does not require a changelog entry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy merged PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "joepembe (via GitHub)" <gi...@apache.org>.
joepembe commented on PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934#issuecomment-3354031841
> Thanks so much for the contribution! Would you mind also extending following tests to reproduce the issue, please?
>
> * `ExtendedThrowablePatternConverterTest`
> * `RootThrowablePatternConverterTest`
> * `ThrowablePatternConverterTest`
Instead of modifying these tests, I added a new unit test specifically for `ThrowableStackTraceRenderer`. Is that sufficient?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934#discussion_r2402495546
##########
log4j-core-test/pom.xml:
##########
@@ -347,6 +347,12 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <scope>test</scope>
+ </dependency>
+
Review Comment:
You're right, it was an oversight from my side. Formatting should take place as long as there're active threads tampering the suppressed exception array. The following should do the trick:
```diff
Index: log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/ThrowablePatternConverterTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/ThrowablePatternConverterTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/ThrowablePatternConverterTest.java
--- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/ThrowablePatternConverterTest.java (revision 0ba25694d606f4ff483ae2b8717d5773b6f99dbc)
+++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/ThrowablePatternConverterTest.java (date 1759507533630)
@@ -29,6 +29,7 @@
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.LockSupport;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
@@ -453,6 +454,7 @@
final CountDownLatch startLatch =
new CountDownLatch(threadCount + /* the main thread invoking the rendering: */ 1);
final int exceptionPerThreadCount = 100;
+ final AtomicInteger runningThreadCountRef = new AtomicInteger(threadCount);
// Schedule threads that will start adding suppressed exceptions with the start signal
for (int threadIndex = 0; threadIndex < threadCount; threadIndex++) {
@@ -472,6 +474,8 @@
} catch (InterruptedException ignored) {
// Restore the interrupt
Thread.currentThread().interrupt();
+ } finally {
+ runningThreadCountRef.decrementAndGet();
}
});
}
@@ -491,7 +495,7 @@
// Trigger the start latch and format the exception
startLatch.countDown();
startLatch.await();
- for (int i = 0; i < 1_000_000; i++) {
+ while (runningThreadCountRef.get() > 0) {
// Give some time slack to increase randomness
LockSupport.parkNanos(1);
patternFormatter.format(logEvent, buffer);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "joepembe (via GitHub)" <gi...@apache.org>.
joepembe commented on code in PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934#discussion_r2392968315
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManagerTest.java:
##########
Review Comment:
See other comment.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RandomRollingAppenderOnStartupTest.java:
##########
Review Comment:
See other comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934#discussion_r2401711710
##########
log4j-core-test/pom.xml:
##########
@@ -347,6 +347,12 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <scope>test</scope>
+ </dependency>
+
Review Comment:
I think we can test this anomaly without overriding `final` methods – see 0ba25694d, which also ensures this will be checked in all `{ExtendedThrowable,RootThrowable,Throwable}PatternConverterTest` classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934#discussion_r2381693640
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManagerTest.java:
##########
Review Comment:
AFAICT these changes are unrelated. If so, would you mind reverting them, please? If so, would you mind explaining the rationale, please?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/ThrowableStackTraceRenderer.java:
##########
@@ -268,9 +268,15 @@ static final class Metadata {
*/
final int stackLength;
- private Metadata(final int commonElementCount, final int stackLength) {
+ /**
+ * The suppressed exceptions attached to this {@link Throwable}
Review Comment:
```suggestion
* The suppressed exceptions attached to this {@link Throwable}.
* This needs to be captured separately since {@link Throwable#getSuppressed()} can change.
*
* @see <a href="https://github.com/apache/logging-log4j2/issues/3929">#3929</a>
* @see <a href="https://github.com/apache/logging-log4j2/pull/3934">#3934</a>
```
##########
log4j-core-test/pom.xml:
##########
@@ -347,6 +347,12 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <scope>test</scope>
+ </dependency>
+
Review Comment:
Why do we need this?
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseAppenderTest.java:
##########
Review Comment:
AFAICT these changes are unrelated. If so, would you mind reverting them, please? If so, would you mind explaining the rationale, please?
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RandomRollingAppenderOnStartupTest.java:
##########
Review Comment:
AFAICT these changes are unrelated. If so, would you mind reverting them, please? If so, would you mind explaining the rationale, please?
##########
src/changelog/2.25.1/3929_ThrowableStackTraceRenderer-npe.xml:
##########
@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns="https://logging.apache.org/xml/ns"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="
+ https://logging.apache.org/xml/ns
+ https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+ type="fixed">
+ <issue id="3929" link="https://github.com/apache/logging-log4j2/issues/3929"/>
Review Comment:
```suggestion
<issue id="3929" link="https://github.com/apache/logging-log4j2/issues/3929"/>
<issue id="3934" link="https://github.com/apache/logging-log4j2/pull/3934"/>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "joepembe (via GitHub)" <gi...@apache.org>.
joepembe commented on code in PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934#discussion_r2402552201
##########
log4j-core-test/pom.xml:
##########
@@ -347,6 +347,12 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <scope>test</scope>
+ </dependency>
+
Review Comment:
Done. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "joepembe (via GitHub)" <gi...@apache.org>.
joepembe commented on code in PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934#discussion_r2402441530
##########
log4j-core-test/pom.xml:
##########
@@ -347,6 +347,12 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <scope>test</scope>
+ </dependency>
+
Review Comment:
This test takes an extremely long time to execute. I'm running it locally on my macbook pro, and I estimate that it will take 31 minutes to finish executing all 1,000,000 iterations of the `patternFormatter.format(logEvent, buffer);` loop. Are you sure this is the direction you want to go?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "joepembe (via GitHub)" <gi...@apache.org>.
joepembe commented on code in PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934#discussion_r2392940649
##########
log4j-core-test/pom.xml:
##########
@@ -347,6 +347,12 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <scope>test</scope>
+ </dependency>
+
Review Comment:
Oops! I wrote a unit test for these changes, but somehow it did not make it into the commit that I pushed to my forked repository. Apologies - I haven't used github much. I'll put together another revision and send it your way.
Anyways, the unit test that I wrote (and failed to include in the original PR) depends on the ability to specify mocked behavior for `Throwable#getSuppressed()`. Since `Throwable#getSuppressed()` is `final`, vanilla mockito can't do this - hence I added `mockito-inline`.
This is also the reason why I had to touch a handful of otherwise-unrelated tests beneath `log4j-core-test/`. These tests were silently depending on mockito's _inability_ to mock `final` methods, so they required some slight tweaking to get them to pass in the presence of `mockito-inline`.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseAppenderTest.java:
##########
Review Comment:
See other comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: prevent ThrowableStackTraceRenderer from throwing NPEs [logging-log4j2]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3934:
URL: https://github.com/apache/logging-log4j2/pull/3934#issuecomment-3310723844
<table>
<tr>
<th>Job</th>
<th>Requested goals</th>
<th>Build Tool Version</th>
<th>Build Outcome</th>
<th>Build Scan®</th>
</tr>
<tr>
<td>build-macos-latest</td>
<td>clean install</td>
<td align='center'>3.9.8</td>
<td align='center'>:white_check_mark:</td>
<td><a href="https://develocity.apache.org/s/nqlpdrro7g6ec" rel="nofollow"><img src="https://img.shields.io/badge/Build%20Scan%C2%AE-PUBLISHED-06A0CE?logo=Gradle" alt="Build Scan PUBLISHED" /></a></td>
</tr>
<tr>
<td>build-ubuntu-latest</td>
<td>clean install</td>
<td align='center'>3.9.8</td>
<td align='center'>:white_check_mark:</td>
<td><a href="https://develocity.apache.org/s/gdeqv7jt2sgpg" rel="nofollow"><img src="https://img.shields.io/badge/Build%20Scan%C2%AE-PUBLISHED-06A0CE?logo=Gradle" alt="Build Scan PUBLISHED" /></a></td>
</tr>
<tr>
<td>build-windows-latest</td>
<td>clean install</td>
<td align='center'>3.9.8</td>
<td align='center'>:white_check_mark:</td>
<td><a href="https://develocity.apache.org/s/xxyh7npgtqv6w" rel="nofollow"><img src="https://img.shields.io/badge/Build%20Scan%C2%AE-PUBLISHED-06A0CE?logo=Gradle" alt="Build Scan PUBLISHED" /></a></td>
</tr>
</table>
###### Generated by gradle/develocity-actions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org