Skip to content

MustFlowConfiguration::allowInterproceduralFlow doesn't work for recursive functions #21240

@intrigus-lgtm

Description

@intrigus-lgtm

For the given test case and the given query, MustFlowConfiguration with allowInterproceduralFlow() { none() } still allows flow which I would consider interprocedural when working with recursive functions.
In essence, it allows "backwards" flow, where the source &nodes[node->left] flows to the node == NULL check.
This flow only works when considering the call, but that would be interprocedural IMHO.

This is because the check for interprocedural flow looks like this and for recursive functions, the enclosing callable of nodeFrom and nodeTo is of course the same:

config.allowInterproceduralFlow()
or
getEnclosingCallable(nodeFrom) = getEnclosingCallable(nodeTo)

I solved the problem by adding a barrier which blocks InitializeParameterInstruction instructions which solved my problem.
A more principled solution would probably be to change the step predicate and inform it about interprocedural flow, but I was afraid of breaking stuff and maybe it's also not good for performance to add another parameter to a cached predicate:

predicate step(Instruction nodeFrom, Instruction nodeTo) {
exists(Operand mid |
instructionToOperandStep(nodeFrom, mid) and
operandToInstructionStep(mid, nodeTo)
)
or
flowThroughCallable(nodeFrom, nodeTo)
}

Testcase
struct node
{
    int left;
    int right;
};
// Test address-of of with a recursive function
// nodes is an array of struct node
// left and right are indices into the array
// Function is called like: test_recursive_function(nodes, &nodes[0]);
void test_recursive_function(struct node *nodes, struct node *node)
{
    if (node == NULL)
        return;
    test_recursive_function(nodes, &nodes[node->left]);
}
Query
/**
 * @name Suspicious check on address-of expression
 * @description Taking the address of a valid object always produces a non-NULL
 *              pointer, so checking the result against NULL is either redundant
 *              or indicates a bug where a different check was intended.
 * @kind problem
 * @problem.severity warning
 * @precision high
 * @id asymmetric-research/suspicious-address-of-null-check
 * @tags reliability
 *       correctness
 *       readability
 */

import cpp
import semmle.code.cpp.ir.dataflow.MustFlow
import semmle.code.cpp.controlflow.Guards

private class MustFlowConfig extends MustFlowConfiguration {
  MustFlowConfig() { this = "SuspiciousAddressOfNullCheckMustFlow" }

  override predicate isSource(Instruction source) {
    source.getUnconvertedResultExpression() instanceof ValidAddressOfExpr
  }

  override predicate isSink(Operand sink) { isNullOrNonNullCheck(sink) }

  override predicate allowInterproceduralFlow() { none() }
  // override predicate isBarrier(Instruction instr) {
  //   instr instanceof InitializeParameterInstruction
  // }
}

/**
 * Technically, we'd ignore cases where we do something like &foo->first_field (where foo is a pointer)
 * this is because if foo is NULL, the expression could actually evaluate to NULL.
 * However, it doesn't seem likely that anyone would write code like that intentionally
 * and it's also undefined behavior to dereference a NULL pointer anyway.
 */
class ValidAddressOfExpr extends AddressOfExpr { }

predicate isNullOrNonNullCheck(Operand operand) {
  any(IRGuardCondition gc).comparesEq(operand, 0, _, _)
}

from MustFlowPathNode source, MustFlowPathNode sink, MustFlowConfig cfg
where
  cfg.hasFlowPath(source, sink) and
  not source.getInstruction().getUnconvertedResultExpression().isInMacroExpansion() and
  source.getInstruction().getEnclosingFunction() = sink.getInstruction().getEnclosingFunction()
select sink, "Taking the address of $@ always produces a non-NULL pointer, but it is checked $@.",
  source.getInstruction().getUnconvertedResultExpression().(ValidAddressOfExpr).getOperand(),
  "this operand", sink, "here"

Metadata

Metadata

Assignees

No one assigned

    Labels

    questionFurther information is requested

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions