Merge pull request github#15047 from MathiasVP/add-puns-for-addresses-of-arguments

C++: Add `PostUpdateNode`s for addresses of outgoing arguments

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -39,7 +39,7 @@ private newtype TIRDataFlowNode =
   } or
   TPostUpdateNodeImpl(Operand operand, int indirectionIndex) {
     operand = any(FieldAddress fa).getObjectAddressOperand() and
-    indirectionIndex = [1 .. Ssa::countIndirectionsForCppType(Ssa::getLanguageType(operand))]
+    indirectionIndex = [0 .. Ssa::countIndirectionsForCppType(Ssa::getLanguageType(operand))]
     or
     Ssa::isModifiableByCall(operand, indirectionIndex)
   } or
@@ -436,7 +436,12 @@ class Node extends TIRDataFlowNode {
    * `x.set(taint())` is a partial definition of `x`, and `transfer(&x, taint())` is
    * a partial definition of `&x`).
    */
-  Expr asPartialDefinition() { result = this.(PartialDefinitionNode).getDefinedExpr() }
+  Expr asPartialDefinition() {
+    exists(PartialDefinitionNode pdn | this = pdn |
+      pdn.getIndirectionIndex() > 0 and
+      result = pdn.getDefinedExpr()
+    )
+  }
 
   /**
    * Gets an upper bound on the type of this node.
@@ -563,11 +568,17 @@ private class PostUpdateNodeImpl extends PartialDefinitionNode, TPostUpdateNodeI
   Operand getOperand() { result = operand }
 
   /** Gets the indirection index associated with this node. */
-  int getIndirectionIndex() { result = indirectionIndex }
+  override int getIndirectionIndex() { result = indirectionIndex }
 
   override Location getLocationImpl() { result = operand.getLocation() }
 
