Merge pull request #561 from knewbury01/knewbury01/fix-374

CTR55-CPP: address 374

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 and size 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)
-    )
+    sizeCompareBoundsChecked(iteratorCreationCall, c)
     or
     // Container created with sufficient size for the input
     exists(ContainerAccessWithoutRangeCheck::ContainerConstructorCall outputIteratorConstructor |

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

@@ -14,24 +14,81 @@
 import cpp
 import codingstandards.cpp.cert
 import codingstandards.cpp.Iterators
+import semmle.code.cpp.controlflow.Dominance
 
-from ContainerIteratorAccess it
+/**
+ * Models a call to an iterator's `operator+`
+ */
+class AdditionOperatorFunctionCall extends AdditiveOperatorFunctionCall {
+  AdditionOperatorFunctionCall() { this.getTarget().hasName("operator+") }
+}
+
+/**
+ *  There exists a calculation for the reference one passed the end of some container
+ *  An example derivation is:
+ *     `end = begin() + size()`
+ */
+Expr getDerivedReferenceToOnePassedTheEndElement(Expr containerReference) {
+  exists(
+    ContainerAccessWithoutRangeCheck::ContainerSizeCall size,
+    ContainerAccessWithoutRangeCheck::ContainerBeginCall begin, AdditionOperatorFunctionCall calc
+  |
+    result = calc
+  |
+    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
+    containerReference = begin.getQualifier()
+  )
+}
+
+/**
+ * a wrapper predicate for a couple of types of permitted end bounds checks
+ */
+Expr getReferenceToOnePassedTheEndElement(Expr containerReference) {
+  //a container end access - v.end()
+  result instanceof ContainerAccessWithoutRangeCheck::ContainerEndCall and
+  containerReference = result.(FunctionCall).getQualifier()
+  or
+  result = getDerivedReferenceToOnePassedTheEndElement(containerReference)
+}
+
+/**
+ * some guard exists like: `iterator != end`
+ * where a relevant`.end()` call flowed into end
+ */
+predicate isUpperBoundEndCheckedIteratorAccess(IteratorSource source, ContainerIteratorAccess it) {
+  exists(
+    Expr referenceToOnePassedTheEndElement, BasicBlock basicBlockOfIteratorAccess,
+    GuardCondition upperBoundCheck, ContainerIteratorAccess checkedIteratorAccess,
+    Expr containerReferenceFromEndGuard
+  |
+    //sufficient end guard
+    referenceToOnePassedTheEndElement =
+      getReferenceToOnePassedTheEndElement(containerReferenceFromEndGuard) and
+    //guard controls the access
+    upperBoundCheck.controls(basicBlockOfIteratorAccess, _) and
+    basicBlockOfIteratorAccess.contains(it) and
+    //guard is comprised of end check and an iterator access
+    DataFlow::localFlow(DataFlow::exprNode(referenceToOnePassedTheEndElement),
+      DataFlow::exprNode(upperBoundCheck.getChild(_))) and
+    upperBoundCheck.getChild(_) = checkedIteratorAccess and
+    //make sure its the same iterator being checked in the guard as accessed
+    checkedIteratorAccess.getOwningContainer() = it.getOwningContainer() and
+    //if its the end call itself (or its parts), make sure its the same container providing its end as giving the iterator
+    globalValueNumber(containerReferenceFromEndGuard) = 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*() = upperBoundCheck
+  )
+}
+
+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 isUpperBoundEndCheckedIteratorAccess(source, it) and
+  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,8 @@
 | 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:27:31:27:31 | 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:28:31:28:31 | i | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:41:5:41:8 | end2 | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:53:42:53:42 | i | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:64:15:64:15 | i | Increment of iterator may overflow since its bounds are not checked. |

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

@@ -20,10 +20,47 @@ 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
   }
+}
+
+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
+    for (auto i = v.begin(); i != end2;
+         ++i) { // NON_COMPLIANT[FALSE_NEGATIVE] - case of invalidations to
+                // check before use expected to be less frequent, can model in
+                // future if need be
+    }
+  }
+}
+
+void test(std::vector<int> &v, std::vector<int> &v2) {
+  {
+    auto end = v2.end();
+    for (auto i = v.begin(); i != end; ++i) { // NON_COMPLIANT - wrong check
+    }
+  }
+}
+
+void test2(std::vector<int> &v) {
+  auto i = v.begin();
+  while (1) {
+    auto i2 = ((i != v.end()) != 0);
+    if (!i2)
+      break;
+    (void)((++i)); // COMPLIANT[FALSE_POSITIVE]
+  }
 }

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 sizeCompareBoundsChecked(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)
+  )
+}

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

@@ -85,6 +85,26 @@ 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.
+ */
+class ContainerEndCall extends FunctionCall {
+  ContainerEndCall() {
+    getTarget().getDeclaringType() instanceof ContainerType and
+    getTarget().getName() = "end"
+  }
+}
+
 /**
  * A call to either `size()` or `length()` on a container.
  */