C++: Make the two must-flow queries use the new must-flow library

Author: MathiasVP

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql

@@ -18,157 +18,70 @@ import cpp
 // recomputing the IR.
 private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.ir.IR
-import semmle.code.cpp.ir.dataflow.DataFlow::DataFlow
+import semmle.code.cpp.ir.dataflow.MustFlow
+import PathGraph
 
 /** Holds if `f` has a name that we intrepret as evidence of intentionally returning the value of the stack pointer. */
 predicate intentionallyReturnsStackPointer(Function f) {
   f.getName().toLowerCase().matches(["%stack%", "%sp%"])
 }
 
-/**
- * Holds if `source` is a node that represents the use of a stack variable
- */
-predicate isSource(Node source) {
-  exists(VariableAddressInstruction var, Function func |
-    var = source.asInstruction() and
-    func = var.getEnclosingFunction() and
-    var.getASTVariable() instanceof StackVariable and
-    // Pointer-to-member types aren't properly handled in the dbscheme.
-    not var.getResultType() instanceof PointerToMemberType and
-    // Rule out FPs caused by extraction errors.
-    not any(ErrorExpr e).getEnclosingFunction() = func and
-    not intentionallyReturnsStackPointer(func)
-  )
-}
-
-/**
- * Holds if `sink` is a node that represents the `StoreInstruction` that is subsequently used in
- * a `ReturnValueInstruction`. We use the `StoreInstruction` instead of the instruction that defines the
- * `ReturnValueInstruction`'s source value oprand because the former has better location information.
- */
-predicate isSink(Node sink) {
-  exists(StoreInstruction store |
-    store.getDestinationAddress().(VariableAddressInstruction).getIRVariable() instanceof
-      IRReturnVariable and
-    sink.asOperand() = store.getSourceValueOperand()
-  )
-}
-
-/** Holds if `node1` _must_ flow to `node2`. */
-predicate step(Node node1, Node node2) {
-  instructionToOperandStep(node1.asInstruction(), node2.asOperand())
-  or
-  operandToInstructionStep(node1.asOperand(), node2.asInstruction())
-}
-
-predicate instructionToOperandStep(Instruction instr, Operand operand) { operand.getDef() = instr }
-
-/**
- * Holds if `operand` flows to the result of `instr`.
- *
- * This predicate ignores flow through `PhiInstruction`s to create a 'must flow' relation. It also
- * intentionally conflates addresses of fields and their object, and pointer offsets with their
- * base pointer as this allows us to detect cases where an object's address flows to a return statement
- * via a field. For example:
- *
- * ```cpp
- * struct S { int x, y };
- * int* test() {
- *   S s;
- *   return &s.x; // BAD: &s.x is an address of a variable on the stack.
- * }
- * ```
- */
-predicate operandToInstructionStep(Operand operand, Instruction instr) {
-  instr.(CopyInstruction).getSourceValueOperand() = operand
-  or
-  instr.(ConvertInstruction).getUnaryOperand() = operand
-  or
-  instr.(CheckedConvertOrNullInstruction).getUnaryOperand() = operand
-  or
-  instr.(InheritanceConversionInstruction).getUnaryOperand() = operand
-  or
-  instr.(FieldAddressInstruction).getObjectAddressOperand() = operand
-  or
-  instr.(PointerOffsetInstruction).getLeftOperand() = operand
-}
-
-/** Holds if a source node flows to `n`. */
-predicate branchlessLocalFlow0(Node n) {
-  isSource(n)
-  or
-  exists(Node mid |
-    branchlessLocalFlow0(mid) and
-    step(mid, n)
-  )
-}
-
-/** Holds if `n` is reachable through some source node, and `n` also eventually reaches a sink. */
-predicate branchlessLocalFlow1(Node n) {
-  branchlessLocalFlow0(n) and
-  (
-    isSink(n)
-    or
-    exists(Node mid |
-      branchlessLocalFlow1(mid) and
-      step(n, mid)
+class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
+  ReturnStackAllocatedMemoryConfig() { this = "ReturnStackAllocatedMemoryConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    // Holds if `source` is a node that represents the use of a stack variable
+    exists(VariableAddressInstruction var, Function func |
+      var = source.asInstruction() and
+      func = var.getEnclosingFunction() and
+      var.getASTVariable() instanceof StackVariable and
+      // Pointer-to-member types aren't properly handled in the dbscheme.
+      not var.getResultType() instanceof PointerToMemberType and
+      // Rule out FPs caused by extraction errors.
+      not any(ErrorExpr e).getEnclosingFunction() = func and
+      not intentionallyReturnsStackPointer(func)
     )
-  )
-}
+  }
 
-newtype TLocalPathNode =
-  TLocalPathNodeMid(Node n) {
-    branchlessLocalFlow1(n) and
-    (
-      isSource(n) or
-      exists(LocalPathNodeMid mid | step(mid.getNode(), n))
+  override predicate isSink(DataFlow::Node sink) {
+    // Holds if `sink` is a node that represents the `StoreInstruction` that is subsequently used in
+    // a `ReturnValueInstruction`.
+    // We use the `StoreInstruction` instead of the instruction that defines the
+    // `ReturnValueInstruction`'s source value oprand because the former has better location information.
+    exists(StoreInstruction store |
+      store.getDestinationAddress().(VariableAddressInstruction).getIRVariable() instanceof
+        IRReturnVariable and
+      sink.asOperand() = store.getSourceValueOperand()
     )
   }
 
-abstract class LocalPathNode extends TLocalPathNode {
-  Node n;
-
-  /** Gets the underlying node. */
-  Node getNode() { result = n }
-
-  /** Gets a textual representation of this node. */
-  string toString() { result = n.toString() }
-
-  /** Gets the location of this element. */
-  Location getLocation() { result = n.getLocation() }
-
-  /** Gets a successor `LocalPathNode`, if any. */
-  LocalPathNode getASuccessor() { step(this.getNode(), result.getNode()) }
-}
-
-class LocalPathNodeMid extends LocalPathNode, TLocalPathNodeMid {
-  LocalPathNodeMid() { this = TLocalPathNodeMid(n) }
-}
-
-class LocalPathNodeSink extends LocalPathNodeMid {
-  LocalPathNodeSink() { isSink(this.getNode()) }
-}
-
-/**
- * Holds if `source` is a source node, `sink` is a sink node, and there's flow
- * from `source` to `sink` using `step` relation.
- */
-predicate hasFlow(LocalPathNode source, LocalPathNodeSink sink) {
-  isSource(source.getNode()) and
-  source.getASuccessor+() = sink
-}
-
-predicate reach(LocalPathNode n) { n instanceof LocalPathNodeSink or reach(n.getASuccessor()) }
-
-query predicate edges(LocalPathNode a, LocalPathNode b) { a.getASuccessor() = b and reach(b) }
-
-query predicate nodes(LocalPathNode n, string key, string val) {
-  reach(n) and key = "semmle.label" and val = n.toString()
+  /**
+   * This configuration intentionally conflates addresses of fields and their object, and pointer offsets
+   * with their base pointer as this allows us to detect cases where an object's address flows to a
+   * return statement via a field. For example:
+   *
+   * ```cpp
+   * struct S { int x, y };
+   * int* test() {
+   *   S s;
+   *   return &s.x; // BAD: &s.x is an address of a variable on the stack.
+   * }
+   */
+  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    node2.asInstruction().(FieldAddressInstruction).getObjectAddressOperand() = node1.asOperand()
+    or
+    node2.asInstruction().(PointerOffsetInstruction).getLeftOperand() = node1.asOperand()
+  }
 }
 
-from LocalPathNode source, LocalPathNodeSink sink, VariableAddressInstruction var
+from
+  MustFlowPathNode source, MustFlowPathNode sink, VariableAddressInstruction var,
+  ReturnStackAllocatedMemoryConfig conf
 where
-  hasFlow(source, sink) and
-  source.getNode().asInstruction() = var
+  conf.hasFlowPath(source, sink) and
+  source.getNode().asInstruction() = var and
+  // Only raise an alert if we're returning from the _same_ callable as the on that
+  // declared the stack variable.
+  var.getEnclosingFunction() = sink.getNode().getEnclosingCallable()
 select sink.getNode(), source, sink, "May return stack-allocated memory from $@.", var.getAST(),
   var.getAST().toString()