SebTardif commented on PR #4172:
URL: https://github.com/apache/logging-log4j2/pull/4172#issuecomment-5005555728
@ramanathan1504
> we'll need a unit test reproducing the NPE and changelog also.
>
> Also, could you check if the other parsers in this switch statement
(parseLogger, etc.) need the exact same null check? ... log a warning when
appender == null ... Can appender.getName() ever be null here?
Thanks for the review. Addressed in the latest commit:
1. **Unit test** `XmlConfigurationTest#testInvalidAppenderDoesNotNpe` with
`log4j1-invalid-appender.xml` (bogus appender class). Without the guard, parse
hits NPE on `appender.getName()` and the valid `list` appender never registers;
with the guard both the NPE is avoided and `list` loads.
2. **Changelog** `src/changelog/.2.x.x/fix_xml_parse_appender_npe.xml`.
3. **Sibling audit** of the same `parse()` switch:
- `parseCategory` / `parseRoot` are `void` and do not use a nullable
return (no equivalent NPE).
- Renderer / factory tags already only log warnings.
- Only `APPENDER_TAG` called `parseAppender(...).getName()` without a
null check.
4. **`LOGGER.warn`** when `parseAppender` returns null (includes name +
class from the element).
5. **Null name guard** before `appenderMap.put` / `addAppender`.
Spotless clean; local green:
`./mvnw -pl log4j-1.2-api test
-Dtest=XmlConfigurationTest#testInvalidAppenderDoesNotNpe`
--
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]
