Merge pull request github#4023 from geoffw0/loopdir

C++: Exclude decrementing unsigned counters from inconsistentLoopDirection.ql

Author: jbj

change-notes/1.26/analysis-cpp.md

@@ -0,0 +1,19 @@
+# Improvements to C/C++ analysis
+
+The following changes in version 1.26 affect C/C++ analysis in all applications.
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+## Changes to existing queries
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+| Inconsistent direction of for loop (`cpp/inconsistent-loop-direction`) | Fewer false positive results | The query now accounts for intentional wrapping of an unsigned loop counter. |
+
+## Changes to libraries
+

cpp/ql/src/Likely Bugs/Likely Typos/inconsistentLoopDirection.ql

@@ -50,7 +50,12 @@ predicate illDefinedDecrForStmt(
     DataFlow::localFlowStep(DataFlow::exprNode(initialCondition), DataFlow::exprNode(lesserOperand)) and
     // `initialCondition` < `terminalCondition`
     (
-      upperBound(initialCondition) < lowerBound(terminalCondition)
+      upperBound(initialCondition) < lowerBound(terminalCondition) and
+      (
+        // exclude cases where the loop counter is `unsigned` (where wrapping behaviour can be used deliberately)
+        v.getUnspecifiedType().(IntegralType).isSigned() or
+        initialCondition.getValue().toInt() = 0
+      )
       or
       (forstmt.conditionAlwaysFalse() or forstmt.conditionAlwaysTrue())
     )

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/inconsistentLoopDirection/inconsistentLoopDirection.cpp

@@ -177,4 +177,43 @@ void FalseNegativeTestCases()
     for (int i = 100; i > 0; i += 2) {}
     // For comparison
     for (int i = 100; i > 0; i ++ ) {}  // BUG
-}
+}
+
+void IntendedOverflow(unsigned char p)
+{
+    const unsigned char m = 10;
+    unsigned char i;
+    signed char s;
+
+    for (i = 63; i < 64; i--) {} // GOOD (legitimate way to count down with an unsigned)
+    for (i = 63; i < 128; i--) {} // DUBIOUS (could still be a typo?)
+    for (i = 63; i < 255; i--) {} // GOOD
+
+    for (i = m - 1; i < m; i--) {} // GOOD
+    for (i = m - 2; i < m; i--) {} // DUBIOUS
+    for (i = m; i < m + 1; i--) {} // GOOD
+
+    for (s = 63; s < 64; s--) {} // BAD (signed numbers don't wrap at 0 / at all)
+    for (s = m + 1; s < m; s--) {} // BAD (never runs)
+
+    for (i = p - 1; i < p; i--) {} // GOOD
+    for (s = p - 1; s < p; s--) {} // BAD [NOT DETECTED]
+
+	{
+		int n;
+		
+		n = 64;
+		for (i = n - 1; i < n; i--) {} // GOOD
+		n = 64;
+		for (i = n - 1; i < 64; i--) {} // GOOD
+		n = 64;
+		for (i = 63; i < n; i--) {} // GOOD
+
+		n = 64;
+		for (s = n - 1; s < n; s--) {} // BAD [NOT DETECTED]
+		n = 64;
+		for (s = n - 1; s < 64; s--) {} // BAD
+		n = 64;
+		for (s = 63; s < n; s--) {} // BAD [NOT DETECTED]
+	}
+}

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/inconsistentLoopDirection/inconsistentLoopDirection.expected

@@ -20,3 +20,6 @@
 | inconsistentLoopDirection.cpp:140:5:142:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts upward from a value (200), but the terminal condition is lower (0). |
 | inconsistentLoopDirection.cpp:175:5:175:36 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (0), but the terminal condition is higher (10). |
 | inconsistentLoopDirection.cpp:179:5:179:38 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts upward from a value (100), but the terminal condition is lower (0). |
+| inconsistentLoopDirection.cpp:196:5:196:32 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "s" counts downward from a value (63), but the terminal condition is higher (64). |
+| inconsistentLoopDirection.cpp:197:5:197:34 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "s" counts downward from a value (... + ...), but the terminal condition is always false. |
+| inconsistentLoopDirection.cpp:215:3:215:33 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "s" counts downward from a value (... - ...), but the terminal condition is higher (64). |