Python: Do not use BarrierGuards

They are simply not right for this problem.
We should not even make them available as an extension point.

Author: yoff

python/ql/src/semmle/python/functions/ModificationOfParameterWithDefault.qll

@@ -33,16 +33,14 @@ module ModificationOfParameterWithDefault {
 
     override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-    override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
-
-    override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
-      // if we are tracking a empty default, then it is ok to modify truthy values,
+    override predicate isBarrier(DataFlow::Node node) {
+      // if we are tracking a non-empty default, then it is ok to modify empty values,
       // so our tracking ends at those.
-      nonEmptyDefault = false and guard instanceof BlocksTruthy
+      nonEmptyDefault = true and node instanceof MustBeEmpty
       or
-      // if we are tracking a non-empty default, then it is ok to modify falsy values,
+      // if we are tracking a empty default, then it is ok to modify non-empty values,
       // so our tracking ends at those.
-      nonEmptyDefault = true and guard instanceof BlocksFalsey
+      nonEmptyDefault = false and node instanceof MustBeNonEmpty
     }
   }
 }

python/ql/src/semmle/python/functions/ModificationOfParameterWithDefaultCustomizations.qll

@@ -14,48 +14,45 @@ private import semmle.python.dataflow.new.BarrierGuards
  */
 module ModificationOfParameterWithDefault {
   /**
-   * A data flow source for detecting modifications of a parameters default value.
+   * A data flow source for detecting modifications of a parameters default value,
+   * that is a default value for some parameter.
    */
   abstract class Source extends DataFlow::Node {
     /** Result is true if the default value is non-empty for this source and false if not. */
     abstract boolean isNonEmpty();
   }
 
   /**
-   * A data flow sink for detecting modifications of a parameters default value.
+   * A data flow sink for detecting modifications of a parameters default value,
+   * that is a node representing a modification.
    */
   abstract class Sink extends DataFlow::Node { }
 
   /**
-   * A sanitizer for detecting modifications of a parameters default value.
-   */
-  abstract class Barrier extends DataFlow::Node { }
-
-  /**
-   * A sanitizer guard that does not let a truthy value flow to the true branch.
+   * A sanitizer for detecting modifications of a parameters default value
+   * should determine if the node (which is perhaps about to be modified)
+   * can be the default value or not.
    *
-   * Implementation note:
-   * Since guards with different behaviour cannot exist on the same node,
-   * we let all guards have the same behaviour, in the sense that they all check
-   * the true branch. Instead, we partition guards into those that block
-   * truthy values and those that block falsy values.
+   * In this query we do not track the default value exactly, but rather wheter
+   * it is empty or not (see `Source`).
    *
-   * If you extend this class, make sure that your barrier checks the true branch.
+   * This is the extension point for determining that a node must be empty and
+   * therefor is allowed to be modified if the tracked default value is non-empty.
    */
-  abstract class BlocksTruthy extends DataFlow::BarrierGuard { }
+  abstract class MustBeEmpty extends DataFlow::Node { }
 
   /**
-   * A sanitizer guard that does not let a falsy value flow to the true branch.
+   * A sanitizer for detecting modifications of a parameters default value
+   * should determine if the node (which is perhaps about to be modified)
+   * can be the default value or not.
    *
-   * Implementation note:
-   * Since guards with different behaviour cannot exist on the same node,
-   * we let all guards have the same behaviour, in the sense that they all check
-   * the true branch. Instead, we partition guards into those that block
-   * truthy values and those that block falsy values.
+   * In this query we do not track the default value exactly, but rather wheter
+   * it is empty or not (see `Source`).
    *
-   * If you extend this class, make sure that your barrier checks the true branch.
+   * This is the extension point for determining that a node must be non-empty
+   * and therefor is allowed to be modified if the tracked default value is empty.
    */
-  abstract class BlocksFalsey extends DataFlow::BarrierGuard { }
+  abstract class MustBeNonEmpty extends DataFlow::Node { }
 
   /** Gets the truthiness (non emptyness) of the default of `p` if that value is mutable */
   private boolean mutableDefaultValue(Parameter p) {
@@ -140,46 +137,48 @@ module ModificationOfParameterWithDefault {
     }
   }
 
-  /**
-   * A simple sanitizer guard that does not let a truthy value flow to the true branch.
-   *
-   * Blocks flow of `x` in the true branch in the example below.
-   * ```py
-   * if x:
-   *     x.append(42)
-   * ```
-   */
-  class BlocksTruthyGuard extends BlocksTruthy {
-    ControlFlowNode guarded;
-
-    BlocksTruthyGuard() { this instanceof NameNode }
-
-    override predicate checks(ControlFlowNode node, boolean branch) {
-      node = this and
-      branch = true
-    }
-  }
+  // This to reimplement some of the functionality of the DataFlow::BarrierGuard
+  private import semmle.python.essa.SsaCompute
 
   /**
-   * A simple sanitizer guard that does not let a truthy value flow to the true branch.
+   * Simple detection of truthy and falsey values.
    *
-   * Blocks flow of `x` in the true branch in the example below.
-   * ```py
-   * if not x:
-   *     x.append(42)
-   * ```
+   * It handles the cases `if x` and `if not x`.
    */
-  class BlocksFalseyGuard extends BlocksFalsey {
+  private class MustBe extends DataFlow::Node {
+    DataFlow::GuardNode guard;
     NameNode guarded;
-
-    BlocksFalseyGuard() {
-      this.(UnaryExprNode).getNode().getOp() instanceof Not and
-      guarded = this.(UnaryExprNode).getOperand()
+    boolean branch;
+    boolean truthy;
+
+    MustBe() {
+      (
+        // case: if x
+        guard = guarded and
+        branch = truthy
+        or
+        // case: if not x
+        guard.(UnaryExprNode).getNode().getOp() instanceof Not and
+        guarded = guard.(UnaryExprNode).getOperand() and
+        branch = truthy.booleanNot()
+      ) and
+      // guard controls this
+      guard.controlsBlock(this.asCfgNode().getBasicBlock(), branch) and
+      // there is a definition tying the guarded value to this
+      exists(EssaDefinition def |
+        AdjacentUses::useOfDef(def, this.asCfgNode()) and
+        AdjacentUses::useOfDef(def, guarded)
+      )
     }
+  }
 
-    override predicate checks(ControlFlowNode node, boolean branch) {
-      node = guarded and
-      branch = true
-    }
+  /** Simple guard detecting truthy values. */
+  private class MustBeTruthy extends MustBe, MustBeNonEmpty {
+    MustBeTruthy() { truthy = true }
+  }
+
+  /** Simple guard detecting falsey values. */
+  private class MustBeFalsey extends MustBe, MustBeEmpty {
+    MustBeFalsey() { truthy = false }
   }
 }

python/ql/test/query-tests/Functions/ModificationOfParameterWithDefault/ModificationOfParameterWithDefault.expected

@@ -27,9 +27,7 @@ edges
 | test.py:124:15:124:15 | ControlFlowNode for l | test.py:128:9:128:9 | ControlFlowNode for l |
 | test.py:131:23:131:23 | ControlFlowNode for l | test.py:135:9:135:9 | ControlFlowNode for l |
 | test.py:138:15:138:15 | ControlFlowNode for l | test.py:140:9:140:9 | ControlFlowNode for l |
-| test.py:138:15:138:15 | ControlFlowNode for l | test.py:142:9:142:9 | ControlFlowNode for l |
 | test.py:145:23:145:23 | ControlFlowNode for l | test.py:147:9:147:9 | ControlFlowNode for l |
-| test.py:145:23:145:23 | ControlFlowNode for l | test.py:149:9:149:9 | ControlFlowNode for l |
 nodes
 | test.py:2:12:2:12 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
 | test.py:3:5:3:5 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
@@ -81,10 +79,8 @@ nodes
 | test.py:135:9:135:9 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
 | test.py:138:15:138:15 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
 | test.py:140:9:140:9 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
-| test.py:142:9:142:9 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
 | test.py:145:23:145:23 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
 | test.py:147:9:147:9 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
-| test.py:149:9:149:9 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
 #select
 | test.py:3:5:3:5 | ControlFlowNode for l | test.py:2:12:2:12 | ControlFlowNode for l | test.py:3:5:3:5 | ControlFlowNode for l | $@ flows to here and is mutated. | test.py:2:12:2:12 | ControlFlowNode for l | Default value |
 | test.py:8:5:8:5 | ControlFlowNode for l | test.py:7:11:7:11 | ControlFlowNode for l | test.py:8:5:8:5 | ControlFlowNode for l | $@ flows to here and is mutated. | test.py:7:11:7:11 | ControlFlowNode for l | Default value |
@@ -108,6 +104,4 @@ nodes
 | test.py:128:9:128:9 | ControlFlowNode for l | test.py:124:15:124:15 | ControlFlowNode for l | test.py:128:9:128:9 | ControlFlowNode for l | $@ flows to here and is mutated. | test.py:124:15:124:15 | ControlFlowNode for l | Default value |
 | test.py:135:9:135:9 | ControlFlowNode for l | test.py:131:23:131:23 | ControlFlowNode for l | test.py:135:9:135:9 | ControlFlowNode for l | $@ flows to here and is mutated. | test.py:131:23:131:23 | ControlFlowNode for l | Default value |
 | test.py:140:9:140:9 | ControlFlowNode for l | test.py:138:15:138:15 | ControlFlowNode for l | test.py:140:9:140:9 | ControlFlowNode for l | $@ flows to here and is mutated. | test.py:138:15:138:15 | ControlFlowNode for l | Default value |
-| test.py:142:9:142:9 | ControlFlowNode for l | test.py:138:15:138:15 | ControlFlowNode for l | test.py:142:9:142:9 | ControlFlowNode for l | $@ flows to here and is mutated. | test.py:138:15:138:15 | ControlFlowNode for l | Default value |
 | test.py:147:9:147:9 | ControlFlowNode for l | test.py:145:23:145:23 | ControlFlowNode for l | test.py:147:9:147:9 | ControlFlowNode for l | $@ flows to here and is mutated. | test.py:145:23:145:23 | ControlFlowNode for l | Default value |
-| test.py:149:9:149:9 | ControlFlowNode for l | test.py:145:23:145:23 | ControlFlowNode for l | test.py:149:9:149:9 | ControlFlowNode for l | $@ flows to here and is mutated. | test.py:145:23:145:23 | ControlFlowNode for l | Default value |

python/ql/test/query-tests/Functions/ModificationOfParameterWithDefault/test.py

@@ -139,12 +139,12 @@ def sanitizer(l = []):
     if not l:
         l.append(1)  #$ modification=l
     else:
-        l.append(1)  #$ SPURIOUS: modification=l
+        l.append(1)
     return l
 
 def sanitizer_negated(l = [1]):
     if l:
         l.append(1)  #$ modification=l
     else:
-        l.append(1)  #$ SPURIOUS: modification=l
+        l.append(1)
     return l