-
Notifications
You must be signed in to change notification settings - Fork 45
Add handling for possible null return to avoid NullPointerException #3391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Arthur Chan <[email protected]>
A Lines 71 to 76 in 455fd74
The method {
"type": null,
...
} In such a case, the call to null.equals("HvdcLineCriterionContingencyList") This results in a |
This is a stability issue due to a lack of validation for null checking from parsing of untrusted JSON. Here is a simple proof of concept to trigger the problem. import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.deser.DefaultDeserializationContext;
import com.powsybl.contingency.contingency.list.HvdcLineCriterionContingencyList;
import com.powsybl.contingency.json.HvdcLineCriterionContingencyListDeserializer;
import java.io.StringReader;
public class ProofOfConcept {
public static void main(String[] args) throws Exception {
String json = "{\"type\": null, \"name\": \"test-list\"}";
JsonFactory factory = new JsonFactory();
JsonParser parser = factory.createParser(new StringReader(json));
ObjectMapper mapper = new ObjectMapper();
DeserializationContext ctx = new DefaultDeserializationContext.Impl(
mapper.getDeserializationContext().getFactory()
);
HvdcLineCriterionContingencyListDeserializer deserializer = new HvdcLineCriterionCon>
parser.nextToken();
deserializer.deserialize(parser, ctx);
}
} To execute and test the PoC, follow the steps below. It is assumed that OpenJDK 17.0.2 and Maven 3.9.9 is used.
You will get the following exception stack trace.
|
The root cause is down at the AbstractEquipmentCriterionContingencyListDeserializer ::deserializeCommonAttributes method. The fix is simply adding a null checking before calling to the |
Signed-off-by: Arthur Chan <[email protected]>
@@ -69,7 +70,9 @@ protected boolean deserializeCommonAttributes(JsonParser parser, Deserialization | |||
return true; | |||
} | |||
case "type" -> { | |||
if (!parser.nextTextValue().equals(expectedType)) { | |||
// parser.nextTextValue() could returns null | |||
String typeStr = Objects.requireNonNull(parser.nextTextValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
Here, calling Objects.requireNonNull(...)
will also throw a NPE if the type is null (but the cause will indeed be different).
It may be better to reverse the test (!expectedType.equals(typeStr)
) because expectedType
is not an input. In this case, an IllegalStateException
with a clear message will be thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the prompt reply. I have fixed that by reversing the equals method. I have also add a null check for safety.
Signed-off-by: Arthur Chan <[email protected]>
This is a proposed fix to stability issue discovered by OSS-Fuzz when fuzzing the powsybl-core module. The original OSS-Fuzz issue can be found in https://issues.oss-fuzz.com/u/1/issues/406830033.