Merge pull request #16246 from MathiasVP/parameter-nodes-for-functions-without-bodies

C++: Add Parameter nodes for functions without bodies

cpp/ql/lib/change-notes/2024-04-18-param-nodes.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Parameters of functions without definitions now have `ParameterNode`s.

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

@@ -78,6 +78,8 @@ module NodeStars {
     result = n.(PostUpdateNodeImpl).getIndirectionIndex()
     or
     result = n.(FinalParameterNode).getIndirectionIndex()
+    or
+    result = n.(BodyLessParameterNodeImpl).getIndirectionIndex()
   }
 
   /**
@@ -1247,7 +1249,7 @@ module IsUnreachableInCall {
 
   predicate isUnreachableInCall(Node n, DataFlowCall call) {
     exists(
-      DirectParameterNode paramNode, ConstantIntegralTypeArgumentNode arg,
+      InstructionDirectParameterNode paramNode, ConstantIntegralTypeArgumentNode arg,
       IntegerConstantInstruction constant, int k, Operand left, Operand right, IRBlock block
     |
       // arg flows into `paramNode`
@@ -1461,7 +1463,7 @@ private predicate getAdditionalFlowIntoCallNodeTermStep(Node node1, Node node2)
 /** Gets the `IRVariable` associated with the parameter node `p`. */
 pragma[nomagic]
 private IRVariable getIRVariableForParameterNode(ParameterNode p) {
-  result = p.(DirectParameterNode).getIRVariable()
+  result = p.(InstructionDirectParameterNode).getIRVariable()
   or
   result.getAst() = p.(IndirectParameterNode).getParameter()
 }

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

@@ -61,6 +61,15 @@ private newtype TIRDataFlowNode =
   } or
   TFinalGlobalValue(Ssa::GlobalUse globalUse) or
   TInitialGlobalValue(Ssa::GlobalDef globalUse) or
