Merge pull request github#11311 from MathiasVP/repair-mustflow

C++: Repair `MustFlow` library for use-use flow

cpp/ql/lib/change-notes/2022-11-16-must-flow.md

@@ -0,0 +1,4 @@
+---
+category: breaking
+---
+The predicates in the `MustFlow::Configuration` class used by the `MustFlow` library (`semmle.code.cpp.ir.dataflow.MustFlow`) have changed to be defined directly in terms of the C++ IR instead of IR dataflow nodes.

cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll

@@ -5,7 +5,6 @@
  */
 
 private import cpp
-import semmle.code.cpp.ir.dataflow.DataFlow
 private import semmle.code.cpp.ir.IR
 
 /**
@@ -25,18 +24,18 @@ abstract class MustFlowConfiguration extends string {
  /**
  * Holds if `source` is a relevant data flow source.
  */
- abstract predicate isSource(DataFlow::Node source);
+ abstract predicate isSource(Instruction source);
 
  /**
  * Holds if `sink` is a relevant data flow sink.
  */
- abstract predicate isSink(DataFlow::Node sink);
+ abstract predicate isSink(Operand sink);
 
  /**
  * Holds if the additional flow step from `node1` to `node2` must be taken
  * into account in the analysis.
  */
- predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { none() }
+ predicate isAdditionalFlowStep(Operand node1, Instruction node2) { none() }
 
  /** Holds if this configuration allows flow from arguments to parameters. */
  predicate allowInterproceduralFlow() { any() }
@@ -48,30 +47,30 @@ abstract class MustFlowConfiguration extends string {
  * included in the module `PathGraph`.
  */
  final predicate hasFlowPath(MustFlowPathNode source, MustFlowPathSink sink) {
- this.isSource(source.getNode()) and
+ this.isSource(source.getInstruction()) and
  source.getASuccessor+() = sink
  }
 }
 
 /** Holds if `node` flows from a source. */
 pragma[nomagic]
