CTR55-CPP: predicate renaming, variable clarity

Author: knewbury01

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

@@ -17,54 +17,66 @@ import codingstandards.cpp.Iterators
 import semmle.code.cpp.controlflow.Dominance
 
 /**
- *  Get a derived one passed the end element for `containerReference`.
+ * 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 calculatedEndCheck(AdditiveOperatorFunctionCall calc) {
+Expr getDerivedReferenceToOnePassedTheEndElement(Expr containerReference) {
   exists(
     ContainerAccessWithoutRangeCheck::ContainerSizeCall size,
-    ContainerAccessWithoutRangeCheck::ContainerBeginCall begin
+    ContainerAccessWithoutRangeCheck::ContainerBeginCall begin, AdditionOperatorFunctionCall calc
   |
-    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
+    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
-    result = begin.getQualifier()
+    containerReference = begin.getQualifier()
   )
 }
 
-Expr validEndCheck(FunctionCall end) {
-  end instanceof ContainerAccessWithoutRangeCheck::ContainerEndCall and
-  result = end.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 = calculatedEndCheck(end)
+  result = getDerivedReferenceToOnePassedTheEndElement(containerReference)
 }
 
 /**
  * some guard exists like: `iterator != end`
  * where a relevant`.end()` call flowed into end
  */
-predicate validEndBoundCheck(ContainerIteratorAccess it, IteratorSource source) {
+predicate isUpperBoundEndCheckedIteratorAccess(IteratorSource source, ContainerIteratorAccess it) {
   exists(
-    FunctionCall end, BasicBlock b, GuardCondition l, ContainerIteratorAccess otherAccess,
-    Expr qualifierToCheck
+    Expr referenceToOnePassedTheEndElement, BasicBlock basicBlockOfIteratorAccess, GuardCondition upperBoundCheck,
+    ContainerIteratorAccess checkedIteratorAccess, Expr containerReferenceFromEndGuard
   |
     //sufficient end guard
-    qualifierToCheck = validEndCheck(end) and
+    referenceToOnePassedTheEndElement = getReferenceToOnePassedTheEndElement(containerReferenceFromEndGuard) and
     //guard controls the access
-    l.controls(b, _) and
-    b.contains(it) and
+    upperBoundCheck.controls(basicBlockOfIteratorAccess, _) and
+    basicBlockOfIteratorAccess.contains(it) and
     //guard is comprised of end check and an iterator access
-    DataFlow::localFlow(DataFlow::exprNode(end), DataFlow::exprNode(l.getChild(_))) and
-    l.getChild(_) = otherAccess and
+    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
-    otherAccess.getOwningContainer() = it.getOwningContainer() and
+    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(qualifierToCheck) = globalValueNumber(source.getQualifier()) and
+    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*() = l
+    source.getASuccessor*() = upperBoundCheck
   )
 }
 
@@ -74,6 +86,6 @@ where
   it.isAdditiveOperation() and
   not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and
   source = it.getANearbyAssigningIteratorCall() and
-  not validEndBoundCheck(it, source) 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

@@ -4,3 +4,4 @@
 | 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:50:42:50:42 | i | Increment of iterator may overflow since its bounds are not checked. |

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

@@ -42,4 +42,12 @@ void test_fp_reported_in_374(std::vector<int> &v) {
     for (auto i = v.begin(); i != end2; ++i) { // NON_COMPLIANT[FALSE_NEGATIVE]
     }
   }
+}
+
+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
+    }
+  }
 }