+  TBodyLessParameterNodeImpl(Parameter p, int indirectionIndex) {
+    // Rule out parameters of catch blocks.
+    not exists(p.getCatchBlock()) and
+    // We subtract one because `getMaxIndirectionsForType` returns the maximum
+    // indirection for a glvalue of a given type, and this doesn't apply to
+    // parameters.
+    indirectionIndex = [0 .. Ssa::getMaxIndirectionsForType(p.getUnspecifiedType()) - 1] and
+    not any(InitializeParameterInstruction init).getParameter() = p
+  } or
   TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn)
 
 /**
@@ -389,7 +398,7 @@ class Node extends TIRDataFlowNode {
     index = 0 and
     result = this.(ExplicitParameterNode).getParameter()
     or
-    this.(IndirectParameterNode).hasInstructionAndIndirectionIndex(_, index) and
+    this.(IndirectParameterNode).getIndirectionIndex() = index and
     result = this.(IndirectParameterNode).getParameter()
   }
 
@@ -737,6 +746,40 @@ class InitialGlobalValue extends Node, TInitialGlobalValue {
   override string toStringImpl() { result = globalDef.toString() }
 }
 
+/**
+ * INTERNAL: do not use.
+ *
+ * A node representing a parameter for a function with no body.
+ */
+class BodyLessParameterNodeImpl extends Node, TBodyLessParameterNodeImpl {
+  Parameter p;
+  int indirectionIndex;
+
+  BodyLessParameterNodeImpl() { this = TBodyLessParameterNodeImpl(p, indirectionIndex) }
+
+  override Declaration getEnclosingCallable() { result = this.getFunction() }
+
+  override Declaration getFunction() { result = p.getFunction() }
+
+  /** Gets the indirection index of this node. */
+  int getIndirectionIndex() { result = indirectionIndex }
+
+  override DataFlowType getType() {
+    result = getTypeImpl(p.getUnderlyingType(), this.getIndirectionIndex())
+  }
+
+  final override Location getLocationImpl() {
+    result = unique( | | p.getLocation())
+    or
+    count(p.getLocation()) != 1 and
+    result instanceof UnknownDefaultLocation
+  }
+
+  final override string toStringImpl() {
+    exists(string prefix | prefix = stars(this) | result = prefix + p.toString())
+  }
+}
+
 /**
  * A data-flow node used to model flow summaries. That is, a dataflow node
  * that is synthesized to represent a parameter, return value, or other part
@@ -767,42 +810,6 @@ class FlowSummaryNode extends Node, TFlowSummaryNode {
   override string toStringImpl() { result = this.getSummaryNode().toString() }
 }
 
-/**
- * INTERNAL: do not use.
- *
- * A node representing an indirection of a parameter.
- */
-class IndirectParameterNode extends Node instanceof IndirectInstruction {
-  InitializeParameterInstruction init;
-
-  IndirectParameterNode() { IndirectInstruction.super.hasInstructionAndIndirectionIndex(init, _) }
-
-  int getArgumentIndex() { init.hasIndex(result) }
-
-  /** Gets the parameter whose indirection is initialized. */
-  Parameter getParameter() { result = init.getParameter() }
-
-  override Declaration getEnclosingCallable() { result = this.getFunction() }
-
-  override Declaration getFunction() { result = init.getEnclosingFunction() }
-
-  /** Gets the underlying operand and the underlying indirection index. */
-  predicate hasInstructionAndIndirectionIndex(Instruction instr, int index) {
-    IndirectInstruction.super.hasInstructionAndIndirectionIndex(instr, index)
-  }
-
-  override Location getLocationImpl() { result = this.getParameter().getLocation() }
-
-  override string toStringImpl() {
-    exists(string prefix | prefix = stars(this) |
-      result = prefix + this.getParameter().toString()
-      or
-      not exists(this.getParameter()) and
-      result = prefix + "this"
-    )
-  }
-}
-
 /**
  * INTERNAL: do not use.
  *
@@ -1655,6 +1662,88 @@ class IndirectExprNode extends Node instanceof IndirectExprNodeBase {
   }
 }
 
+abstract private class AbstractParameterNode extends Node {
+  /**
+   * Holds if this node is the parameter of `f` at the specified position. The
+   * implicit `this` parameter is considered to have position `-1`, and
+   * pointer-indirection parameters are at further negative positions.
+   */
+  abstract predicate isParameterOf(DataFlowCallable f, ParameterPosition pos);
+
+  /** Gets the `Parameter` associated with this node, if it exists. */
+  Parameter getParameter() { none() } // overridden by subclasses
+}
+
+abstract private class AbstractIndirectParameterNode extends AbstractParameterNode {
+  /** Gets the indirection index of this parameter node. */
+  abstract int getIndirectionIndex();
+}
+
+/**
+ * INTERNAL: do not use.
+ *
+ * A node representing an indirection of a parameter.
+ */
+final class IndirectParameterNode = AbstractIndirectParameterNode;
+
+pragma[noinline]
+private predicate indirectParameterNodeHasArgumentIndexAndIndex(
+  IndirectInstructionParameterNode node, int argumentIndex, int indirectionIndex
+) {
+  node.hasInstructionAndIndirectionIndex(_, indirectionIndex) and
+  node.getArgumentIndex() = argumentIndex
+}
+
+pragma[noinline]
+private predicate indirectPositionHasArgumentIndexAndIndex(
+  IndirectionPosition pos, int argumentIndex, int indirectionIndex
+) {
+  pos.getArgumentIndex() = argumentIndex and
+  pos.getIndirectionIndex() = indirectionIndex
+}
+
+private class IndirectInstructionParameterNode extends AbstractIndirectParameterNode instanceof IndirectInstruction
+{
+  InitializeParameterInstruction init;
+
+  IndirectInstructionParameterNode() {
+    IndirectInstruction.super.hasInstructionAndIndirectionIndex(init, _)
+  }
+
+  int getArgumentIndex() { init.hasIndex(result) }
+
+  override string toStringImpl() {
+    exists(string prefix | prefix = stars(this) |
+      result = prefix + this.getParameter().toString()
+      or
+      not exists(this.getParameter()) and
+      result = prefix + "this"
+    )
+  }
+
+  /** Gets the parameter whose indirection is initialized. */
+  override Parameter getParameter() { result = init.getParameter() }
+
+  override Declaration getEnclosingCallable() { result = this.getFunction() }
+
+  override Declaration getFunction() { result = init.getEnclosingFunction() }
+
+  override predicate isParameterOf(DataFlowCallable f, ParameterPosition pos) {
+    this.getEnclosingCallable() = f.getUnderlyingCallable() and
+    exists(int argumentIndex, int indirectionIndex |
+      indirectPositionHasArgumentIndexAndIndex(pos, argumentIndex, indirectionIndex) and
+      indirectParameterNodeHasArgumentIndexAndIndex(this, argumentIndex, indirectionIndex)
+    )
+  }
+
+  /** Gets the underlying operand and the underlying indirection index. */
+  predicate hasInstructionAndIndirectionIndex(Instruction instr, int index) {
+    IndirectInstruction.super.hasInstructionAndIndirectionIndex(instr, index)
+  }
+
+  final override int getIndirectionIndex() { this.hasInstructionAndIndirectionIndex(init, result) }
+}
+
 /**
  * The value of a parameter at function entry, viewed as a node in a data
  * flow graph. This includes both explicit parameters such as `x` in `f(x)`
@@ -1664,42 +1753,38 @@ class IndirectExprNode extends Node instanceof IndirectExprNodeBase {
  * `ExplicitParameterNode`, `ThisParameterNode`, or
  * `ParameterIndirectionNode`.
  */
