fix: potential null dereference fix

In file: StepGraph.java, class: StepGraph, there is a method equals that there is a potential Null pointer dereference. This may throw an unexpected null pointer exception which, if unhandled, may crash the program.

Possible Null Dereference Path

in the implementation of the following class

@Override
    public boolean equals(final Object obj) {

        ...

        for (final Node node : this.steps) {
            final Node otherNode = findNode(node.getName(), other.steps);
            if (!node.getNextNodes().equals(otherNode.getNextNodes()) || !node.getPrevNodes().equals(otherNode.getPrevNodes())) {
                return false;
            }

        }

        return true;
    }

to initialize the value of otherNode findNode method was invoked and from the following implementation of findNode

private Node findNode(final String name, final Set<Node> nodes) {
        for (final Node node : nodes) {
            if (name.equals(node.getName())) {
                return node;
            }

        }

        return null;
    }

it is possible that value of otherNode can be null so it is safe to check if the value is null or not before method invocation.

Suggested fix

null check inside the if-statement

if (otherNode != null && (!node.getNextNodes().equals(otherNode.getNextNodes()) || !node.getPrevNodes().equals(otherNode.getPrevNodes()))) {
    return false;
}

The same logic can be applied for method hasCircle in file ActionCircleDetector.java

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

I think these changes are unnecessary and the suggested fixes are wrong/unfinished.

In the case of StepGraph, the findNode() never returns null, because before the point of possible null dereferencing we check whether the two step sets are equal or not (by comparing step names). If they are not equal, the method returns false before that point. If they are equals then the findNode() always finds the appropriate step. Even if this possible null dereferencing would be a problem, the suggested fix is not good. If the findNode() would return null, then the equals method should return false immediately.
The situation is similar in the case of ActionCircleDetector: the findAction() practically never returns null. This detector is called by the ChoreographerPlanValidator, which makes sure that for every referenced action name has an action defined. This check is performed before the circle detection. The suggested fix here (which is not needed) is not wrong (not causing any problem) but not finished. It handles the result of the first findAction() call, but not all the others in the loop.

If you agree with my analysis, please close this PR request.

eclipse-arrowhead / core-java-spring