Merge pull request #332 from raulgarciamsft/users/raulga/c6293a

Approved by dave-bartolomeo

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

@@ -0,0 +1,7 @@
+void f()
+{
+    for (signed char i = 0; i < 100; i--)
+    {
+        // code ...
+    }
+}

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

@@ -0,0 +1,27 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+  <p>A <code>for-loop</code> iteration expression goes backwards with respect of the initialization statement and condition expression.</p>
+  <p>This warning indicates that a <code>for-loop</code> might not function as intended.</p>
+</overview>
+
+<recommendation>
+<p>To fix this issue, check that the loop condition is correct and change the iteration expression to match.</p>
+</recommendation>
+
+<example>
+<p>In the following example, the initialization statement (<code>i = 0</code>) and the condition expression (<code>i &lt; 100</code>) indicate that the intended iteration expression should have been incrementing, but instead a postfix decrement operator is used (<code>i--</code>).</p>
+<sample src="inconsistentLoopDirection.c" />
+
+<p>To fix this issue, change the iteration expression to match the direction of the initialization statement and the condition expression: <code>i++</code>.</p>
+</example>
+
+<references>
+  <li><a href="https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2012/58teb7hd(v=vs.110)">warning C6293: Ill-defined for-loop: counts down from minimum</a>
+  </li>
+</references>
+
+</qhelp>

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

@@ -0,0 +1,116 @@
+/**
+ * @name Inconsistent direction of for loop
+ * @description A for-loop iteration expression goes backward with respect of the initialization statement and condition expression.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/inconsistent-loop-direction
+ * @tags correctness
+ *       external/cwe/cwe-835 
+ *       external/microsoft/6293
+ * @msrc.severity important
+ */
+import cpp
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
+import semmle.code.cpp.dataflow.DataFlow
+
+predicate illDefinedDecrForStmt( ForStmt forstmt, Variable v, Expr initialCondition, Expr terminalCondition ) { 
+  v.getAnAssignedValue() = initialCondition
+  and
+  exists( 
+    RelationalOperation rel |
+    rel = forstmt.getCondition() |
+    terminalCondition = rel.getGreaterOperand()
+    and v.getAnAccess() = rel.getLesserOperand()
+    and
+    DataFlow::localFlowStep(DataFlow::exprNode(initialCondition), DataFlow::exprNode(rel.getLesserOperand()))
+    )
+  and
+  exists( 
+    DecrementOperation dec |
+    dec = forstmt.getUpdate().(DecrementOperation)
+    and dec.getAnOperand() = v.getAnAccess()
+  )
+  and
+  ( 
+    ( upperBound(initialCondition) < lowerBound(terminalCondition) )
+    or
+    ( forstmt.conditionAlwaysFalse() or forstmt.conditionAlwaysTrue() )
+  )
+}
+
+predicate illDefinedIncrForStmt( ForStmt forstmt, Variable v, Expr initialCondition, Expr terminalCondition ) { 
+  v.getAnAssignedValue() = initialCondition 
+  and
+  exists( 
+    RelationalOperation rel |
+    rel = forstmt.getCondition() |
+    terminalCondition = rel.getLesserOperand()
+    and v.getAnAccess() = rel.getGreaterOperand()
+    and
+    DataFlow::localFlowStep(DataFlow::exprNode(initialCondition), DataFlow::exprNode(rel.getGreaterOperand()))
+  )
+  and
+  exists( IncrementOperation incr |
+    incr = forstmt.getUpdate().(IncrementOperation)
+    and
+    incr.getAnOperand() = v.getAnAccess()
+  )
+  and
+  ( 
+    ( upperBound(terminalCondition) < lowerBound(initialCondition))
+    or
+    ( forstmt.conditionAlwaysFalse() or forstmt.conditionAlwaysTrue())
+  )
+}
+ 
+predicate illDefinedForStmtWrongDirection( ForStmt forstmt, Variable v, Expr initialCondition, Expr terminalCondition
+  , boolean isIncr ) {
+  ( illDefinedDecrForStmt( forstmt, v, initialCondition, terminalCondition) and isIncr = false )
+  or
+  ( illDefinedIncrForStmt( forstmt, v, initialCondition, terminalCondition) and isIncr = true)
+}
+ 
+bindingset[b]
+private string forLoopdirection(boolean b){
+  if( b = true ) then  result = "upward" 
+  else result = "downward"
+}
+
+bindingset[b]
+private string forLoopTerminalConditionRelationship(boolean b){
+  if( b = true ) then  result = "lower" 
+  else result = "higher"
+}
+
+ predicate illDefinedForStmt( ForStmt for, string message ) {
+  exists( 
+    boolean isIncr, 
+    Variable v,
+    Expr initialCondition,
+    Expr terminalCondition |
+    illDefinedForStmtWrongDirection(for, v, initialCondition, terminalCondition, isIncr)
+    and
+    if( for.conditionAlwaysFalse() ) then 
+    (
+      message = "Ill-defined for-loop: a loop using variable \"" + v + "\" counts " 
+      + forLoopdirection(isIncr) + " from a value ("+ initialCondition +"), but the terminal condition is always false."
+    ) 
+    else if 
+    ( 
+      for.conditionAlwaysTrue() ) then (
+      message = "Ill-defined for-loop: a loop using variable \"" + v + "\" counts " 
+      + forLoopdirection(isIncr) + " from a value ("+ initialCondition +"), but the terminal condition is always true."
+    ) 
+    else 
+    (
+      message = "Ill-defined for-loop: a loop using variable \"" + v + "\" counts " 
+        + forLoopdirection(isIncr) + " from a value ("+ initialCondition +"), but the terminal condition is "
+        + forLoopTerminalConditionRelationship(isIncr) + " (" + terminalCondition + ")."
+    )
+  )
+}
+ 
+from ForStmt forstmt, string message 
+ where illDefinedForStmt(forstmt, message)
+select forstmt, message

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