-private predicate flowsFromSource(DataFlow::Node node, MustFlowConfiguration config) {
+private predicate flowsFromSource(Instruction node, MustFlowConfiguration config) {
  config.isSource(node)
  or
- exists(DataFlow::Node mid |
+ exists(Instruction mid |
  step(mid, node, config) and
  flowsFromSource(mid, pragma[only_bind_into](config))
  )
 }
 
 /** Holds if `node` flows to a sink. */
 pragma[nomagic]
-private predicate flowsToSink(DataFlow::Node node, MustFlowConfiguration config) {
+private predicate flowsToSink(Instruction node, MustFlowConfiguration config) {
  flowsFromSource(node, pragma[only_bind_into](config)) and
  (
- config.isSink(node)
+ config.isSink(node.getAUse())
  or
- exists(DataFlow::Node mid |
+ exists(Instruction mid |
  step(node, mid, config) and
  flowsToSink(mid, pragma[only_bind_into](config))
  )
@@ -198,12 +197,13 @@ private module Cached {
  }
 
  cached
- predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
- instructionToOperandStep(nodeFrom.asInstruction(), nodeTo.asOperand())
- or
- flowThroughCallable(nodeFrom.asInstruction(), nodeTo.asInstruction())
+ predicate step(Instruction nodeFrom, Instruction nodeTo) {
+ exists(Operand mid |
+ instructionToOperandStep(nodeFrom, mid) and
+ operandToInstructionStep(mid, nodeTo)
+ )
  or
- operandToInstructionStep(nodeFrom.asOperand(), nodeTo.asInstruction())
+ flowThroughCallable(nodeFrom, nodeTo)
  }
 }
 
@@ -213,12 +213,12 @@ private module Cached {
  * way around.
  */
 pragma[inline]
-private Declaration getEnclosingCallable(DataFlow::Node n) {
- pragma[only_bind_into](result) = pragma[only_bind_out](n).getEnclosingCallable()
+private IRFunction getEnclosingCallable(Instruction n) {
+ pragma[only_bind_into](result) = pragma[only_bind_out](n).getEnclosingIRFunction()
 }
 
 /** Holds if `nodeFrom` flows to `nodeTo`. */
-private predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, MustFlowConfiguration config) {
+private predicate step(Instruction nodeFrom, Instruction nodeTo, MustFlowConfiguration config) {
  exists(config) and
  Cached::step(pragma[only_bind_into](nodeFrom), pragma[only_bind_into](nodeTo)) and
  (
@@ -227,45 +227,45 @@ private predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, MustFlowC
  getEnclosingCallable(nodeFrom) = getEnclosingCallable(nodeTo)
  )
  or
- config.isAdditionalFlowStep(nodeFrom, nodeTo)
+ config.isAdditionalFlowStep(nodeFrom.getAUse(), nodeTo)
 }
 
 private newtype TLocalPathNode =
- MkLocalPathNode(DataFlow::Node n, MustFlowConfiguration config) {
+ MkLocalPathNode(Instruction n, MustFlowConfiguration config) {
  flowsToSink(n, config) and
  (
  config.isSource(n)
  or
- exists(MustFlowPathNode mid | step(mid.getNode(), n, config))
+ exists(MustFlowPathNode mid | step(mid.getInstruction(), n, config))
  )
  }
 
 /** A `Node` that is in a path from a source to a sink. */
 class MustFlowPathNode extends TLocalPathNode {
- DataFlow::Node n;
+ Instruction n;
 
  MustFlowPathNode() { this = MkLocalPathNode(n, _) }
 
  /** Gets the underlying node. */
- DataFlow::Node getNode() { result = n }
+ Instruction getInstruction() { result = n }
 
  /** Gets a textual representation of this node. */
- string toString() { result = n.toString() }
+ string toString() { result = n.getAst().toString() }
 
  /** Gets the location of this element. */
  Location getLocation() { result = n.getLocation() }
 
  /** Gets a successor node, if any. */
  MustFlowPathNode getASuccessor() {
- step(this.getNode(), result.getNode(), this.getConfiguration())
+ step(this.getInstruction(), result.getInstruction(), this.getConfiguration())
  }
 
  /** Gets the associated configuration. */
  MustFlowConfiguration getConfiguration() { this = MkLocalPathNode(_, result) }
 }
 
 private class MustFlowPathSink extends MustFlowPathNode {
- MustFlowPathSink() { this.getConfiguration().isSink(this.getNode()) }
+ MustFlowPathSink() { this.getConfiguration().isSink(this.getInstruction().getAUse()) }
 }
 
 /**

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

@@ -26,11 +26,11 @@ predicate intentionallyReturnsStackPointer(Function f) {
 class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
  ReturnStackAllocatedMemoryConfig() { this = "ReturnStackAllocatedMemoryConfig" }
 
- override predicate isSource(DataFlow::Node source) {
+ override predicate isSource(Instruction 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 = source and
+ func = source.getEnclosingFunction() and
  var.getAstVariable() instanceof StackVariable and
  // Pointer-to-member types aren't properly handled in the dbscheme.
  not var.getResultType() instanceof PointerToMemberType and
@@ -40,15 +40,15 @@ class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
  )
  }
 
- override predicate isSink(DataFlow::Node sink) {
+ override predicate isSink(Operand 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 operand because the former has better location information.
  exists(StoreInstruction store |
  store.getDestinationAddress().(VariableAddressInstruction).getIRVariable() instanceof
  IRReturnVariable and
- sink.asOperand() = store.getSourceValueOperand()
+ sink = store.getSourceValueOperand()
  )
  }
 
@@ -77,10 +77,10 @@ class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
  * }
  * ```
  */
- override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
- node2.asInstruction().(FieldAddressInstruction).getObjectAddressOperand() = node1.asOperand()
+ override predicate isAdditionalFlowStep(Operand node1, Instruction node2) {
+ node2.(FieldAddressInstruction).getObjectAddressOperand() = node1
  or
- node2.asInstruction().(PointerOffsetInstruction).getLeftOperand() = node1.asOperand()
+ node2.(PointerOffsetInstruction).getLeftOperand() = node1
  }
 }
 
@@ -89,6 +89,6 @@ from
  ReturnStackAllocatedMemoryConfig conf
 where
  conf.hasFlowPath(pragma[only_bind_into](source), pragma[only_bind_into](sink)) and
- source.getNode().asInstruction() = var
-select sink.getNode(), source, sink, "May return stack-allocated memory from $@.", var.getAst(),
- var.getAst().toString()
+ source.getInstruction() = var
+select sink.getInstruction(), source, sink, "May return stack-allocated memory from $@.",
+ var.getAst(), var.getAst().toString()

cpp/ql/src/Likely Bugs/OO/UnsafeUseOfThis.ql

@@ -22,37 +22,40 @@ import PathGraph
 class UnsafeUseOfThisConfig extends MustFlowConfiguration {
  UnsafeUseOfThisConfig() { this = "UnsafeUseOfThisConfig" }
 
- override predicate isSource(DataFlow::Node source) { isSource(source, _, _) }
+ override predicate isSource(Instruction source) { isSource(source, _, _) }
 
- override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
+ override predicate isSink(Operand sink) { isSink(sink, _) }
 }
 
-/** Holds if `instr` is a `this` pointer used by the call instruction `call`. */
-predicate isSink(DataFlow::Node sink, CallInstruction call) {
+/** Holds if `sink` is a `this` pointer used by the call instruction `call`. */
+predicate isSink(Operand sink, CallInstruction call) {
  exists(PureVirtualFunction func |
  call.getStaticCallTarget() = func and
- call.getThisArgument() = sink.asInstruction() and
+ call.getThisArgumentOperand() = sink and
  // Weed out implicit calls to destructors of a base class
  not func instanceof Destructor
  )
 }
 
-/** Holds if `init` initializes the `this` pointer in class `c`. */
-predicate isSource(DataFlow::Node source, string msg, Class c) {
- exists(InitializeParameterInstruction init | init = source.asInstruction() |
- (
- exists(Constructor func |
- not func instanceof CopyConstructor and
- not func instanceof MoveConstructor and
- func = init.getEnclosingFunction() and
- msg = "construction"
- )
- or
- init.getEnclosingFunction() instanceof Destructor and msg = "destruction"
- ) and
- init.getIRVariable() instanceof IRThisVariable and
- init.getEnclosingFunction().getDeclaringType() = c
- )
+/**
+ * Holds if `source` initializes the `this` pointer in class `c`.
+ *
+ * The string `msg` describes whether the enclosing function is a
+ * constructor or destructor.
+ */
+predicate isSource(InitializeParameterInstruction source, string msg, Class c) {
+ (
+ exists(Constructor func |
+ not func instanceof CopyConstructor and
+ not func instanceof MoveConstructor and
+ func = source.getEnclosingFunction() and
+ msg = "construction"
+ )
+ or
+ source.getEnclosingFunction() instanceof Destructor and msg = "destruction"
+ ) and
+ source.getIRVariable() instanceof IRThisVariable and
+ source.getEnclosingFunction().getDeclaringType() = c
 }
 
 /**
@@ -68,8 +71,8 @@ predicate flows(
 ) {
  exists(UnsafeUseOfThisConfig conf |
  conf.hasFlowPath(source, sink) and
- isSource(source.getNode(), msg, sourceClass) and
- isSink(sink.getNode(), call)
+ isSource(source.getInstruction(), msg, sourceClass) and
+ isSink(sink.getInstruction().getAUse(), call)
  )
 }