CTR55-CPP: improve end check logic for iterators

Author: knewbury01

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

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

cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql

@@ -80,16 +80,7 @@ where
     iteratorCreationCall = outputContainer.getAnIteratorFunctionCall() and
     iteratorCreationCall = c.getOutputIteratorSource()
   |
-    // Guarded by a bounds check that ensures our destination is larger than "some" value
-    exists(
-      GuardCondition guard, ContainerAccessWithoutRangeCheck::ContainerSizeCall sizeCall,
-      boolean branch
-    |
-      globalValueNumber(sizeCall.getQualifier()) =
-        globalValueNumber(iteratorCreationCall.getQualifier()) and
-      guard.controls(c.getBasicBlock(), branch) and
-      relOpWithSwapAndNegate(guard, sizeCall, _, Greater(), _, branch)
-    )
+    size_compare_bounds_checked(iteratorCreationCall, c)
     or
     // Container created with sufficient size for the input
     exists(ContainerAccessWithoutRangeCheck::ContainerConstructorCall outputIteratorConstructor |

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

@@ -15,23 +15,44 @@ import cpp
 import codingstandards.cpp.cert
 import codingstandards.cpp.Iterators
 
-from ContainerIteratorAccess it
+/**
+ * any `.size()` check above our access
+ */
+predicate size_checked_above(ContainerIteratorAccess it, IteratorSource source) {
+  exists(STLContainer c, FunctionCall guardCall |
+    c.getACallToSize() = guardCall and
+    guardCall = it.getAPredecessor*() 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
+  )
+}
+
+/**
+ * some loop check exists like: `iterator != end`
+ * where a relevant`.end()` call flowed into end
+ */
+predicate valid_end_bound_check(ContainerIteratorAccess it, IteratorSource source) {
+  exists(STLContainer c, Loop l, ContainerIteratorAccess otherAccess, IteratorSource end |
+    end = c.getAnIteratorEndFunctionCall() and
+    //flow exists between end() and the loop condition
+    DataFlow::localFlow(DataFlow::exprNode(end), DataFlow::exprNode(l.getCondition().getAChild())) and
+    l.getCondition().getAChild() = otherAccess and
+    //make sure its the same iterator being checked as incremented
+    otherAccess.getOwningContainer() = it.getOwningContainer() and
+    //make sure its the same container providing its end as giving the iterator
+    globalValueNumber(end.getQualifier()) = globalValueNumber(source.getQualifier())
+  )
+}
+
+from ContainerIteratorAccess it, IteratorSource source
 where
   not isExcluded(it, IteratorsPackage::doNotUseAnAdditiveOperatorOnAnIteratorQuery()) and
   it.isAdditiveOperation() and
   not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and
-  // we get the neraby assignment
-  not exists(STLContainer c, FunctionCall nearbyAssigningIteratorCall, FunctionCall guardCall |
-    nearbyAssigningIteratorCall = it.getANearbyAssigningIteratorCall() and
-    // we look for calls to size or end
-    (guardCall = c.getACallToSize() or guardCall = c.getAnIteratorEndFunctionCall()) and
-    // such that the call to size is before this
-    // access
-    guardCall = it.getAPredecessor*() and
-    // and it uses the same qualifier as the one we were just assigned
-    nearbyAssigningIteratorCall.getQualifier().(VariableAccess).getTarget() =
-      guardCall.getQualifier().(VariableAccess).getTarget() and
-    // and the size call we match must be after the assignment call
-    nearbyAssigningIteratorCall.getASuccessor*() = guardCall
-  )
+  source = it.getANearbyAssigningIteratorCall() and
+  not size_compare_bounds_checked(source, it) and
+  not valid_end_bound_check(it, source) and
+  not size_checked_above(it, source)
 select it, "Increment of iterator may overflow since its bounds are not checked."

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

@@ -26,4 +26,19 @@ void f1(std::vector<int> &v) {
 
   for (auto i = v.begin();; ++i) { // NON_COMPLIANT
   }
+}
+
+void test_fp_reported_in_374(std::vector<int> &v) {
+  {
+    auto end = v.end();
+    for (auto i = v.begin(); i != end; ++i) { // COMPLIANT
+    }
+  }
+
+  {
+    auto end2 = v.end();
+    end2++;                                    // NON_COMPLIANT[FALSE_NEGATIVE]
+    for (auto i = v.begin(); i != end2; ++i) { // NON_COMPLIANT[FALSE_NEGATIVE]
+    }
+  }
 }

cpp/common/src/codingstandards/cpp/Iterators.qll

@@ -6,6 +6,10 @@ import cpp
 import codingstandards.cpp.dataflow.DataFlow
 import codingstandards.cpp.dataflow.TaintTracking
 import codingstandards.cpp.StdNamespace
+import codingstandards.cpp.rules.containeraccesswithoutrangecheck.ContainerAccessWithoutRangeCheck as ContainerAccessWithoutRangeCheck
+import semmle.code.cpp.controlflow.Guards
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
 
 abstract class ContainerAccess extends VariableAccess {
   abstract Variable getOwningContainer();
@@ -63,9 +67,11 @@ class ContainerIteratorAccess extends ContainerAccess {
     )
   }
 
-  // get a function call to cbegin/begin that
-  // assigns its value to the iterator represented by this
-  // access
+  /**
+   * gets a function call to cbegin/begin that
+   * assigns its value to the iterator represented by this
+   * access
+   */
   FunctionCall getANearbyAssigningIteratorCall() {
     // the underlying container for this variable is one wherein
     // there is an assigned value of cbegin/cend
@@ -462,3 +468,18 @@ ControlFlowNode getANonInvalidatedSuccessor(ContainerInvalidationOperation op) {
     not result instanceof ContainerInvalidationOperation
   )
 }
+
+/**
+ * Guarded by a bounds check that ensures our destination is larger than "some" value
+ */
+predicate size_compare_bounds_checked(IteratorSource iteratorCreationCall, Expr guarded) {
+  exists(
+    GuardCondition guard, ContainerAccessWithoutRangeCheck::ContainerSizeCall sizeCall,
+    boolean branch
+  |
+    globalValueNumber(sizeCall.getQualifier()) =
+      globalValueNumber(iteratorCreationCall.getQualifier()) and
+    guard.controls(guarded.getBasicBlock(), branch) and
+    relOpWithSwapAndNegate(guard, sizeCall, _, Greater(), _, branch)
+  )
+}