Merge pull request github#7237 from hvitved/dataflow/consistency-config

Data flow: Introduce `ConsistencyConfiguration` class
davidvenegasr · Nov 26, 2021 · 00ee34c · 00ee34c
2 parents 57fd397 + 6cb0099
commit 00ee34c
Show file tree

Hide file tree

Showing 13 changed files with 136 additions and 82 deletions.
diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowImplConsistency.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowImplConsistency.qll
@@ -9,6 +9,19 @@ private import tainttracking1.TaintTrackingParameter::Private
 private import tainttracking1.TaintTrackingParameter::Public
 
 module Consistency {
+  private newtype TConsistencyConfiguration = MkConsistencyConfiguration()
+
+  /** A class for configuring the consistency queries. */
+  class ConsistencyConfiguration extends TConsistencyConfiguration {
+    string toString() { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `postWithInFlow`. */
+    predicate postWithInFlowExclude(Node n) { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `argHasPostUpdate`. */
+    predicate argHasPostUpdateExclude(ArgumentNode n) { none() }
+  }
+
   private class RelevantNode extends Node {
     RelevantNode() {
       this instanceof ArgumentNode or
@@ -164,7 +177,7 @@ module Consistency {
 
   query predicate argHasPostUpdate(ArgumentNode n, string msg) {
     not hasPost(n) and
-    not isImmutableOrUnobservable(n) and
+    not any(ConsistencyConfiguration c).argHasPostUpdateExclude(n) and
     msg = "ArgumentNode is missing PostUpdateNode."
   }
 
@@ -177,6 +190,7 @@ module Consistency {
     isPostUpdateNode(n) and
     not clearsContent(n, _) and
     simpleLocalFlowStep(_, n) and
+    not any(ConsistencyConfiguration c).postWithInFlowExclude(n) and
     msg = "PostUpdateNode should not be the target of local flow."
   }
 }
diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowPrivate.qll
@@ -2,6 +2,7 @@ private import cpp
 private import DataFlowUtil
 private import DataFlowDispatch
 private import FlowVar
+private import DataFlowImplConsistency
 
 /** Gets the callable in which this node occurs. */
 DataFlowCallable nodeGetEnclosingCallable(Node n) { result = n.getEnclosingCallable() }
@@ -259,27 +260,6 @@ class Unit extends TUnit {
   string toString() { result = "unit" }
 }
 
-/**
- * Holds if `n` does not require a `PostUpdateNode` as it either cannot be
- * modified or its modification cannot be observed, for example if it is a
- * freshly created object that is not saved in a variable.
- *
- * This predicate is only used for consistency checks.
- */
-predicate isImmutableOrUnobservable(Node n) {
-  // Is the null pointer (or something that's not really a pointer)
-  exists(n.asExpr().getValue())
-  or
-  // Isn't a pointer or is a pointer to const
-  forall(DerivedType dt | dt = n.asExpr().getActualType() |
-    dt.getBaseType().isConst()
-    or
-    dt.getBaseType() instanceof RoutineType
-  )
-  // The above list of cases isn't exhaustive, but it narrows down the
-  // consistency alerts enough that most of them are interesting.
-}
-
 /** Holds if `n` should be hidden from path explanations. */
 predicate nodeIsHidden(Node n) { none() }
 
@@ -302,3 +282,19 @@ predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preserves
  * by default as a heuristic.
  */
 predicate allowParameterReturnInSelf(ParameterNode p) { none() }
+
+private class MyConsistencyConfiguration extends Consistency::ConsistencyConfiguration {
+  override predicate argHasPostUpdateExclude(ArgumentNode n) {
+    // Is the null pointer (or something that's not really a pointer)
+    exists(n.asExpr().getValue())
+    or
+    // Isn't a pointer or is a pointer to const
+    forall(DerivedType dt | dt = n.asExpr().getActualType() |
+      dt.getBaseType().isConst()
+      or
+      dt.getBaseType() instanceof RoutineType
+    )
+    // The above list of cases isn't exhaustive, but it narrows down the
+    // consistency alerts enough that most of them are interesting.
+  }
+}
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowImplConsistency.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowImplConsistency.qll
@@ -9,6 +9,19 @@ private import tainttracking1.TaintTrackingParameter::Private
 private import tainttracking1.TaintTrackingParameter::Public
 
 module Consistency {
+  private newtype TConsistencyConfiguration = MkConsistencyConfiguration()
+
+  /** A class for configuring the consistency queries. */
+  class ConsistencyConfiguration extends TConsistencyConfiguration {
+    string toString() { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `postWithInFlow`. */
+    predicate postWithInFlowExclude(Node n) { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `argHasPostUpdate`. */
+    predicate argHasPostUpdateExclude(ArgumentNode n) { none() }
+  }
+
   private class RelevantNode extends Node {
     RelevantNode() {
       this instanceof ArgumentNode or
@@ -164,7 +177,7 @@ module Consistency {
 
   query predicate argHasPostUpdate(ArgumentNode n, string msg) {
     not hasPost(n) and
-    not isImmutableOrUnobservable(n) and
+    not any(ConsistencyConfiguration c).argHasPostUpdateExclude(n) and
     msg = "ArgumentNode is missing PostUpdateNode."
   }
 
@@ -177,6 +190,7 @@ module Consistency {
     isPostUpdateNode(n) and
     not clearsContent(n, _) and
     simpleLocalFlowStep(_, n) and
+    not any(ConsistencyConfiguration c).postWithInFlowExclude(n) and
     msg = "PostUpdateNode should not be the target of local flow."
   }
 }
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
@@ -2,6 +2,7 @@ private import cpp
 private import DataFlowUtil
 private import semmle.code.cpp.ir.IR
 private import DataFlowDispatch
+private import DataFlowImplConsistency
 
 /** Gets the callable in which this node occurs. */
 DataFlowCallable nodeGetEnclosingCallable(Node n) { result = n.getEnclosingCallable() }
@@ -285,19 +286,6 @@ class Unit extends TUnit {
   string toString() { result = "unit" }
 }
 
-/**
- * Holds if `n` does not require a `PostUpdateNode` as it either cannot be
- * modified or its modification cannot be observed, for example if it is a
- * freshly created object that is not saved in a variable.
- *
- * This predicate is only used for consistency checks.
- */
-predicate isImmutableOrUnobservable(Node n) {
-  // The rules for whether an IR argument gets a post-update node are too
-  // complex to model here.
-  any()
-}
-
 /** Holds if `n` should be hidden from path explanations. */
 predicate nodeIsHidden(Node n) {
   n instanceof OperandNode and not n instanceof ArgumentNode
@@ -330,3 +318,11 @@ predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preserves
  * by default as a heuristic.
  */
 predicate allowParameterReturnInSelf(ParameterNode p) { none() }
+
+private class MyConsistencyConfiguration extends Consistency::ConsistencyConfiguration {
+  override predicate argHasPostUpdateExclude(ArgumentNode n) {
+    // The rules for whether an IR argument gets a post-update node are too
+    // complex to model here.
+    any()
+  }
+}
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImplConsistency.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImplConsistency.qll
@@ -9,6 +9,19 @@ private import tainttracking1.TaintTrackingParameter::Private
 private import tainttracking1.TaintTrackingParameter::Public
 
 module Consistency {
+  private newtype TConsistencyConfiguration = MkConsistencyConfiguration()
+
+  /** A class for configuring the consistency queries. */
+  class ConsistencyConfiguration extends TConsistencyConfiguration {
+    string toString() { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `postWithInFlow`. */
+    predicate postWithInFlowExclude(Node n) { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `argHasPostUpdate`. */
+    predicate argHasPostUpdateExclude(ArgumentNode n) { none() }
+  }
+
   private class RelevantNode extends Node {
     RelevantNode() {
       this instanceof ArgumentNode or
@@ -164,7 +177,7 @@ module Consistency {
 
   query predicate argHasPostUpdate(ArgumentNode n, string msg) {
     not hasPost(n) and
-    not isImmutableOrUnobservable(n) and
+    not any(ConsistencyConfiguration c).argHasPostUpdateExclude(n) and
     msg = "ArgumentNode is missing PostUpdateNode."
   }
 
@@ -177,6 +190,7 @@ module Consistency {
     isPostUpdateNode(n) and
     not clearsContent(n, _) and
     simpleLocalFlowStep(_, n) and
+    not any(ConsistencyConfiguration c).postWithInFlowExclude(n) and
     msg = "PostUpdateNode should not be the target of local flow."
   }
 }
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll
@@ -1947,15 +1947,6 @@ class Unit extends TUnit {
   string toString() { result = "unit" }
 }
 
-/**
- * Holds if `n` does not require a `PostUpdateNode` as it either cannot be
- * modified or its modification cannot be observed, for example if it is a
- * freshly created object that is not saved in a variable.
- *
- * This predicate is only used for consistency checks.
- */
-predicate isImmutableOrUnobservable(Node n) { none() }
-
 class LambdaCallKind = Unit;
 
 /** Holds if `creation` is an expression that creates a delegate for `c`. */

diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowImplConsistency.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowImplConsistency.qll
@@ -9,6 +9,19 @@ private import tainttracking1.TaintTrackingParameter::Private
 private import tainttracking1.TaintTrackingParameter::Public
 
 module Consistency {
+  private newtype TConsistencyConfiguration = MkConsistencyConfiguration()
+
+  /** A class for configuring the consistency queries. */
+  class ConsistencyConfiguration extends TConsistencyConfiguration {
+    string toString() { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `postWithInFlow`. */
+    predicate postWithInFlowExclude(Node n) { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `argHasPostUpdate`. */
+    predicate argHasPostUpdateExclude(ArgumentNode n) { none() }
+  }
+
   private class RelevantNode extends Node {
     RelevantNode() {
       this instanceof ArgumentNode or
@@ -164,7 +177,7 @@ module Consistency {
 
   query predicate argHasPostUpdate(ArgumentNode n, string msg) {
     not hasPost(n) and
-    not isImmutableOrUnobservable(n) and
+    not any(ConsistencyConfiguration c).argHasPostUpdateExclude(n) and
     msg = "ArgumentNode is missing PostUpdateNode."
   }
 
@@ -177,6 +190,7 @@ module Consistency {
     isPostUpdateNode(n) and
     not clearsContent(n, _) and
     simpleLocalFlowStep(_, n) and
+    not any(ConsistencyConfiguration c).postWithInFlowExclude(n) and
     msg = "PostUpdateNode should not be the target of local flow."
   }
 }
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll
@@ -8,6 +8,7 @@ private import ContainerFlow
 private import semmle.code.java.dataflow.FlowSteps
 private import semmle.code.java.dataflow.FlowSummary
 private import FlowSummaryImpl as FlowSummaryImpl
+private import DataFlowImplConsistency
 import DataFlowNodes::Private
 
 private newtype TReturnKind = TNormalReturnKind()
@@ -365,17 +366,6 @@ predicate forceHighPrecision(Content c) {
   c instanceof ArrayContent or c instanceof CollectionContent
 }
 
-/**
- * Holds if `n` does not require a `PostUpdateNode` as it either cannot be
- * modified or its modification cannot be observed, for example if it is a
- * freshly created object that is not saved in a variable.
- *
- * This predicate is only used for consistency checks.
- */
-predicate isImmutableOrUnobservable(Node n) {
-  n.getType() instanceof ImmutableType or n instanceof ImplicitVarargsArray
-}
-
 /** Holds if `n` should be hidden from path explanations. */
 predicate nodeIsHidden(Node n) {
   n instanceof SummaryNode
@@ -420,3 +410,9 @@ predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preserves
 predicate allowParameterReturnInSelf(ParameterNode p) {
   FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(p)
 }
+
+private class MyConsistencyConfiguration extends Consistency::ConsistencyConfiguration {
+  override predicate argHasPostUpdateExclude(ArgumentNode n) {
+    n.getType() instanceof ImmutableType or n instanceof ImplicitVarargsArray
+  }
+}
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowImplConsistency.qll b/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowImplConsistency.qll
@@ -9,6 +9,19 @@ private import tainttracking1.TaintTrackingParameter::Private
 private import tainttracking1.TaintTrackingParameter::Public
 
 module Consistency {
+  private newtype TConsistencyConfiguration = MkConsistencyConfiguration()
+
+  /** A class for configuring the consistency queries. */
+  class ConsistencyConfiguration extends TConsistencyConfiguration {
+    string toString() { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `postWithInFlow`. */
+    predicate postWithInFlowExclude(Node n) { none() }
+
+    /** Holds if `n` should be excluded from the consistency test `argHasPostUpdate`. */
+    predicate argHasPostUpdateExclude(ArgumentNode n) { none() }
+  }
+
   private class RelevantNode extends Node {
     RelevantNode() {
       this instanceof ArgumentNode or
@@ -164,7 +177,7 @@ module Consistency {
 
   query predicate argHasPostUpdate(ArgumentNode n, string msg) {
     not hasPost(n) and
-    not isImmutableOrUnobservable(n) and
+    not any(ConsistencyConfiguration c).argHasPostUpdateExclude(n) and
     msg = "ArgumentNode is missing PostUpdateNode."
   }
 
@@ -177,6 +190,7 @@ module Consistency {
     isPostUpdateNode(n) and
     not clearsContent(n, _) and
     simpleLocalFlowStep(_, n) and
+    not any(ConsistencyConfiguration c).postWithInFlowExclude(n) and
     msg = "PostUpdateNode should not be the target of local flow."
   }
 }
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll b/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll
@@ -1649,18 +1649,6 @@ DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) {
  */
 predicate mayBenefitFromCallContext(DataFlowCall call, DataFlowCallable c) { none() }
 
-//--------
-// Misc
-//--------
-/**
- * Holds if `n` does not require a `PostUpdateNode` as it either cannot be
- * modified or its modification cannot be observed, for example if it is a
- * freshly created object that is not saved in a variable.
- *
- * This predicate is only used for consistency checks.
- */
-predicate isImmutableOrUnobservable(Node n) { none() }
-
 int accessPathLimit() { result = 5 }
 
 /**

diff --git a/ruby/ql/consistency-queries/DataFlowConsistency.ql b/ruby/ql/consistency-queries/DataFlowConsistency.ql
@@ -1 +1,13 @@
+import codeql.ruby.DataFlow::DataFlow
+import codeql.ruby.dataflow.internal.DataFlowPrivate
 import codeql.ruby.dataflow.internal.DataFlowImplConsistency::Consistency
+
+private class MyConsistencyConfiguration extends ConsistencyConfiguration {
+  override predicate postWithInFlowExclude(Node n) { n instanceof SummaryNode }
+
+  override predicate argHasPostUpdateExclude(ArgumentNode n) {
+    n instanceof BlockArgumentNode
+    or
+    n instanceof SummaryNode
+  }
+}