[WFCORE-5850] Null pointers should not be dereferenced (controller) #5012
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
deps-ok
|
Core - Full Integration Build 11326 outcome was FAILURE using a merge of 08bcff3 Failed tests |
|
Core - Full Integration Build 11329 outcome was FAILURE using a merge of 08bcff3 Failed tests |
yersan
reviewed
Mar 23, 2022
Comment on lines
358
to
362
| OperationTransformerRegistry operationTransformerRegistry = get(value); | ||
| if (operationTransformerRegistry != null) { | ||
| operationTransformerRegistry.registerTransformer(iterator, operationName, entry); | ||
| } |
There was a problem hiding this comment.
I've reviewed the changes but I don't feel capable to approve them as they are. In general, they look harmless; we can argue that if the CI is passing, then we are fine, but there are possibilities for introducing an unexpected bug or a side effect.
I think we should be more cautious with those changes.
Specifically, this one, if operationTransformerRegistry is null the old behavior was to get a null pointer exception, but now the method is silently invoked without registering the expected transformer and that can be a wrong result difficult to track and debug on a mixed domain scenario. Maybe it could be better to keep the NPE or throw an IAE if it is considered an invalid state. This argument can be also extrapolated to YamlConfigurationExtension.java, OperationValidator.java, in which case leaving the code as it is now is also a valid alternative.
There was a problem hiding this comment.
@yersan thanks for your feedback. I would provide an explicit exception here
if (operationTransformerRegistry == null) {
throw new IllegalStateException("No OperationTransformerRegistry found for " + value); //implemented in I18N style as logger
}
For the other ones, YamlConfigurationExtension andOperationValidator I'm going to ask Emmanuel as original author. All if/else avoid a null pointer, except the ones I have changed.
boris-unckel
commented
Mar 23, 2022
| @@ -498,7 +498,9 @@ public void processOperation(ImmutableManagementResourceRegistration rootRegistr | |||
| if (map != null && map.containsKey("index")) { | |||
| op.get("index").set((Integer) map.get("index")); | |||
| } | |||
| op.get(VALUE).set(processObjectAttribute((ObjectTypeAttributeDefinition) type, map)); | |||
| if (map != null) { | |||
| op.get(VALUE).set(processObjectAttribute((ObjectTypeAttributeDefinition) type, map)); | |||
There was a problem hiding this comment.
@ehsavoie Could you please have a look at this fix? All other calls prevent a null pointer deref, except this one. Is it intended to break here?
There was a problem hiding this comment.
I think the null check used to be before the check from the index initially
There was a problem hiding this comment.
@ehsavoie I'm still in doubt, are you fine with this change then?
boris-unckel
commented
Mar 23, 2022
| @@ -130,6 +130,7 @@ public void validateOperation(final ModelNode operation) { | |||
| OperationEntry entry = root.getOperationEntry(address, name); | |||
| if (entry == null) { | |||
| throwOrWarnAboutDescriptorProblem(ControllerLogger.ROOT_LOGGER.noOperationEntry(name, address)); | |||
| return; // in case of warning no further processing possible (entry causes NullPointer) | |||
There was a problem hiding this comment.
@kabir Could you have a look at this fix, please? Many other points in the code have the pattern
throwOrWarnAboutDescriptorProblem(ControllerLogger....);
return;
Here the return; is missing. Is it intended to break later with an NPE in case of logging a warning?
There was a problem hiding this comment.
@boris-unckel I think it looks fine. A lot of this is cruft from ancient times before we became a lot stricter about needing definitions for everything
boris-unckel
force-pushed
the
WFCORE-5850_null_deref_ctrl
branch
from
08bcff3
to
abebf08
Compare
|
Core - Full Integration Build 11493 outcome was FAILURE using a merge of abebf08 Failed tests |
|
@yersan The changes requested are present or clarified with the original authors. Test failures seem unrelated. Could you have a final look at this, please? |
yersan
reviewed
Mar 29, 2022
| @@ -1028,7 +1029,7 @@ public Resource createResource(PathAddress relativeAddress) { | |||
| } | |||
|
|
|||
| private Resource createResourceInternal(PathAddress relativeAddress, int index) throws UnsupportedOperationException { | |||
| ImmutableManagementResourceRegistration current = getResourceRegistration(); | |||
| ImmutableManagementResourceRegistration current = checkNotNullParamWithNullPointerException("getResourceRegistration", getResourceRegistration()); | |||
There was a problem hiding this comment.
Sorry for the back-and-forth @boris-unckel , I missed this one; the purpose of checkNotNullParamWithNullPointerException is to check method params, so I would say we should not use it for any kind of situation. If you still want to avoid deferring the NPE here, we could use a specific logger entry for this one.
There was a problem hiding this comment.
@yersan NP, thanks for the additional feedback. I have changed accordingly.
| @@ -498,7 +498,9 @@ public void processOperation(ImmutableManagementResourceRegistration rootRegistr | |||
| if (map != null && map.containsKey("index")) { | |||
| op.get("index").set((Integer) map.get("index")); | |||
| } | |||
| op.get(VALUE).set(processObjectAttribute((ObjectTypeAttributeDefinition) type, map)); | |||
| if (map != null) { | |||
| op.get(VALUE).set(processObjectAttribute((ObjectTypeAttributeDefinition) type, map)); | |||
There was a problem hiding this comment.
@ehsavoie I'm still in doubt, are you fine with this change then?
boris-unckel
force-pushed
the
WFCORE-5850_null_deref_ctrl
branch
from
abebf08
to
dc59a29
Compare
|
Core - Full Integration Build 11373 outcome was FAILURE using a merge of dc59a29 Failed tests |
|
Core - Full Integration Build 11505 outcome was FAILURE using a merge of dc59a29 Failed tests |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
|
There has been no activity on this PR for 90 days and it has been closed automatically. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes https://issues.redhat.com/browse/WFCORE-5850