Merge pull request #4186 from github/rc/1.25

Mergeback: 1.25 -> main

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll

@@ -50,13 +50,25 @@ class Node extends TNode {
   /** Gets the type of this node. */
   Type getType() { none() } // overridden in subclasses
 
-  /** Gets the expression corresponding to this node, if any. */
+  /**
+   * Gets the expression corresponding to this node, if any. This predicate
+   * only has a result on nodes that represent the value of evaluating the
+   * expression. For data flowing _out of_ an expression, like when an
+   * argument is passed by reference, use `asDefiningArgument` instead of
+   * `asExpr`.
+   */
   Expr asExpr() { result = this.(ExprNode).getExpr() }
 
   /** Gets the parameter corresponding to this node, if any. */
   Parameter asParameter() { result = this.(ExplicitParameterNode).getParameter() }
 
-  /** Gets the argument that defines this `DefinitionByReferenceNode`, if any. */
+  /**
+   * Gets the argument that defines this `DefinitionByReferenceNode`, if any.
+   * This predicate should be used instead of `asExpr` when referring to the
+   * value of a reference argument _after_ the call has returned. For example,
+   * in `f(&x)`, this predicate will have `&x` as its result for the `Node`
+   * that represents the new value of `x`.
+   */
   Expr asDefiningArgument() { result = this.(DefinitionByReferenceNode).getArgument() }
 
   /**
@@ -383,7 +395,9 @@ class PreConstructorInitThis extends Node, TPreConstructorInitThis {
 }
 
 /**
- * Gets the `Node` corresponding to `e`.
+ * Gets the `Node` corresponding to the value of evaluating `e`. For data
+ * flowing _out of_ an expression, like when an argument is passed by
+ * reference, use `definitionByReferenceNodeFromArgument` instead.
  */
 ExprNode exprNode(Expr e) { result.getExpr() = e }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll

@@ -120,15 +120,25 @@ private module PartialDefinitions {
       )
     }
 
-    predicate partiallyDefines(Variable v) { innerDefinedExpr = v.getAnAccess() }
+    deprecated predicate partiallyDefines(Variable v) { innerDefinedExpr = v.getAnAccess() }
 
-    predicate partiallyDefinesThis(ThisExpr e) { innerDefinedExpr = e }
+    deprecated predicate partiallyDefinesThis(ThisExpr e) { innerDefinedExpr = e }
 
     /**
      * Gets the subBasicBlock where this `PartialDefinition` is defined.
      */
     ControlFlowNode getSubBasicBlockStart() { result = node }
 
+    /**
+     * Holds if this `PartialDefinition` defines variable `v` at control-flow
+     * node `cfn`.
+     */
+    pragma[noinline]
+    predicate partiallyDefinesVariableAt(Variable v, ControlFlowNode cfn) {
+      innerDefinedExpr = v.getAnAccess() and
+      cfn = node
+    }
+
     /**
      * Holds if this partial definition may modify `inner` (or what it points
      * to) through `outer`. These expressions will never be `Conversion`s.
@@ -188,7 +198,7 @@ module FlowVar_internal {
   predicate fullySupportedSsaVariable(Variable v) {
     v = any(SsaDefinition def).getAVariable() and
     // A partially-defined variable is handled using the partial definitions logic.
-    not any(PartialDefinition p).partiallyDefines(v) and
+    not any(PartialDefinition p).partiallyDefinesVariableAt(v, _) and
     // SSA variables do not exist before their first assignment, but one
     // feature of this data flow library is to track where uninitialized data
     // ends up.
@@ -232,7 +242,7 @@ module FlowVar_internal {
         or
         assignmentLikeOperation(sbb, v, _, _)
         or
-        sbb = any(PartialDefinition p | p.partiallyDefines(v)).getSubBasicBlockStart()
+        exists(PartialDefinition p | p.partiallyDefinesVariableAt(v, sbb))
         or
         blockVarDefinedByVariable(sbb, v)
       )
@@ -363,8 +373,7 @@ module FlowVar_internal {
 
     override predicate definedPartiallyAt(Expr e) {
       exists(PartialDefinition p |
-        p.partiallyDefines(v) and
-        sbb = p.getSubBasicBlockStart() and
+        p.partiallyDefinesVariableAt(v, sbb) and
         p.definesExpressions(_, e)
       )
     }
@@ -427,7 +436,7 @@ module FlowVar_internal {
     /**
      * Gets a variable that is assigned in this loop and read outside the loop.
      */
