Merge pull request #1515 from geoffw0/continuefalseloop

CPP: Improvements to ContinueInFalseLoop.ql

Author: semmledocs-ac

change-notes/1.22/analysis-cpp.md

@@ -11,6 +11,7 @@
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Continue statement that does not continue (`cpp/continue-in-false-loop`) | Fewer false positive results | Analysis is now restricted to `do`-`while` loops. This query is now run and displayed by default on LGTM. |
 | Expression has no effect (`cpp/useless-expression`) | Fewer false positive results | Calls to functions with the `weak` attribute are no longer considered to be side effect free, because they could be overridden with a different implementation at link time. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | False positives involving strings that are not null-terminated have been excluded. |
 | Suspicious pointer scaling (`cpp/suspicious-pointer-scaling`) | Lower precision | The precision of this query has been reduced to "medium". This coding pattern is used intentionally and safely in a number of real-world projects. Results are no longer displayed on LGTM unless you choose to display them. |

cpp/ql/src/Likely Bugs/ContinueInFalseLoop.qhelp

@@ -0,0 +1,24 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>A <code>continue</code> statement only re-runs the loop if the loop condition is true. Therefore using <code>continue</code> in a loop with a constant false condition will never cause the loop body to be re-run, which is misleading.
+</p>
+
+</overview>
+<recommendation>
+
+<p>Replace the <code>continue</code> statement with a <code>break</code> statement if the intent is to break from the loop.
+</p>
+
+</recommendation>
+
+<references>
+<li>
+  Tutorialspoint - C Programming Language: <a href="https://www.tutorialspoint.com/cprogramming/c_continue_statement.htm">Continue Statement in C</a>.
+</li>
+</references>
+</qhelp>

cpp/ql/src/Likely Bugs/ContinueInFalseLoop.ql

@@ -6,25 +6,43 @@
  * @kind problem
  * @id cpp/continue-in-false-loop
  * @problem.severity warning
+ * @precision high
+ * @tags correctness
  */
 
 import cpp
 
-Loop getAFalseLoop() {
+/**
+ * Gets a `do` ... `while` loop with a constant false condition.
+ */
+DoStmt getAFalseLoop() {
   result.getControllingExpr().getValue() = "0"
   and not result.getControllingExpr().isAffectedByMacro()
 }
 
-Loop enclosingLoop(Stmt s) {
+/**
+ * Gets a `do` ... `while` loop surrounding a statement.  This is blocked by a
+ * `switch` statement, since a `continue` inside a `switch` inside a loop may be
+ * jusitifed (`continue` breaks out of the loop whereas `break` only escapes the
+ * `switch`).
+ */
+DoStmt enclosingLoop(Stmt s) {
   exists(Stmt parent |
     parent = s.getParent() and
-    if parent instanceof Loop then
-      result = parent
-    else
-      result = enclosingLoop(parent))
+    (
+      (
+        parent instanceof Loop and
+        result = parent
+      ) or (
+        not parent instanceof Loop and
+        not parent instanceof SwitchStmt and
+        result = enclosingLoop(parent)
+      )
+    )
+  )
 }
 
-from Loop loop, ContinueStmt continue
+from DoStmt loop, ContinueStmt continue
 where loop = getAFalseLoop()
   and loop = enclosingLoop(continue)
 select continue,

cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/ContinueInFalseLoop.expected

@@ -0,0 +1,2 @@
+| test.cpp:13:4:13:12 | continue; | This 'continue' never re-runs the loop - the $@ is always false. | test.cpp:16:11:16:15 | 0 | loop condition |
+| test.cpp:59:5:59:13 | continue; | This 'continue' never re-runs the loop - the $@ is always false. | test.cpp:62:12:62:16 | 0 | loop condition |

cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/ContinueInFalseLoop.qlref

@@ -0,0 +1 @@
+Likely Bugs/ContinueInFalseLoop.ql

cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/test.cpp

@@ -0,0 +1,94 @@
+
+bool cond();
+
+void test1(int x)
+{
+	int i;
+
+	// --- do loops ---
+
+	do
+	{
+		if (cond())
+			continue; // BAD
+		if (cond())
+			break;
+	} while (false);
+
+	do
+	{
+		if (cond())
+			continue;
+		if (cond())
+			break;
+	} while (true);
+
+	do
+	{
+		if (cond())
+			continue;
+		if (cond())
+			break;
+	} while (cond());
+
+	// --- while, for loops ---
+
+	while (false)
+	{
+		if (cond())
+			continue; // GOOD [never reached, if the condition changed so it was then the result would no longer apply]
+		if (cond())
+			break;
+	}
+
+	for (i = 0; false; i++)
+	{
+		if (cond())
+			continue; // GOOD [never reached, if the condition changed so it was then the result would no longer apply]
+		if (cond())
+			break;
+	}
+
+	// --- nested loops ---
+
+	do
+	{
+		do
+		{
+			if (cond())
+				continue; // BAD
+			if (cond())
+				break;
+		} while (false);
+	} while (true);
+
+	do
+	{
+		do
+		{
+			if (cond())
+				continue; // GOOD
+			if (cond())
+				break;
+		} while (true);
+	} while (false);
+
+	do
+	{
+		switch (x)
+		{
+		case 1:
+			// do [1]
+
+			break; // break out of the switch
+
+		default:
+			// do [2]
+
+			continue; // GOOD; break out of the loop entirely, skipping [3]
+		};
+
+		// do [3]
+
+	} while (0);
+}