-  final override Node getPreUpdateNode() { hasOperandAndIndex(result, operand, indirectionIndex) }
+  final override Node getPreUpdateNode() {
+    indirectionIndex > 0 and
+    hasOperandAndIndex(result, operand, indirectionIndex)
+    or
+    indirectionIndex = 0 and
+    result.asOperand() = operand
+  }
 
   final override Expr getDefinedExpr() {
     result = operand.getDef().getUnconvertedResultExpression()
@@ -836,12 +847,14 @@ class IndirectArgumentOutNode extends PostUpdateNodeImpl {
   Function getStaticCallTarget() { result = this.getCallInstruction().getStaticCallTarget() }
 
   override string toStringImpl() {
-    // This string should be unique enough to be helpful but common enough to
-    // avoid storing too many different strings.
-    result = this.getStaticCallTarget().getName() + " output argument"
-    or
-    not exists(this.getStaticCallTarget()) and
-    result = "output argument"
+    exists(string prefix | if indirectionIndex > 0 then prefix = "" else prefix = "pointer to " |
+      // This string should be unique enough to be helpful but common enough to
+      // avoid storing too many different strings.
+      result = prefix + this.getStaticCallTarget().getName() + " output argument"
+      or
+      not exists(this.getStaticCallTarget()) and
+      result = prefix + "output argument"
+    )
   }
 }
 
@@ -1699,6 +1712,10 @@ abstract class PostUpdateNode extends Node {
  * ```
  */
 abstract private class PartialDefinitionNode extends PostUpdateNode {
+  /** Gets the indirection index of this node. */
+  abstract int getIndirectionIndex();
+
+  /** Gets the expression that is partially defined by this node. */
   abstract Expr getDefinedExpr();
 }
 
@@ -1713,6 +1730,8 @@ abstract private class PartialDefinitionNode extends PostUpdateNode {
  * `getVariableAccess()` equal to `x`.
  */
 class DefinitionByReferenceNode extends IndirectArgumentOutNode {
+  DefinitionByReferenceNode() { this.getIndirectionIndex() > 0 }
+
   /** Gets the unconverted argument corresponding to this node. */
   Expr getArgument() { result = this.getAddressOperand().getDef().getUnconvertedResultExpression() }
 

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll

@@ -417,56 +417,34 @@ class BaseCallVariable extends AbstractBaseSourceVariable, TBaseCallVariable {
   override CppType getLanguageType() { result = getResultLanguageType(call) }
 }
 
-/**
- * Holds if the value pointed to by `operand` can potentially be
- * modified be the caller.
- */
-predicate isModifiableByCall(ArgumentOperand operand, int indirectionIndex) {
-  exists(CallInstruction call, int index, CppType type |
-    indirectionIndex = [1 .. countIndirectionsForCppType(type)] and
-    type = getLanguageType(operand) and
-    call.getArgumentOperand(index) = operand and
-    if index = -1
-    then
-      // A qualifier is "modifiable" if:
-      // 1. the member function is not const specified, or
-      // 2. the member function is `const` specified, but returns a pointer or reference
-      // type that is non-const.
-      //
-      // To see why this is necessary, consider the following function:
-      // ```
-      // struct C {
-      //   void* data_;
-      //   void* data() const { return data; }
-      // };
-      // ...
-      // C c;
-      // memcpy(c.data(), source, 16)
-      // ```
-      // the data pointed to by `c.data_` is potentially modified by the call to `memcpy` even though
-      // `C::data` has a const specifier. So we further place the restriction that the type returned
-      // by `call` should not be of the form `const T*` (for some deeply const type `T`).
-      if call.getStaticCallTarget() instanceof Cpp::ConstMemberFunction
-      then
-        exists(PointerOrArrayOrReferenceType resultType |
-          resultType = call.getResultType() and
-          not resultType.isDeeplyConstBelow()
-        )
-      else any()
-    else
-      // An argument is modifiable if it's a non-const pointer or reference type.
-      isModifiableAt(type, indirectionIndex)
-  )
-}
-
-/**
- * Holds if `t` is a pointer or reference type that supports at least `indirectionIndex` number
- * of indirections, and the `indirectionIndex` indirection cannot be modfiied by passing a
- * value of `t` to a function.
- */
-private predicate isModifiableAtImpl(CppType cppType, int indirectionIndex) {
-  indirectionIndex = [1 .. countIndirectionsForCppType(cppType)] and
-  (
+private module IsModifiableAtImpl {
+  /**
+   * Holds if the `indirectionIndex`'th dereference of a value of type
+   * `cppType` is a type that can be modified (either by modifying the value
+   * itself or one of its fields if it's a class type).
+   *
+   * For example, a value of type `const int* const` cannot be modified
+   * at any indirection index (because it's a constant pointer to constant
+   * data), and a value of type `int *const *` is modifiable at indirection index
+   * 2 only.
+   *
+   * A value of type `const S2* s2` where `s2` is
+   * ```cpp
+   * struct S { int x; }
+   * ```
+   * can be modified at indirection index 1. This is to ensure that we generate
+   * a `PostUpdateNode` for the argument corresponding to the `s2` parameter in
+   * an example such as:
+   * ```cpp
+   * void set_field(const S2* s2)
+   * {
+   *  s2->s->x = 42;
+   * }
+   * ```
+   */
+  bindingset[cppType, indirectionIndex]
+  pragma[inline_late]
+  private predicate impl(CppType cppType, int indirectionIndex) {
     exists(Type pointerType, Type base, Type t |
       pointerType = t.getUnderlyingType() and
       pointerType = any(Indirection ind).getUnderlyingType() and
@@ -480,29 +458,115 @@ private predicate isModifiableAtImpl(CppType cppType, int indirectionIndex) {
       // one of the members was modified.
       exists(base.stripType().(Cpp::Class).getAField())
     )
+  }
+
+  /**
+   * Holds if `cppType` is modifiable with an indirection index of at least 1.
+   *
+   * This predicate factored out into a separate predicate for two reasons:
+   * - This predicate needs to be recursive because, if a type is modifiable
+   * at indirection `i`, then it's also modifiable at indirection index `i+1`
+   * (because the pointer could be completely re-assigned at indirection `i`).
+   * - We special-case indirection index `0` so that pointer arguments that can
+   * be modified at some index always have a `PostUpdateNode` at indiretion
+   * index 0 even though the 0'th indirection can never be modified by a
+   * callee.
+   */
+  private predicate isModifiableAtImplAtLeast1(CppType cppType, int indirectionIndex) {
+    indirectionIndex = [1 .. countIndirectionsForCppType(cppType)] and
+    (
+      impl(cppType, indirectionIndex)
+      or
+      // If the `indirectionIndex`'th dereference of a type can be modified
+      // then so can the  `indirectionIndex + 1`'th dereference.
+      isModifiableAtImplAtLeast1(cppType, indirectionIndex - 1)
+    )
+  }
+
+  /**
+   * Holds if `cppType` is modifiable at indirection index 0.
+   *
+   * In reality, the 0'th indirection of a pointer (i.e., the pointer itself)
+   * can never be modified by a callee, but it is sometimes useful to be able
+   * to specify the value of the pointer, as its coming out of a function, as
+   * a source of dataflow since the shared library's reverse-read mechanism
+   * then ensures that field-flow is accounted for.
+   */
+  private predicate isModifiableAtImplAt0(CppType cppType) { impl(cppType, 0) }
+
+  /**
+   * Holds if `t` is a pointer or reference type that supports at least
+   * `indirectionIndex` number of indirections, and the `indirectionIndex`
+   * indirection cannot be modfiied by passing a value of `t` to a function.
+   */
+  private predicate isModifiableAtImpl(CppType cppType, int indirectionIndex) {
+    isModifiableAtImplAtLeast1(cppType, indirectionIndex)
     or
-    // If the `indirectionIndex`'th dereference of a type can be modified
-    // then so can the  `indirectionIndex + 1`'th dereference.
-    isModifiableAtImpl(cppType, indirectionIndex - 1)
-  )
-}
+    indirectionIndex = 0 and
+    isModifiableAtImplAt0(cppType)
+  }
 
-/**
- * Holds if `t` is a type with at least `indirectionIndex` number of indirections,
- * and the `indirectionIndex` indirection can be modified by passing a value of
- * type `t` to a function function.
- */
-bindingset[indirectionIndex]
-predicate isModifiableAt(CppType cppType, int indirectionIndex) {
-  isModifiableAtImpl(cppType, indirectionIndex)
-  or
-  exists(PointerWrapper pw, Type t |
-    cppType.hasType(t, _) and
-    t.stripType() = pw and
-    not pw.pointsToConst()
-  )
+  /**
+   * Holds if `t` is a type with at least `indirectionIndex` number of
+   * indirections, and the `indirectionIndex` indirection can be modified by
+   * passing a value of type `t` to a function function.
+   */
+  bindingset[indirectionIndex]
+  predicate isModifiableAt(CppType cppType, int indirectionIndex) {
+    isModifiableAtImpl(cppType, indirectionIndex)
+    or
+    exists(PointerWrapper pw, Type t |
+      cppType.hasType(t, _) and
+      t.stripType() = pw and
+      not pw.pointsToConst()
+    )
+  }
+
+  /**
+   * Holds if the value pointed to by `operand` can potentially be
+   * modified be the caller.
+   */
+  predicate isModifiableByCall(ArgumentOperand operand, int indirectionIndex) {
+    exists(CallInstruction call, int index, CppType type |
+      indirectionIndex = [0 .. countIndirectionsForCppType(type)] and
+      type = getLanguageType(operand) and
+      call.getArgumentOperand(index) = operand and
+      if index = -1
+      then
+        // A qualifier is "modifiable" if:
+        // 1. the member function is not const specified, or
+        // 2. the member function is `const` specified, but returns a pointer or reference
+        // type that is non-const.
+        //
+        // To see why this is necessary, consider the following function:
+        // ```
+        // struct C {
+        //   void* data_;
+        //   void* data() const { return data; }
+        // };
+        // ...
+        // C c;
+        // memcpy(c.data(), source, 16)
+        // ```
+        // the data pointed to by `c.data_` is potentially modified by the call to `memcpy` even though
+        // `C::data` has a const specifier. So we further place the restriction that the type returned
+        // by `call` should not be of the form `const T*` (for some deeply const type `T`).
+        if call.getStaticCallTarget() instanceof Cpp::ConstMemberFunction
+        then
+          exists(PointerOrArrayOrReferenceType resultType |
+            resultType = call.getResultType() and
+            not resultType.isDeeplyConstBelow()
+          )
+        else any()
+      else
+        // An argument is modifiable if it's a non-const pointer or reference type.
+        isModifiableAt(type, indirectionIndex)
+    )
+  }
 }
 
+import IsModifiableAtImpl
+
 abstract class BaseSourceVariableInstruction extends Instruction {
   /** Gets the base source variable accessed by this instruction. */
   abstract BaseSourceVariable getBaseSourceVariable();

cpp/ql/src/Critical/DoubleFree.ql

@@ -16,7 +16,11 @@ import semmle.code.cpp.dataflow.new.DataFlow
 import FlowAfterFree
 import DoubleFree::PathGraph
 
-predicate isFree(DataFlow::Node n, Expr e) { isFree(n, e, _) }
+/**
+ * Holds if `n` is a dataflow node that represents a pointer going into a
+ * deallocation function, and `e` is the corresponding expression.
+ */
+predicate isFree(DataFlow::Node n, Expr e) { isFree(_, n, e, _) }
 
 /**
  * `dealloc1` is a deallocation expression and `e` is an expression such
@@ -28,7 +32,7 @@ predicate isFree(DataFlow::Node n, Expr e) { isFree(n, e, _) }
  */
 bindingset[dealloc1, e]
 predicate isExcludeFreePair(DeallocationExpr dealloc1, Expr e) {
-  exists(DeallocationExpr dealloc2 | isFree(_, e, dealloc2) |
+  exists(DeallocationExpr dealloc2 | isFree(_, _, e, dealloc2) |
     dealloc1.(FunctionCall).getTarget().hasGlobalName("MmFreePagesFromMdl") and
     // From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl:
     // "After calling MmFreePagesFromMdl, the caller must also call ExFreePool
@@ -42,7 +46,7 @@ module DoubleFree = FlowFromFree<isFree/2, isExcludeFreePair/2>;
 from DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2
 where
   DoubleFree::flowPath(source, sink) and
-  isFree(source.getNode(), _, dealloc) and
+  isFree(source.getNode(), _, _, dealloc) and
   isFree(sink.getNode(), e2)
 select sink.getNode(), source, sink,
   "Memory pointed to by '" + e2.toString() + "' may already have been freed by $@.", dealloc,

cpp/ql/src/Critical/FlowAfterFree.qll

@@ -50,12 +50,12 @@ predicate strictlyDominates(IRBlock b1, int i1, IRBlock b2, int i2) {
 module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
   module FlowFromFreeConfig implements DataFlow::StateConfigSig {
     class FlowState instanceof Expr {
-      FlowState() { isFree(_, this, _) }
+      FlowState() { isFree(_, _, this, _) }
 
       string toString() { result = super.toString() }
     }
 
-    predicate isSource(DataFlow::Node node, FlowState state) { isFree(node, state, _) }
+    predicate isSource(DataFlow::Node node, FlowState state) { isFree(node, _, state, _) }
 
     pragma[inline]
     predicate isSink(DataFlow::Node sink, FlowState state) {
@@ -64,7 +64,7 @@ module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
         DeallocationExpr dealloc
       |
         isASink(sink, e) and
-        isFree(source, state, dealloc) and
+        isFree(source, _, state, dealloc) and
         e != state and
         source.hasIndexInBlock(b1, i1) and
         sink.hasIndexInBlock(b2, i2) and
@@ -94,14 +94,17 @@ module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
 }
 
 /**
- * Holds if `n` is a dataflow node such that `n.asExpr() = e` and `e`
- * is being freed by a deallocation expression `dealloc`.
+ * Holds if `outgoing` is a dataflow node that represents the pointer passed to
+ * `dealloc` after the call returns (i.e., the post-update node associated with
+ * the argument to `dealloc`), and `incoming` is the corresponding argument
+ * node going into `dealloc` (i.e., the pre-update node of `outgoing`).
  */
-predicate isFree(DataFlow::Node n, Expr e, DeallocationExpr dealloc) {
+predicate isFree(DataFlow::Node outgoing, DataFlow::Node incoming, Expr e, DeallocationExpr dealloc) {
   exists(Expr conv |
     e = conv.getUnconverted() and
     conv = dealloc.getFreedExpr().getFullyConverted() and
-    conv = n.asConvertedExpr()
+    incoming = outgoing.(DataFlow::PostUpdateNode).getPreUpdateNode() and
+    conv = incoming.asConvertedExpr()
   ) and
   // Ignore realloc functions
   not exists(dealloc.(FunctionCall).getTarget().(AllocationFunction).getReallocPtrArg())

cpp/ql/src/Critical/UseAfterFree.ql

@@ -30,7 +30,7 @@ private predicate externalCallNeverDereferences(FormattingFunctionCall call, int
 }
 
 predicate isUse0(Expr e) {
-  not isFree(_, e, _) and
+  not isFree(_, _, e, _) and
   (
     e = any(PointerDereferenceExpr pde).getOperand()
     or
@@ -170,6 +170,6 @@ module UseAfterFree = FlowFromFree<isUse/2, isExcludeFreeUsePair/2>;
 from UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc
 where
   UseAfterFree::flowPath(source, sink) and
-  isFree(source.getNode(), _, dealloc)
+  isFree(source.getNode(), _, _, dealloc)
 select sink.getNode(), source, sink, "Memory may have been previously freed by $@.", dealloc,
   dealloc.toString()