You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "azakordonets (via GitHub)" <gi...@apache.org> on 2023/03/26 08:30:57 UTC
[PR] fix: fix NPE in PluginElementVisitor findNamedNode (logging-log4j2)
azakordonets opened a new pull request, #1392:
URL: https://github.com/apache/logging-log4j2/pull/1392
If "childType" is null, then the "if" block just get's skipped and on line 104 nullpointer will be called for `childType.getElement` command.
This PR fixes the issue.
--
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: fix NPE in PluginElementVisitor findNamedNode (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1392:
URL: https://github.com/apache/logging-log4j2/pull/1392#issuecomment-1484802355
@azakordonets, thanks so much for the report and the fix! I would appreciate it if you can next time also attach a `src/changelog/.2.x.x/<ticketId>_<description>.xml` entry along your PR.
@jvz, I don't think that FIXME is relevant any more. `visit()` contains `namedNode = findNamedNode(...); ...; return namedNode.getObject()`and its return value is checked against `null` at every call site.
--
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: fix NPE in PluginElementVisitor findNamedNode (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1392:
URL: https://github.com/apache/logging-log4j2/pull/1392#issuecomment-1484830541
@azakordonets, if you have a test, I would be happy to merge that too. :heart_eyes:
--
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: fix NPE in PluginElementVisitor findNamedNode (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on PR #1392:
URL: https://github.com/apache/logging-log4j2/pull/1392#issuecomment-1484169271
What do you think about the adjacent "FIXME" comment? Do you think this would fix that, too?
--
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: fix NPE in PluginElementVisitor findNamedNode (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1392:
URL: https://github.com/apache/logging-log4j2/pull/1392#discussion_r1149028700
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/visitors/PluginElementVisitor.java:
##########
@@ -98,13 +98,12 @@ public Object visit(final Configuration configuration, final Node node, final Lo
private Node findNamedNode(final String name, final Iterable<Node> children) {
for (final Node child : children) {
final PluginType<?> childType = child.getType();
- if (childType == null) {
- //System.out.println();
- }
- if (name.equalsIgnoreCase(childType.getElementName()) ||
- this.conversionType.isAssignableFrom(childType.getPluginClass())) {
- // FIXME: check child.getObject() for null?
- // doing so would be more consistent with the array version
+ boolean elementNameMatch = childType != null && name.equalsIgnoreCase(childType.getElementName());
+ boolean isAssignableByPluginClass = childType != null &&
+ this.conversionType.isAssignableFrom(childType.getPluginClass());
+ // FIXME: check child.getObject() for null?
+ // doing so would be more consistent with the array version
Review Comment:
```suggestion
```
--
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: fix NPE in PluginElementVisitor findNamedNode (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy merged PR #1392:
URL: https://github.com/apache/logging-log4j2/pull/1392
--
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: fix NPE in PluginElementVisitor findNamedNode (logging-log4j2)
Posted by "azakordonets (via GitHub)" <gi...@apache.org>.
azakordonets commented on PR #1392:
URL: https://github.com/apache/logging-log4j2/pull/1392#issuecomment-1485676669
Hey @vy , sorry for not including changes into the `changes` file and the test. I saw it in the template, but I have not done that in the past, so I was going to ask for help to whoever would review this. Two questions:
1. How and in which format should I include changes to `src/changelog/.2.x.x/<ticketId>_<description>.xml` ? Any examples that I could use ?
2. I wanted to write a test, but I could not find any tests related in that module and I was not sure whether it's on purpose or not. If not, then I will add those somewhere this week
--
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: fix NPE in PluginElementVisitor findNamedNode (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1392:
URL: https://github.com/apache/logging-log4j2/pull/1392#issuecomment-1486642082
1. For changelog entry, just clone one of the XML files there. I already added one for you in bb0f38a50ffa12a86cef3db3096efa5351746357.
2. I had the impression you get this NPE from a particular configuration. If so, you can write a test that feeds this configuration and verify the successful bootstrap of the Log4j. (There are plenty of such tests in `log4j-core` for inspiration.)
--
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