Merge branch 'main' into alexdenisov+redsun82/tuple-mangling

Author: redsun82

.github/workflows/check-change-note.yml

@@ -11,7 +11,6 @@ on:
       - "*/ql/lib/**/*.yml"
       - "!**/experimental/**"
       - "!ql/**"
-      - "!swift/**"
       - ".github/workflows/check-change-note.yml"
 
 jobs:
@@ -32,4 +31,4 @@ jobs:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         run: |
           gh api 'repos/${{github.repository}}/pulls/${{github.event.number}}/files' --paginate --jq '[.[].filename | select(test("/change-notes/.*[.]md$"))] | all(test("/change-notes/[0-9]{4}-[0-9]{2}-[0-9]{2}.*[.]md$") or test("/change-notes/released/[0-9]*[.][0-9]*[.][0-9]*[.]md$"))' |
-          grep true -c 
+          grep true -c

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

@@ -1640,8 +1640,15 @@ predicate localInstructionFlow(Instruction e1, Instruction e2) {
   localFlow(instructionNode(e1), instructionNode(e2))
 }
 
+/**
+ * INTERNAL: Do not use.
+ *
+ * Ideally this module would be private, but the `asExprInternal` predicate is
+ * needed in `DefaultTaintTrackingImpl`. Once `DefaultTaintTrackingImpl` is gone
+ * we can make this module private.
+ */
 cached
