From ac846b96d17967c53a18c9193a8a1572b15c7723 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sat, 22 Aug 2020 02:16:26 -0500 Subject: [PATCH] New check: Iterating a known empty container (#2740) --- lib/astutils.cpp | 11 ++++++++++- lib/checkstl.cpp | 42 +++++++++++++++++++++++++++++++++++++++++ lib/checkstl.h | 7 +++++++ lib/forwardanalyzer.cpp | 16 +++++++++++++--- lib/valueflow.cpp | 4 ++-- test/teststl.cpp | 31 ++++++++++++++++++++++++++++++ test/testvalueflow.cpp | 2 +- 7 files changed, 106 insertions(+), 7 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index d6d1a8f80..370dc0d55 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1349,6 +1349,13 @@ static const Variable* getArgumentVar(const Token* tok, int argnr) return nullptr; } +static bool isCPPCastKeyword(const Token* tok) +{ + if (!tok) + return false; + return endsWith(tok->str(), "_cast", 5); +} + bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Settings *settings, bool *inconclusive) { if (!tok) @@ -1366,6 +1373,8 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti tok = getTokenArgumentFunction(tok, argnr); if (!tok) return false; // not a function => variable not changed + if (tok->isKeyword() && !isCPPCastKeyword(tok)) + return false; const Token * parenTok = tok->next(); if (Token::simpleMatch(parenTok, "<") && parenTok->link()) parenTok = parenTok->link()->next(); @@ -1734,7 +1743,7 @@ bool isLikelyStreamRead(bool cpp, const Token *op) bool isCPPCast(const Token* tok) { - return tok && Token::simpleMatch(tok->previous(), "> (") && tok->astOperand2() && tok->astOperand1() && tok->astOperand1()->str().find("_cast") != std::string::npos; + return tok && Token::simpleMatch(tok->previous(), "> (") && tok->astOperand2() && tok->astOperand1() && isCPPCastKeyword(tok->astOperand1()); } bool isConstVarExpression(const Token *tok, const char* skipMatch) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 856682b98..d66e38679 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2461,6 +2461,48 @@ void CheckStl::useStlAlgorithm() } } +void CheckStl::knownEmptyContainerLoopError(const Token *tok) +{ + const std::string cont = tok ? tok->expressionString() : std::string("var"); + + reportError(tok, Severity::style, + "knownEmptyContainerLoop", + "Iterating over container '" + cont + "' that is always empty.", CWE398, false); +} + +void CheckStl::knownEmptyContainerLoop() +{ + if (!mSettings->isEnabled(Settings::STYLE)) + return; + for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) { + for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) { + // Parse range-based for loop + if (!Token::simpleMatch(tok, "for (")) + continue; + if (!Token::simpleMatch(tok->next()->link(), ") {")) + continue; + const Token *bodyTok = tok->next()->link()->next(); + const Token *splitTok = tok->next()->astOperand2(); + if (!Token::simpleMatch(splitTok, ":")) + continue; + const Token* contTok = splitTok->astOperand2(); + if (!contTok) + continue; + for(const ValueFlow::Value& v:contTok->values()) { + if (!v.isKnown()) + continue; + if (!v.isContainerSizeValue()) + continue; + if (v.intvalue != 0) + continue; + knownEmptyContainerLoopError(contTok); + } + + + } + } +} + static bool isMutex(const Variable* var) { const Token* tok = Token::typeDecl(var->nameToken()).first; diff --git a/lib/checkstl.h b/lib/checkstl.h index 55256a39f..ffa98cf88 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -81,6 +81,7 @@ public: checkStl.invalidContainerLoop(); checkStl.mismatchingContainers(); checkStl.mismatchingContainerIterator(); + checkStl.knownEmptyContainerLoop(); checkStl.stlBoundaries(); checkStl.checkDereferenceInvalidIterator(); @@ -188,6 +189,8 @@ public: /** @brief Look for loops that can replaced with std algorithms */ void useStlAlgorithm(); + + void knownEmptyContainerLoop(); void checkMutexes(); @@ -235,6 +238,8 @@ private: void useStlAlgorithmError(const Token *tok, const std::string &algoName); + void knownEmptyContainerLoopError(const Token *tok); + void globalLockGuardError(const Token *tok); void localMutexError(const Token *tok); @@ -274,6 +279,7 @@ private: c.dereferenceInvalidIteratorError(nullptr, "i"); c.readingEmptyStlContainerError(nullptr); c.useStlAlgorithmError(nullptr, ""); + c.knownEmptyContainerLoopError(nullptr); c.globalLockGuardError(nullptr); c.localMutexError(nullptr); } @@ -298,6 +304,7 @@ private: "- useless calls of string and STL functions\n" "- dereferencing an invalid iterator\n" "- reading from empty STL container\n" + "- iterating over an empty STL container\n" "- consider using an STL algorithm instead of raw loop\n" "- incorrect locking with mutex\n"; } diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index e2ae467c2..a2bebe6d8 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -330,9 +330,19 @@ struct ForwardTraversal { if (initTok && updateRecursive(initTok) == Progress::Break) return Progress::Break; if (Token::Match(tok, "for|while (")) { - Token* stepTok = getStepTok(tok); - if (updateLoop(endBlock, condTok, initTok, stepTok) == Progress::Break) - return Progress::Break; + // For-range loop + if (Token::simpleMatch(condTok, ":")) { + Token* conTok = condTok->astOperand2(); + if (conTok && updateRecursive(conTok) == Progress::Break) + return Progress::Break; + if (updateLoop(endBlock, condTok) == Progress::Break) + return Progress::Break; + } else { + Token* stepTok = getStepTok(tok); + if (updateLoop(endBlock, condTok, initTok, stepTok) == Progress::Break) + return Progress::Break; + + } tok = endBlock; } else { // Traverse condition diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index cb28510c7..b96d7b842 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1961,7 +1961,7 @@ static bool isConditionKnown(const Token* tok, bool then) if (then) op = "&&"; const Token* parent = tok->astParent(); - while (parent && parent->str() == op) + while (parent && (parent->str() == op || parent->str() == "!")) parent = parent->astParent(); return (parent && parent->str() == "("); } @@ -5977,7 +5977,7 @@ static void valueFlowContainerAfterCondition(TokenList *tokenlist, return cond; const Token *parent = tok->astParent(); while (parent) { - if (Token::Match(parent, "%comp%|!")) + if (Token::Match(parent, "%comp%")) return cond; parent = parent->astParent(); } diff --git a/test/teststl.cpp b/test/teststl.cpp index e0f1bb01b..1dcedb538 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -166,6 +166,7 @@ private: TEST_CASE(invalidContainerLoop); TEST_CASE(findInsert); + TEST_CASE(checkKnownEmptyContainerLoop); TEST_CASE(checkMutexes); } @@ -4524,6 +4525,36 @@ private: ASSERT_EQUALS("", errout.str()); } + void checkKnownEmptyContainerLoop() { + check("void f() {\n" + " std::vector v;\n" + " for(auto x:v) {}\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (style) Iterating over container 'v' that is always empty.\n", errout.str()); + + check("void f(std::vector v) {\n" + " v.clear();\n" + " for(auto x:v) {}\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (style) Iterating over container 'v' that is always empty.\n", errout.str()); + + check("void f(std::vector v) {\n" + " if (!v.empty()) { return; }\n" + " for(auto x:v) {}\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (style) Iterating over container 'v' that is always empty.\n", errout.str()); + + check("void f(std::vector v) {\n" + " if (v.empty()) { return; }\n" + " for(auto x:v) {}\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + } + void checkMutexes() { check("void f() {\n" " static std::mutex m;\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 77f3f06b0..ffae23ab3 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -4068,7 +4068,7 @@ private: " if (!static_cast(ints.empty()))\n" " ints.front();\n" "}"; - ASSERT(tokenValues(code, "ints . front").empty()); + ASSERT_EQUALS("", isImpossibleContainerSizeValue(tokenValues(code, "ints . front"), 0)); // valueFlowContainerReverse code = "void f(const std::list &ints) {\n"