@@ -0,0 +1,89 @@
+void Signed()
+{
+    signed char i;
+
+    for (i = 0; i < 100; i--)   //BUG
+    {
+    }
+
+    for (i = 0; i < 100; i++)
+    {
+    }
+
+    for (i = 100; i >= 0; i++)   //BUG
+    {
+    }
+
+    for (i = 100; i >= 0; i--)
+    {
+    }
+
+}
+
+void Unsigned()
+{
+    unsigned long i;
+
+    for (i = 0; i < 100; i--)   //BUG
+    {
+    }
+
+    for (i = 0; i < 100; i++)
+    {
+    }
+
+    for (i = 100; i >= 0; i++)   //BUG
+    {
+    }
+
+    for (i = 100; i >= 0; i--)
+    {
+    }
+}
+
+void InitializationOutsideLoop()
+{
+    signed char i = 0;
+
+    for (; i < 100; i--)   //BUG
+    {
+    }
+
+    i = 0;
+    for (; i < 100; i++)
+    {
+    }
+
+    i = 100;
+    for (; i >= 0; i++)   //BUG
+    {
+    }
+
+    i = 100;
+    for (; i >= 0; i--)
+    {
+    }
+}
+
+void NegativeTestCase()
+{
+    int i;
+    for (i = 0; (200 - i) < 100; i--)
+    {
+        // code ...
+    }
+}
+
+void NegativeTestCaseNested()
+{
+    int k;
+    int i;
+
+    for (k = 200; k < 300; k++)
+    {
+        for (i = 0; (k - i) < 100; i--)
+        {
+            // code ...
+        }
+    }
+}

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