-class ParameterNode extends Node {
-  ParameterNode() {
-    // To avoid making this class abstract, we enumerate its values here
-    this.asInstruction() instanceof InitializeParameterInstruction
-    or
-    this instanceof IndirectParameterNode
-    or
-    FlowSummaryImpl::Private::summaryParameterNode(this.(FlowSummaryNode).getSummaryNode(), _)
-  }
+final class ParameterNode = AbstractParameterNode;
 
-  /**
-   * Holds if this node is the parameter of `f` at the specified position. The
-   * implicit `this` parameter is considered to have position `-1`, and
-   * pointer-indirection parameters are at further negative positions.
-   */
-  predicate isParameterOf(DataFlowCallable f, ParameterPosition pos) { none() } // overridden by subclasses
-
-  /** Gets the `Parameter` associated with this node, if it exists. */
-  Parameter getParameter() { none() } // overridden by subclasses
-}
+abstract private class AbstractDirectParameterNode extends AbstractParameterNode { }
 
 /** An explicit positional parameter, including `this`, but not `...`. */
-class DirectParameterNode extends InstructionNode {
-  override InitializeParameterInstruction instr;
+final class DirectParameterNode = AbstractDirectParameterNode;
+
+/**
+ * INTERNAL: Do not use.
+ *
+ * A non-indirect parameter node that is represented as an `Instruction`.
+ */
+abstract class InstructionDirectParameterNode extends InstructionNode, AbstractDirectParameterNode {
+  final override InitializeParameterInstruction instr;
 
   /**
    * INTERNAL: Do not use.
    *
    * Gets the `IRVariable` that this parameter references.
    */
-  IRVariable getIRVariable() { result = instr.getIRVariable() }
+  final IRVariable getIRVariable() { result = instr.getIRVariable() }
 }
 
+abstract private class AbstractExplicitParameterNode extends AbstractDirectParameterNode { }
+
+final class ExplicitParameterNode = AbstractExplicitParameterNode;
+
 /** An explicit positional parameter, not including `this` or `...`. */
-private class ExplicitParameterNode extends ParameterNode, DirectParameterNode {
-  ExplicitParameterNode() { exists(instr.getParameter()) }
+private class ExplicitParameterInstructionNode extends AbstractExplicitParameterNode,
+  InstructionDirectParameterNode
+{
+  ExplicitParameterInstructionNode() { exists(instr.getParameter()) }
 
   override predicate isParameterOf(DataFlowCallable f, ParameterPosition pos) {
     f.getUnderlyingCallable().(Function).getParameter(pos.(DirectPosition).getIndex()) =
@@ -1712,8 +1797,10 @@ private class ExplicitParameterNode extends ParameterNode, DirectParameterNode {
 }
 
 /** An implicit `this` parameter. */
-class ThisParameterNode extends ParameterNode, DirectParameterNode {
-  ThisParameterNode() { instr.getIRVariable() instanceof IRThisVariable }
+class ThisParameterInstructionNode extends AbstractExplicitParameterNode,
+  InstructionDirectParameterNode
+{
+  ThisParameterInstructionNode() { instr.getIRVariable() instanceof IRThisVariable }
 
   override predicate isParameterOf(DataFlowCallable f, ParameterPosition pos) {
     pos.(DirectPosition).getIndex() = -1 and
@@ -1726,7 +1813,7 @@ class ThisParameterNode extends ParameterNode, DirectParameterNode {
 /**
  * A parameter node that is part of a summary.
  */
-class SummaryParameterNode extends ParameterNode, FlowSummaryNode {
+class SummaryParameterNode extends AbstractParameterNode, FlowSummaryNode {
   SummaryParameterNode() {
     FlowSummaryImpl::Private::summaryParameterNode(this.getSummaryNode(), _)
   }
@@ -1741,31 +1828,41 @@ class SummaryParameterNode extends ParameterNode, FlowSummaryNode {
   }
 }
 
-pragma[noinline]
-private predicate indirectPositionHasArgumentIndexAndIndex(
-  IndirectionPosition pos, int argumentIndex, int indirectionIndex
-) {
-  pos.getArgumentIndex() = argumentIndex and
-  pos.getIndirectionIndex() = indirectionIndex
-}
+private class DirectBodyLessParameterNode extends AbstractExplicitParameterNode,
+  BodyLessParameterNodeImpl
+{
+  DirectBodyLessParameterNode() { indirectionIndex = 0 }
 
-pragma[noinline]
-private predicate indirectParameterNodeHasArgumentIndexAndIndex(
-  IndirectParameterNode node, int argumentIndex, int indirectionIndex
-) {
-  node.hasInstructionAndIndirectionIndex(_, indirectionIndex) and
-  node.getArgumentIndex() = argumentIndex
-}
-
-/** A synthetic parameter to model the pointed-to object of a pointer parameter. */
-class ParameterIndirectionNode extends ParameterNode instanceof IndirectParameterNode {
   override predicate isParameterOf(DataFlowCallable f, ParameterPosition pos) {
-    IndirectParameterNode.super.getEnclosingCallable() = f.getUnderlyingCallable() and
-    exists(int argumentIndex, int indirectionIndex |
-      indirectPositionHasArgumentIndexAndIndex(pos, argumentIndex, indirectionIndex) and
-      indirectParameterNodeHasArgumentIndexAndIndex(this, argumentIndex, indirectionIndex)
+    exists(Function func |
+      this.getFunction() = func and
+      f.asSourceCallable() = func and
+      func.getParameter(pos.(DirectPosition).getIndex()) = p
     )
   }
+
+  override Parameter getParameter() { result = p }
+}
+
+private class IndirectBodyLessParameterNode extends AbstractIndirectParameterNode,
+  BodyLessParameterNodeImpl
+{
+  IndirectBodyLessParameterNode() { not this instanceof DirectBodyLessParameterNode }
+
+  override predicate isParameterOf(DataFlowCallable f, ParameterPosition pos) {
+    exists(Function func, int argumentPosition |
+      this.getFunction() = func and
+      f.asSourceCallable() = func and
+      indirectPositionHasArgumentIndexAndIndex(pos, argumentPosition, indirectionIndex) and
+      func.getParameter(argumentPosition) = p
+    )
+  }
+
+  override int getIndirectionIndex() {
+    result = BodyLessParameterNodeImpl.super.getIndirectionIndex()
+  }
+
+  override Parameter getParameter() { result = p }
 }
 
 /**

cpp/ql/test/library-tests/dataflow/parameters-without-defs/test.cpp

@@ -0,0 +1,11 @@
+void sink(int); // $ ir
+void indirect_sink(int*); // $ ir
+int source();
+
+void test() {
+  int x = source();
+  sink(x);
+
+  int* p = &x;
+  indirect_sink(p);
+}

cpp/ql/test/library-tests/dataflow/parameters-without-defs/test.expected

@@ -0,0 +1,2 @@
+testFailures
+failures

cpp/ql/test/library-tests/dataflow/parameters-without-defs/test.ql

@@ -0,0 +1,16 @@
+import TestUtilities.dataflow.FlowTestCommon
+import semmle.code.cpp.dataflow.new.DataFlow
+
+module ParamConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source.asExpr().(Call).getTarget().hasName("source") }
+
+  predicate isSink(DataFlow::Node sink) {
+    sink.asParameter().getFunction().hasName("sink")
+    or
+    sink.asParameter(1).getFunction().hasName("indirect_sink")
+  }
+}
+
+module IRFlow = DataFlow::Global<ParamConfig>;
+
+import MakeTest<IRFlowTest<IRFlow>>