diff --git a/change-notes/1.22/analysis-cpp.md b/change-notes/1.22/analysis-cpp.md index 19bb2898ec0..dcbe8174893 100644 --- a/change-notes/1.22/analysis-cpp.md +++ b/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. | diff --git a/cpp/ql/src/Likely Bugs/ContinueInFalseLoop.qhelp b/cpp/ql/src/Likely Bugs/ContinueInFalseLoop.qhelp new file mode 100644 index 00000000000..aba04ef7f6b --- /dev/null +++ b/cpp/ql/src/Likely Bugs/ContinueInFalseLoop.qhelp @@ -0,0 +1,24 @@ + + + + + +

A continue statement only re-runs the loop if the loop condition is true. Therefore using continue in a loop with a constant false condition will never cause the loop body to be re-run, which is misleading. +

+ +
+ + +

Replace the continue statement with a break statement if the intent is to break from the loop. +

+ +
+ + +
  • + Tutorialspoint - C Programming Language: Continue Statement in C. +
  • +
    +
    diff --git a/cpp/ql/src/Likely Bugs/ContinueInFalseLoop.ql b/cpp/ql/src/Likely Bugs/ContinueInFalseLoop.ql index b3a44c0e2b1..23249564a38 100644 --- a/cpp/ql/src/Likely Bugs/ContinueInFalseLoop.ql +++ b/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, diff --git a/cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/ContinueInFalseLoop.expected b/cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/ContinueInFalseLoop.expected new file mode 100644 index 00000000000..e65ad7b79b1 --- /dev/null +++ b/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 | diff --git a/cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/ContinueInFalseLoop.qlref b/cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/ContinueInFalseLoop.qlref new file mode 100644 index 00000000000..48d9feb2072 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/ContinueInFalseLoop.qlref @@ -0,0 +1 @@ +Likely Bugs/ContinueInFalseLoop.ql diff --git a/cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/ContinueInFalseLoop/test.cpp new file mode 100644 index 00000000000..0ece8727e66 --- /dev/null +++ b/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); +}