CTR55-CPP: model precise end calculation checks and switch to use of size bounds check only

Author: knewbury01

change_notes/2024-03-22-fix-fp-ctr55-cpp.md

@@ -1,2 +1,2 @@
 - `CTR55-CPP` - `DoNotUseAnAdditiveOperatorOnAnIterator.ql`:
-  - Address reported FP in #374. Improve logic on valid end checks on iterators.
+  - Address reported FP in #374. Improve logic on valid end checks and size checks on iterators.

cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql

@@ -17,37 +17,53 @@ import codingstandards.cpp.Iterators
 import semmle.code.cpp.controlflow.Dominance
 
 /**
- * any `.size()` check above our access
+ * something like:
+ * `end = begin() + size()`
  */
-predicate sizeCheckedAbove(ContainerIteratorAccess it, IteratorSource source) {
-  exists(ContainerAccessWithoutRangeCheck::ContainerSizeCall guardCall |
-    strictlyDominates(guardCall, it) and
-    //make sure its the same container providing its size as giving the iterator
-    globalValueNumber(guardCall.getQualifier()) = globalValueNumber(source.getQualifier()) and
-    // and the size call we match must be after the assignment call
-    source.getASuccessor*() = guardCall
+Expr calculatedEndCheck(AdditiveOperatorFunctionCall calc) {
+  exists(
+    ContainerAccessWithoutRangeCheck::ContainerSizeCall size,
+    ContainerAccessWithoutRangeCheck::ContainerBeginCall begin
+  |
+    calc.getTarget().hasName("operator+") and
+    DataFlow::localFlow(DataFlow::exprNode(size), DataFlow::exprNode(calc.getAChild*())) and
+    DataFlow::localFlow(DataFlow::exprNode(begin), DataFlow::exprNode(calc.getAChild*())) and
+    //make sure its the same container providing its size as giving the begin
+    globalValueNumber(begin.getQualifier()) = globalValueNumber(size.getQualifier()) and
+    result = begin.getQualifier()
   )
 }
 
+Expr validEndCheck(FunctionCall end) {
+  end instanceof ContainerAccessWithoutRangeCheck::ContainerEndCall and
+  result = end.getQualifier()
+  or
+  result = calculatedEndCheck(end)
+}
+
 /**
  * some guard exists like: `iterator != end`
  * where a relevant`.end()` call flowed into end
  */
 predicate validEndBoundCheck(ContainerIteratorAccess it, IteratorSource source) {
   exists(
-    ContainerAccessWithoutRangeCheck::ContainerEndCall end, BasicBlock b, GuardCondition l,
-    ContainerIteratorAccess otherAccess
+    FunctionCall end, BasicBlock b, GuardCondition l, ContainerIteratorAccess otherAccess,
+    Expr qualifierToCheck
   |
+    //sufficient end guard
+    qualifierToCheck = validEndCheck(end) and
     //guard controls the access
     l.controls(b, _) and
     b.contains(it) and
-    //guard is comprised of (anything flowing to) end check and an iterator access
+    //guard is comprised of end check and an iterator access
     DataFlow::localFlow(DataFlow::exprNode(end), DataFlow::exprNode(l.getChild(_))) and
     l.getChild(_) = otherAccess and
     //make sure its the same iterator being checked in the guard as accessed
     otherAccess.getOwningContainer() = it.getOwningContainer() and
-    //make sure its the same container providing its end as giving the iterator
-    globalValueNumber(end.getQualifier()) = globalValueNumber(source.getQualifier())
+    //if its the end call itself (or its parts), make sure its the same container providing its end as giving the iterator
+    globalValueNumber(qualifierToCheck) = globalValueNumber(source.getQualifier()) and
+    // and the guard call we match must be after the assignment call (to avoid valid guards protecting new iterator accesses further down)
+    source.getASuccessor*() = l
   )
 }
 
@@ -58,5 +74,5 @@ where
   not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and
   source = it.getANearbyAssigningIteratorCall() and
   not validEndBoundCheck(it, source) and
-  not sizeCheckedAbove(it, source)
+  not sizeCompareBoundsChecked(source, it)
 select it, "Increment of iterator may overflow since its bounds are not checked."

cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected

@@ -1,4 +1,6 @@
 | test.cpp:8:7:8:7 | i | Increment of iterator may overflow since its bounds are not checked. |
 | test.cpp:9:9:9:9 | i | Increment of iterator may overflow since its bounds are not checked. |
 | test.cpp:10:9:10:9 | i | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:22:18:22:18 | i | Increment of iterator may overflow since its bounds are not checked. |
 | test.cpp:27:31:27:31 | i | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:40:5:40:8 | end2 | Increment of iterator may overflow since its bounds are not checked. |

cpp/cert/test/rules/CTR55-CPP/test.cpp

@@ -20,8 +20,9 @@ void f1(std::vector<int> &v) {
   }
   for (auto i = v.begin(),
             l = (i + std::min(static_cast<std::vector<int>::size_type>(10),
-                              v.size()));
-       i != l; ++i) { // COMPLIANT
+                              v.size())); // NON_COMPLIANT - technically in the
+                                          // calculation
+       i != l; ++i) {                     // COMPLIANT
   }
 
   for (auto i = v.begin();; ++i) { // NON_COMPLIANT
@@ -37,7 +38,7 @@ void test_fp_reported_in_374(std::vector<int> &v) {
 
   {
     auto end2 = v.end();
-    end2++;                                    // NON_COMPLIANT[FALSE_NEGATIVE]
+    end2++;                                    // NON_COMPLIANT
     for (auto i = v.begin(); i != end2; ++i) { // NON_COMPLIANT[FALSE_NEGATIVE]
     }
   }

cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll

@@ -85,6 +85,16 @@ class ContainerEmptyCall extends FunctionCall {
   }
 }
 
+/**
+ * A call to either `begin()` on a container.
+ */
+class ContainerBeginCall extends FunctionCall {
+  ContainerBeginCall() {
+    getTarget().getDeclaringType() instanceof ContainerType and
+    getTarget().getName() = "begin"
+  }
+}
+
 /**
  * A call to either `end()` on a container.
  */