MaxFreedomPollard opened a new pull request, #4420:
URL: https://github.com/apache/logging-log4j2/pull/4420
## What breaks
`Rfc5424Layout` merges a `StructuredDataMessage` into an element that
already uses the same SD-ID: the MDC element, a `LoggerFields` element, or an
earlier message of a `StructuredDataCollectionMessage`. That merge path throws
instead of merging:
```java
StructuredDataMessage first = new StructuredDataMessage("Transfer",
"Transfer Complete", "Audit");
first.put("Amount", "200.00");
StructuredDataMessage second = new StructuredDataMessage("Transfer", null,
"Audit");
second.put("ToAccount", "123456");
LogEvent event = Log4jLogEvent.newBuilder()
.setMessage(new StructuredDataCollectionMessage(Arrays.asList(first,
second)))
.build();
Rfc5424Layout.newBuilder().build().toSerializable(event);
// java.lang.NullPointerException
// at
org.apache.logging.log4j.core.layout.Rfc5424Layout.addStructuredData(Rfc5424Layout.java:446)
```
With `"Transfer@32473"` as the ID it throws `UnsupportedOperationException`
instead. A single `new StructuredDataMessage("mdc", ...)` logged with a
non-empty thread context also throws the `NullPointerException`, because its
SD-ID matches the default MDC element.
## Cause
Two problems in the merge path of
`log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java`:
1. `addStructuredData` checks `sdElements.containsKey(sdId)`, where `sdId`
comes from `getId(id)`, but then fetches the element with
`sdElements.get(id.toString())`. For an ID without an enterprise number,
`getId` appends the layout's enterprise number (`Transfer@32473`) and
`StructuredDataId.toString()` does not (`Transfer`), so the lookup returns
`null`.
2. An element created from a `StructuredDataMessage` keeps the map returned
by `MapMessage.getData()`, which is unmodifiable, so
`StructuredDataElement.union` fails on `putAll` when a second message with the
same SD-ID is merged into it.
## Fix
Fetch the element by `sdId`, and let `StructuredDataElement.union` copy the
fields into a new `HashMap` before adding the new ones. The copy only happens
when two elements are merged. The same code is on `main`.
## Testing
Two tests in `Rfc5424LayoutTest`:
- `testCollectionMergesElementsWithSameId`, parameterized with `Transfer`
and `Transfer@32473`, expects `[Transfer@32473 Amount="200.00"
ToAccount="123456"]`.
- `testMdcElementMergesStructuredDataWithSameId` expects `[mdc@32473
Amount="200.00" loginId="JohnDoe"]`.
I could not run Maven in my environment, so `./mvnw verify` (including
Spotless) and the real JUnit run have not been done. Instead I compiled
`Rfc5424Layout` and `Rfc5424LayoutTest` with `javac` (JDK 17) against the
`log4j-api`, `log4j-core` and `log4j-core-test` sources. JUnit and AssertJ were
replaced by small stand-ins, and the `Log4j2Plugins.dat` was generated with
`PluginProcessor`. I then ran every test method of `Rfc5424LayoutTest` through
a reflection-based runner. On `2.x` at 8d7d3c5d1c, 35 of 38 invocations pass,
and the 3 failures are the new ones (the two `NullPointerException`s and the
`UnsupportedOperationException` above). With the fix, all 38 pass.
## Checklist
* [x] Base your changes on `2.x` branch if you are targeting Log4j 2; use
`main` otherwise
* [ ] `./mvnw verify` succeeds ([the build
instructions](https://logging.apache.org/log4j/2.x/development.html#building)):
not run, see Testing
* [x] Non-trivial changes contain an entry file in the
`src/changelog/.2.x.x` directory
* [x] Tests are provided
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