-    private Variable getARelevantVariable() {
+    Variable getARelevantVariable() {
       result = this.getAVariableAssignedInLoop() and
       exists(VariableAccess va |
         va.getTarget() = result and
@@ -472,10 +481,16 @@ module FlowVar_internal {
         reachesWithoutAssignment(bb.getAPredecessor(), v) and
         this.bbInLoop(bb)
       ) and
-      not assignmentLikeOperation(bb.getANode(), v, _, _)
+      not assignsToVar(bb, v)
     }
   }
 
+  pragma[noinline]
+  private predicate assignsToVar(BasicBlock bb, Variable v) {
+    assignmentLikeOperation(bb.getANode(), v, _, _) and
+    exists(AlwaysTrueUponEntryLoop loop | v = loop.getARelevantVariable())
+  }
+
   /**
    * Holds if `loop` always assigns to `v` before leaving through an edge
    * from `bbInside` in its condition to `bbOutside` outside the loop. Also,
@@ -736,7 +751,7 @@ module FlowVar_internal {
       exists(Variable v | not fullySupportedSsaVariable(v) |
         assignmentLikeOperation(this, v, _, _)
         or
-        this = any(PartialDefinition p | p.partiallyDefines(v)).getSubBasicBlockStart()
+        exists(PartialDefinition p | p.partiallyDefinesVariableAt(v, this))
         // It is not necessary to cut the basic blocks at `Initializer` nodes
         // because the affected variable can have no _other_ value before its
         // initializer. It is not necessary to cut basic blocks at procedure

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -70,7 +70,7 @@ private DataFlow::Node getNodeForSource(Expr source) {
     //
     // This case goes together with the similar (but not identical) rule in
     // `nodeIsBarrierIn`.
-    result = DataFlow::definitionByReferenceNode(source) and
+    result = DataFlow::definitionByReferenceNodeFromArgument(source) and
     not argv(source.(VariableAccess).getTarget())
   )
 }
@@ -210,7 +210,7 @@ private predicate nodeIsBarrierIn(DataFlow::Node node) {
     or
     // This case goes together with the similar (but not identical) rule in
     // `getNodeForSource`.
-    node = DataFlow::definitionByReferenceNode(source)
+    node = DataFlow::definitionByReferenceNodeFromArgument(source)
   )
 }
 

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

@@ -42,9 +42,14 @@ class Node extends TIRDataFlowNode {
   Operand asOperand() { result = this.(OperandNode).getOperand() }
 
   /**
-   * Gets the non-conversion expression corresponding to this node, if any. If
-   * this node strictly (in the sense of `asConvertedExpr`) corresponds to a
-   * `Conversion`, then the result is that `Conversion`'s non-`Conversion` base
+   * Gets the non-conversion expression corresponding to this node, if any.
+   * This predicate only has a result on nodes that represent the value of
+   * evaluating the expression. For data flowing _out of_ an expression, like
+   * when an argument is passed by reference, use `asDefiningArgument` instead
+   * of `asExpr`.
+   *
+   * If this node strictly (in the sense of `asConvertedExpr`) corresponds to
+   * a `Conversion`, then the result is the underlying non-`Conversion` base
    * expression.
    */
   Expr asExpr() { result = this.(ExprNode).getExpr() }
@@ -55,7 +60,13 @@ class Node extends TIRDataFlowNode {
    */
   Expr asConvertedExpr() { result = this.(ExprNode).getConvertedExpr() }
 
-  /** Gets the argument that defines this `DefinitionByReferenceNode`, if any. */
+  /**
+   * Gets the argument that defines this `DefinitionByReferenceNode`, if any.
+   * This predicate should be used instead of `asExpr` when referring to the
+   * value of a reference argument _after_ the call has returned. For example,
+   * in `f(&x)`, this predicate will have `&x` as its result for the `Node`
+   * that represents the new value of `x`.
+   */
   Expr asDefiningArgument() { result = this.(DefinitionByReferenceNode).getArgument() }
 
   /** Gets the positional parameter corresponding to this node, if any. */
@@ -378,7 +389,7 @@ private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNod
 class DefinitionByReferenceNode extends InstructionNode {
   override WriteSideEffectInstruction instr;
 
-  /** Gets the argument corresponding to this node. */
+  /** Gets the unconverted argument corresponding to this node. */
   Expr getArgument() {
     result =
       instr
@@ -462,20 +473,26 @@ class VariableNode extends Node, TVariableNode {
 InstructionNode instructionNode(Instruction instr) { result.getInstruction() = instr }
 
 /**
+ * DEPRECATED: use `definitionByReferenceNodeFromArgument` instead.
+ *
  * Gets the `Node` corresponding to a definition by reference of the variable
  * that is passed as `argument` of a call.
  */
-DefinitionByReferenceNode definitionByReferenceNode(Expr e) { result.getArgument() = e }
+deprecated DefinitionByReferenceNode definitionByReferenceNode(Expr e) { result.getArgument() = e }
 
 /**
- * Gets a `Node` corresponding to `e` or any of its conversions. There is no
- * result if `e` is a `Conversion`.
+ * Gets the `Node` corresponding to the value of evaluating `e` or any of its
+ * conversions. There is no result if `e` is a `Conversion`. For data flowing
+ * _out of_ an expression, like when an argument is passed by reference, use
+ * `definitionByReferenceNodeFromArgument` instead.
  */
 ExprNode exprNode(Expr e) { result.getExpr() = e }
 
 /**
- * Gets the `Node` corresponding to `e`, if any. Here, `e` may be a
- * `Conversion`.
+ * Gets the `Node` corresponding to the value of evaluating `e`. Here, `e` may
+ * be a `Conversion`. For data flowing _out of_ an expression, like when an
+ * argument is passed by reference, use
+ * `definitionByReferenceNodeFromArgument` instead.
  */
 ExprNode convertedExprNode(Expr e) { result.getConvertedExpr() = e }
 
@@ -484,6 +501,14 @@ ExprNode convertedExprNode(Expr e) { result.getConvertedExpr() = e }
  */
 ExplicitParameterNode parameterNode(Parameter p) { result.getParameter() = p }
 
+/**
+ * Gets the `Node` corresponding to a definition by reference of the variable
+ * that is passed as unconverted `argument` of a call.
+ */
+DefinitionByReferenceNode definitionByReferenceNodeFromArgument(Expr argument) {
+  result.getArgument() = argument
+}
+
 /** Gets the `VariableNode` corresponding to the variable `v`. */
 VariableNode variableNode(Variable v) { result.getVariable() = v }