@@ -0,0 +1,180 @@
+void Signed()
+{
+    signed char i;
+
+    for (i = 0; i < 100; i--)   //BUG
+    {
+    }
+
+    for (i = 0; i < 100; i++)
+    {
+    }
+
+    for (i = 100; i >= 0; i++)   //BUG
+    {
+    }
+
+    for (i = 100; i >= 0; i--)
+    {
+    }
+
+}
+
+void Unsigned()
+{
+    unsigned long i;
+
+    for (i = 0; i < 100; i--)   //BUG
+    {
+    }
+
+    for (i = 0; i < 100; i++)
+    {
+    }
+
+    for (i = 100; i >= 0; i++)   //BUG
+    {
+    }
+
+    for (i = 100; i >= 0; i--)
+    {
+    }
+}
+
+void DeclarationInLoop()
+{
+    for (signed char i = 0; i < 100; --i)   //BUG
+    {
+    }
+
+    for (signed char i = 0; i < 100; ++i)
+    {
+    }
+
+    for (unsigned char i = 100; i >= 0; ++i)   //BUG
+    {
+    }
+
+    for (unsigned char i = 100; i >= 0; --i)
+    {
+    }
+}
+
+void SignedWithVariables()
+{
+    signed char i;
+    signed char min = 0;
+    signed char max = 100;
+
+    for (i = min; i < max; i--)   //BUG
+    {
+    }
+
+    for (i = min; i < max; i++)
+    {
+    }
+
+    for (i = max; i >= min; i++)   //BUG
+    {
+    }
+
+    for (i = max; i >= min; i--)
+    {
+    }
+
+}
+
+void InitializationOutsideLoop()
+{
+    signed char i = 0;
+
+    for (; i < 100; --i)   //BUG
+    {
+    }
+
+    i = 0;
+    for (; i < 100; ++i)
+    {
+    }
+
+    i = 100;
+    for (; i >= 0; ++i)   //BUG
+    {
+    }
+
+    i = 100;
+    for (; i >= 0; --i)
+    {
+    }
+}
+
+
+void InvalidCondition()
+{
+    signed char i;
+    signed char min = 0;
+    signed char max = 100;
+
+    for (i = max; i < min; i--)   //BUG
+    {
+    }
+
+    for (i = min; i > max; i++)   //BUG
+    {
+    }
+}
+
+void InvalidConditionUnsignedCornerCase()
+{
+    unsigned char i;
+    unsigned char min = 0;
+    unsigned char max = 100;
+
+    for (i = 100; i < 0; i--)   //BUG
+    {
+    }
+
+    // Limitation. 
+    // Currently odasa will not detect this for-loop condition as always true
+    // The rule will still detect the mismatch iterator, but the error message may change in the future.
+    for (i = 200; i >= 0; i++)   //BUG
+    {
+    }
+}
+
+void NegativeTestCase()
+{
+    for (int i = 0; (100 - i) < 200; i--)
+    {
+        // code ...
+    }
+}
+
+void NegativeTestCaseNested()
+{
+    for (int k = 200; k < 300; k++)
+    {
+        for (int i = 0; (k - i) < 100; i--)
+        {
+            // code ...
+        }
+    }
+}
+
+//////////////////////////////////////
+// Query limitation:
+//
+// The following test cases are bugs,
+// but will not be found due to the itearion expression 
+// not being a prefix or postfix increment/decrement
+//
+void FalseNegativeTestCases()
+{
+    for (int i = 0; i < 10; i = i - 1) {}
+    // For comparison
+    for (int i = 0; i < 10; i-- ) {} // BUG
+
+    for (int i = 100; i > 0; i += 2) {}
+    // For comparison
+    for (int i = 100; i > 0; i ++ ) {}  // BUG
+}

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

@@ -0,0 +1,22 @@
+| inconsistentLoopDirection.c:5:5:7:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (0), but the terminal condition is higher (100). |
+| inconsistentLoopDirection.c:13:5:15:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts upward from a value (100), but the terminal condition is lower (0). |
+| inconsistentLoopDirection.c:27:5:29:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (0), but the terminal condition is higher (100). |
+| inconsistentLoopDirection.c:35:5:37:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts upward from a value (100), but the terminal condition is lower (0). |
+| inconsistentLoopDirection.c:48:5:50:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (0), but the terminal condition is higher (100). |
+| inconsistentLoopDirection.c:58:5:60:5 | 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:5:5:7:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (0), but the terminal condition is higher (100). |
+| inconsistentLoopDirection.cpp:13:5:15:5 | 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:27:5:29:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (0), but the terminal condition is higher (100). |
+| inconsistentLoopDirection.cpp:35:5:37:5 | 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:46:5:48:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (0), but the terminal condition is higher (100). |
+| inconsistentLoopDirection.cpp:54:5:56:5 | 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:69:5:71:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (min), but the terminal condition is higher (max). |
+| inconsistentLoopDirection.cpp:77:5:79:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts upward from a value (max), but the terminal condition is lower (min). |
+| inconsistentLoopDirection.cpp:91:5:93:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (0), but the terminal condition is higher (100). |
+| inconsistentLoopDirection.cpp:101:5:103:5 | 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:118:5:120:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (max), but the terminal condition is always false. |
+| inconsistentLoopDirection.cpp:122:5:124:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts upward from a value (min), but the terminal condition is always false. |
+| inconsistentLoopDirection.cpp:133:5:135:5 | for(...;...;...) ... | Ill-defined for-loop: a loop using variable "i" counts downward from a value (100), but the terminal condition is always false. |
+| 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). |

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

@@ -0,0 +1 @@
+Likely Bugs/Likely Typos/inconsistentLoopDirection.ql