-private module ExprFlowCached {
+module ExprFlowCached {
   /**
    * Holds if `n` is an indirect operand of a `PointerArithmeticInstruction`, and
    * `e` is the result of loading from the `PointerArithmeticInstruction`.
@@ -1692,7 +1699,8 @@ private module ExprFlowCached {
    * `x[i]` steps to the expression `x[i - 1]` without traversing the
    * entire chain.
    */
-  private Expr asExpr(Node n) {
+  cached
+  Expr asExprInternal(Node n) {
     isIndirectBaseOfArrayAccess(n, result)
     or
     not isIndirectBaseOfArrayAccess(n, _) and
@@ -1704,7 +1712,7 @@ private module ExprFlowCached {
    * dataflow step.
    */
   private predicate localStepFromNonExpr(Node n1, Node n2) {
-    not exists(asExpr(n1)) and
+    not exists(asExprInternal(n1)) and
     localFlowStep(n1, n2)
   }
 
@@ -1715,7 +1723,7 @@ private module ExprFlowCached {
   pragma[nomagic]
   private predicate localStepsToExpr(Node n1, Node n2, Expr e2) {
     localStepFromNonExpr*(n1, n2) and
-    e2 = asExpr(n2)
+    e2 = asExprInternal(n2)
   }
 
   /**
@@ -1726,7 +1734,7 @@ private module ExprFlowCached {
     exists(Node mid |
       localFlowStep(n1, mid) and
       localStepsToExpr(mid, n2, e2) and
-      e1 = asExpr(n1)
+      e1 = asExprInternal(n1)
     )
   }
 

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

@@ -60,7 +60,7 @@ private DataFlow::Node getNodeForSource(Expr source) {
 }
 
 private DataFlow::Node getNodeForExpr(Expr node) {
-  result = DataFlow::exprNode(node)
+  node = DataFlow::ExprFlowCached::asExprInternal(result)
   or
   // Some of the sources in `isUserInput` are intended to match the value of
   // an expression, while others (those modeled below) are intended to match
@@ -221,7 +221,7 @@ private module Cached {
   predicate nodeIsBarrierIn(DataFlow::Node node) {
     // don't use dataflow into taint sources, as this leads to duplicate results.
     exists(Expr source | isUserInput(source, _) |
-      node = DataFlow::exprNode(source)
+      source = DataFlow::ExprFlowCached::asExprInternal(node)
       or
       // This case goes together with the similar (but not identical) rule in
       // `getNodeForSource`.

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/analysis/RangeAnalysisStage.qll

@@ -729,7 +729,7 @@ module RangeStage<
   ) {
     exists(SemExpr e, D::Delta d1, D::Delta d2 |
       unequalFlowStepIntegralSsa(v, pos, e, d1, reason) and
-      boundedUpper(e, b, d1) and
+      boundedUpper(e, b, d2) and
       boundedLower(e, b, d2) and
       delta = D::fromFloat(D::toFloat(d1) + D::toFloat(d2))
     )

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/test.cpp

@@ -78,3 +78,36 @@ void testInterproc(BigArray *arr) {
 
     addToPointerAndAssign(arr->buf);
 }
+
+void testEqRefinement() {
+    int arr[MAX_SIZE];
+
+    for(int i = 0; i <= MAX_SIZE; i++) {
+        if(i != MAX_SIZE) {
+            arr[i] = 0; // GOOD
+        }
+    }
+}
+
+void testEqRefinement2() {
+    int arr[MAX_SIZE];
+
+    int n = 0;
+
+    for(int i = 0; i <= MAX_SIZE; i++) {
+        if(n == 0) {
+            if(i == MAX_SIZE) {
+                break;
+            }
+            n = arr[i]; // GOOD
+            continue;
+        }
+
+        if (i == MAX_SIZE || n != arr[i]) {
+            if (i == MAX_SIZE) {
+                break;
+            }
+            n = arr[i]; // GOOD
+        }
+    }
+}

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected

@@ -653,7 +653,24 @@ edges
 | test.cpp:304:15:304:26 | new[] | test.cpp:308:5:308:6 | xs |
 | test.cpp:308:5:308:6 | xs | test.cpp:308:5:308:11 | access to array |
 | test.cpp:308:5:308:11 | access to array | test.cpp:308:5:308:29 | Store: ... = ... |
-| test.cpp:313:16:313:29 | new[] | test.cpp:314:17:314:18 | xs |
+| test.cpp:313:14:313:27 | new[] | test.cpp:314:15:314:16 | xs |
+| test.cpp:325:14:325:27 | new[] | test.cpp:326:15:326:16 | xs |
+| test.cpp:326:15:326:16 | xs | test.cpp:326:15:326:23 | ... + ... |
+| test.cpp:326:15:326:16 | xs | test.cpp:326:15:326:23 | ... + ... |
+| test.cpp:326:15:326:16 | xs | test.cpp:338:8:338:15 | * ... |
+| test.cpp:326:15:326:16 | xs | test.cpp:341:8:341:17 | * ... |
+| test.cpp:326:15:326:23 | ... + ... | test.cpp:342:8:342:17 | * ... |
+| test.cpp:326:15:326:23 | ... + ... | test.cpp:342:8:342:17 | * ... |
+| test.cpp:338:8:338:15 | * ... | test.cpp:342:8:342:17 | * ... |
+| test.cpp:341:8:341:17 | * ... | test.cpp:342:8:342:17 | * ... |
+| test.cpp:342:8:342:17 | * ... | test.cpp:333:5:333:21 | Store: ... = ... |
+| test.cpp:342:8:342:17 | * ... | test.cpp:341:5:341:21 | Store: ... = ... |
+| test.cpp:347:14:347:27 | new[] | test.cpp:348:15:348:16 | xs |
+| test.cpp:348:15:348:16 | xs | test.cpp:350:16:350:19 | ... ++ |
+| test.cpp:348:15:348:16 | xs | test.cpp:350:16:350:19 | ... ++ |
+| test.cpp:350:16:350:19 | ... ++ | test.cpp:350:15:350:19 | Load: * ... |
+| test.cpp:350:16:350:19 | ... ++ | test.cpp:350:16:350:19 | ... ++ |
+| test.cpp:350:16:350:19 | ... ++ | test.cpp:350:16:350:19 | ... ++ |
 subpaths
 #select
 | test.cpp:6:14:6:15 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:6:14:6:15 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
@@ -679,3 +696,6 @@ subpaths
 | test.cpp:264:13:264:14 | Load: * ... | test.cpp:260:13:260:24 | new[] | test.cpp:264:13:264:14 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:260:13:260:24 | new[] | new[] | test.cpp:261:19:261:21 | len | len |
 | test.cpp:274:5:274:10 | Store: ... = ... | test.cpp:270:13:270:24 | new[] | test.cpp:274:5:274:10 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:270:13:270:24 | new[] | new[] | test.cpp:271:19:271:21 | len | len |
 | test.cpp:308:5:308:29 | Store: ... = ... | test.cpp:304:15:304:26 | new[] | test.cpp:308:5:308:29 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:304:15:304:26 | new[] | new[] | test.cpp:308:8:308:10 | ... + ... | ... + ... |
+| test.cpp:333:5:333:21 | Store: ... = ... | test.cpp:325:14:325:27 | new[] | test.cpp:333:5:333:21 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:325:14:325:27 | new[] | new[] | test.cpp:326:20:326:23 | size | size |
+| test.cpp:341:5:341:21 | Store: ... = ... | test.cpp:325:14:325:27 | new[] | test.cpp:341:5:341:21 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:325:14:325:27 | new[] | new[] | test.cpp:326:20:326:23 | size | size |
+| test.cpp:350:15:350:19 | Load: * ... | test.cpp:347:14:347:27 | new[] | test.cpp:350:15:350:19 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:347:14:347:27 | new[] | new[] | test.cpp:348:20:348:23 | size | size |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp

@@ -310,15 +310,43 @@ void test21() {
 }
 
 void test22(unsigned size, int val) {
-    char *xs = new char[size];
-    char *end = xs + size; // GOOD
-    char **current = &end;
-    do
-    {
-        if( *current - xs < 1 ) // GOOD
-            return;
-        *--(*current) = 0; // GOOD
-        val >>= 8;
-    }
-    while( val > 0 );
+  char *xs = new char[size];
+  char *end = xs + size; // GOOD
+  char **current = &end;
+  do {
+    if (*current - xs < 1) // GOOD
+      return;
+    *--(*current) = 0; // GOOD
+      val >>= 8;
+  } while (val > 0);
+}
+
+void test23(unsigned size, int val) {
+  char *xs = new char[size];
+  char *end = xs + size;
+  char **current = &end;
+
+  if (val < 1) {
+    if(*current - xs < 1)
+      return;
+
+    *--(*current) = 0; // GOOD [FALSE POSITIVE]
+    return;
+  }
+
+  if (val < 2) {
+    if(*current - xs < 2)
+      return;
+
+    *--(*current) = 0; // GOOD [FALSE POSITIVE]
+    *--(*current) = 0; // GOOD
+  }
+}
+
+void test24(unsigned size) {
+  char *xs = new char[size];
+  char *end = xs + size;
+  if (xs < end) {
+    int val = *xs++; // GOOD [FALSE POSITIVE]
+  }
 }

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_path_to_sink/tainted.expected

@@ -1,2 +1,4 @@
 WARNING: Module TaintedWithPath has been deprecated and may be removed in future (tainted.ql:9,8-47)
 WARNING: Predicate tainted has been deprecated and may be removed in future (tainted.ql:20,49-74)
+failures
+testFailures

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_path_to_sink/tainted.ql

@@ -38,12 +38,10 @@ predicate irTaint(Element source, TaintedWithPath::PathNode predNode, string tag
   )
 }
 
-class IRDefaultTaintTrackingTest extends InlineExpectationsTest {
-  IRDefaultTaintTrackingTest() { this = "IRDefaultTaintTrackingTest" }
+module IRDefaultTaintTrackingTest implements TestSig {
+  string getARelevantTag() { result = ["ir-path", "ir-sink"] }
 
-  override string getARelevantTag() { result = ["ir-path", "ir-sink"] }
-
-  override predicate hasActualResult(Location location, string element, string tag, string value) {
+  predicate hasActualResult(Location location, string element, string tag, string value) {
     exists(Element elem, TaintedWithPath::PathNode node, int n |
       irTaint(_, node, tag) and
       elem = getElementFromPathNode(node) and
@@ -67,12 +65,10 @@ class IRDefaultTaintTrackingTest extends InlineExpectationsTest {
   }
 }
 
-class AstTaintTrackingTest extends InlineExpectationsTest {
-  AstTaintTrackingTest() { this = "ASTTaintTrackingTest" }
-
-  override string getARelevantTag() { result = "ast" }
+module AstTaintTrackingTest implements TestSig {
+  string getARelevantTag() { result = "ast" }
 
-  override predicate hasActualResult(Location location, string element, string tag, string value) {
+  predicate hasActualResult(Location location, string element, string tag, string value) {
     exists(Expr source, Element tainted, int n |
       tag = "ast" and
       astTaint(source, tainted) and
@@ -100,3 +96,5 @@ class AstTaintTrackingTest extends InlineExpectationsTest {
     )
   }
 }
+
+import MakeTest<MergeTests<IRDefaultTaintTrackingTest, AstTaintTrackingTest>>

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_sinks_only/tainted.expected

@@ -1,2 +1,4 @@
 WARNING: Module TaintedWithPath has been deprecated and may be removed in future (tainted.ql:10,8-47)
 WARNING: Predicate tainted has been deprecated and may be removed in future (tainted.ql:21,3-28)
+failures
+testFailures