diff --git a/lib/astutils.cpp b/lib/astutils.cpp index ca48483dd..30a064146 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1877,6 +1877,16 @@ std::vector getArguments(const Token *ftok) return astFlatten(startTok, ","); } +int getArgumentPos(const Variable* var, const Function* f) +{ + auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable& v) { + return v.nameToken() == var->nameToken(); + }); + if (arg_it == f->argumentList.end()) + return -1; + return std::distance(f->argumentList.begin(), arg_it); +} + const Token *findLambdaStartToken(const Token *last) { if (!last || last->str() != "}") diff --git a/lib/astutils.h b/lib/astutils.h index a4f6d83a9..d506606e1 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -30,6 +30,7 @@ #include "errortypes.h" #include "utils.h" +class Function; class Library; class Scope; class Settings; @@ -234,6 +235,8 @@ int numberOfArguments(const Token *start); */ std::vector getArguments(const Token *ftok); +int getArgumentPos(const Variable* var, const Function* f); + const Token *findLambdaStartToken(const Token *last); /** diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index bc2237a1a..860a23624 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -18,17 +18,18 @@ #include "checkstl.h" +#include "astutils.h" #include "check.h" #include "checknullpointer.h" +#include "errortypes.h" #include "library.h" #include "mathlib.h" +#include "pathanalysis.h" #include "settings.h" #include "standards.h" #include "symboldatabase.h" #include "token.h" #include "utils.h" -#include "astutils.h" -#include "pathanalysis.h" #include "valueflow.h" #include @@ -38,6 +39,8 @@ #include #include #include +#include +#include #include // Register this check class (by creating a static instance of it) @@ -843,6 +846,105 @@ static bool isInvalidMethod(const Token * tok) return false; } +struct InvalidContainerAnalyzer { + struct Info { + struct Reference { + const Token* tok; + ErrorPath errorPath; + }; + std::unordered_map expressions; + ErrorPath errorPath; + void add(const std::vector& refs) + { + for (const Reference& r : refs) { + add(r); + } + } + void add(const Reference& r) + { + if (!r.tok) + return; + expressions.insert(std::make_pair(r.tok->exprId(), r)); + } + + std::vector invalidTokens() const + { + std::vector result; + std::transform(expressions.begin(), expressions.end(), std::back_inserter(result), SelectMapValues{}); + return result; + } + }; + std::unordered_map invalidMethods; + + std::vector invalidatesContainer(const Token* tok) const + { + std::vector result; + if (Token::Match(tok, "%name% (")) { + const Function* f = tok->function(); + if (!f) + return result; + ErrorPathItem epi = std::make_pair(tok, "Calling function " + tok->str()); + const bool dependsOnThis = exprDependsOnThis(tok->next()); + auto it = invalidMethods.find(f); + if (it != invalidMethods.end()) { + std::vector refs = it->second.invalidTokens(); + std::copy_if(refs.begin(), refs.end(), std::back_inserter(result), [&](const Info::Reference& r) { + const Variable* var = r.tok->variable(); + if (!var) + return false; + if (dependsOnThis && !var->isLocal() && !var->isGlobal() && !var->isStatic()) + return true; + if (!var->isArgument()) + return false; + if (!var->isReference()) + return false; + return true; + }); + std::vector args = getArguments(tok); + for (Info::Reference& r : result) { + r.errorPath.push_front(epi); + const Variable* var = r.tok->variable(); + if (!var) + continue; + if (var->isArgument()) { + int n = getArgumentPos(var, f); + const Token* tok2 = nullptr; + if (n >= 0 && n < args.size()) + tok2 = args[n]; + r.tok = tok2; + } + } + } + } else if (astIsContainer(tok)) { + if (isInvalidMethod(tok)) { + ErrorPath ep; + ep.emplace_front(tok, + "After calling '" + tok->strAt(2) + + "', iterators or references to the container's data may be invalid ."); + result.push_back(Info::Reference{tok, ep}); + } + } + return result; + } + + void analyze(const SymbolDatabase* symboldatabase) + { + for (const Scope* scope : symboldatabase->functionScopes) { + const Function* f = scope->function; + if (!f) + continue; + for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) { + if (Token::Match(tok, "if|while|for|goto|return")) + break; + std::vector c = invalidatesContainer(tok); + if (c.empty()) + continue; + invalidMethods[f].add(c); + } + } + } +}; + static bool isVariableDecl(const Token* tok) { if (!tok) @@ -861,91 +963,91 @@ void CheckStl::invalidContainer() { const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); const Library& library = mSettings->library; + InvalidContainerAnalyzer analyzer; + analyzer.analyze(symbolDatabase); for (const Scope * scope : symbolDatabase->functionScopes) { for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { - if (!Token::Match(tok, "%var%")) - continue; - if (tok->varId() == 0) - continue; - if (!astIsContainer(tok)) - continue; - if (!isInvalidMethod(tok)) - continue; - std::set skipVarIds; - // Skip if the variable is assigned to - const Token* assignExpr = tok; - while (assignExpr->astParent()) { - bool isRHS = astIsRHS(assignExpr); - assignExpr = assignExpr->astParent(); - if (Token::Match(assignExpr, "%assign%")) { - if (!isRHS) - assignExpr = nullptr; - break; + for (const InvalidContainerAnalyzer::Info::Reference& r : analyzer.invalidatesContainer(tok)) { + if (!astIsContainer(r.tok)) + continue; + std::set skipVarIds; + // Skip if the variable is assigned to + const Token* assignExpr = tok; + while (assignExpr->astParent()) { + bool isRHS = astIsRHS(assignExpr); + assignExpr = assignExpr->astParent(); + if (Token::Match(assignExpr, "%assign%")) { + if (!isRHS) + assignExpr = nullptr; + break; + } } - } - if (Token::Match(assignExpr, "%assign%") && Token::Match(assignExpr->astOperand1(), "%var%")) - skipVarIds.insert(assignExpr->astOperand1()->varId()); - const Token * endToken = nextAfterAstRightmostLeaf(tok->next()->astParent()); - if (!endToken) - endToken = tok->next(); - const ValueFlow::Value* v = nullptr; - ErrorPath errorPath; - PathAnalysis::Info info = PathAnalysis{endToken, library} .forwardFind([&](const PathAnalysis::Info& info) { - if (!info.tok->variable()) - return false; - if (info.tok->varId() == 0) - return false; - if (skipVarIds.count(info.tok->varId()) > 0) - return false; - // if (Token::simpleMatch(info.tok->next(), ".")) - // return false; - if (Token::Match(info.tok->astParent(), "%assign%") && astIsLHS(info.tok)) - skipVarIds.insert(info.tok->varId()); - if (info.tok->variable()->isReference() && - !isVariableDecl(info.tok) && - reaches(info.tok->variable()->nameToken(), tok, library, nullptr)) { + if (Token::Match(assignExpr, "%assign%") && Token::Match(assignExpr->astOperand1(), "%var%")) + skipVarIds.insert(assignExpr->astOperand1()->varId()); + const Token* endToken = nextAfterAstRightmostLeaf(tok->next()->astParent()); + if (!endToken) + endToken = tok->next(); + const ValueFlow::Value* v = nullptr; + ErrorPath errorPath; + PathAnalysis::Info info = + PathAnalysis{endToken, library}.forwardFind([&](const PathAnalysis::Info& info) { + if (!info.tok->variable()) + return false; + if (info.tok->varId() == 0) + return false; + if (skipVarIds.count(info.tok->varId()) > 0) + return false; + // if (Token::simpleMatch(info.tok->next(), ".")) + // return false; + if (Token::Match(info.tok->astParent(), "%assign%") && astIsLHS(info.tok)) + skipVarIds.insert(info.tok->varId()); + if (info.tok->variable()->isReference() && !isVariableDecl(info.tok) && + reaches(info.tok->variable()->nameToken(), tok, library, nullptr)) { - ErrorPath ep; - bool addressOf = false; - const Variable* var = getLifetimeVariable(info.tok, ep, &addressOf); - // Check the reference is created before the change - if (var && var->declarationId() == tok->varId() && !addressOf) { - // An argument always reaches - if (var->isArgument() || (!var->isReference() && !var->isRValueReference() && - !isVariableDecl(tok) && reaches(var->nameToken(), tok, library, &ep))) { - errorPath = ep; - return true; + ErrorPath ep; + bool addressOf = false; + const Variable* var = getLifetimeVariable(info.tok, ep, &addressOf); + // Check the reference is created before the change + if (var && var->declarationId() == r.tok->varId() && !addressOf) { + // An argument always reaches + if (var->isArgument() || + (!var->isReference() && !var->isRValueReference() && !isVariableDecl(tok) && + reaches(var->nameToken(), tok, library, &ep))) { + errorPath = ep; + return true; + } + } } - } + for (const ValueFlow::Value& val : info.tok->values()) { + if (!val.isLocalLifetimeValue()) + continue; + if (val.lifetimeKind == ValueFlow::Value::LifetimeKind::Address) + continue; + if (val.lifetimeKind == ValueFlow::Value::LifetimeKind::SubObject) + continue; + if (!val.tokvalue->variable()) + continue; + if (val.tokvalue->varId() != r.tok->varId()) + continue; + ErrorPath ep; + // Check the iterator is created before the change + if (val.tokvalue != tok && reaches(val.tokvalue, tok, library, &ep)) { + v = &val; + errorPath = ep; + return true; + } + } + return false; + }); + if (!info.tok) + continue; + errorPath.insert(errorPath.end(), info.errorPath.begin(), info.errorPath.end()); + errorPath.insert(errorPath.end(), r.errorPath.begin(), r.errorPath.end()); + if (v) { + invalidContainerError(info.tok, r.tok, v, errorPath); + } else { + invalidContainerReferenceError(info.tok, r.tok, errorPath); } - for (const ValueFlow::Value& val:info.tok->values()) { - if (!val.isLocalLifetimeValue()) - continue; - if (val.lifetimeKind == ValueFlow::Value::LifetimeKind::Address) - continue; - if (val.lifetimeKind == ValueFlow::Value::LifetimeKind::SubObject) - continue; - if (!val.tokvalue->variable()) - continue; - if (val.tokvalue->varId() != tok->varId()) - continue; - ErrorPath ep; - // Check the iterator is created before the change - if (val.tokvalue != tok && reaches(val.tokvalue, tok, library, &ep)) { - v = &val; - errorPath = ep; - return true; - } - } - return false; - }); - if (!info.tok) - continue; - errorPath.insert(errorPath.end(), info.errorPath.begin(), info.errorPath.end()); - if (v) { - invalidContainerError(info.tok, tok, v, errorPath); - } else { - invalidContainerReferenceError(info.tok, tok, errorPath); } } } @@ -1011,8 +1113,6 @@ void CheckStl::invalidContainerLoopError(const Token *tok, const Token * loopTok void CheckStl::invalidContainerError(const Token *tok, const Token * contTok, const ValueFlow::Value *val, ErrorPath errorPath) { const bool inconclusive = val ? val->isInconclusive() : false; - std::string method = contTok ? contTok->strAt(2) : "erase"; - errorPath.emplace_back(contTok, "After calling '" + method + "', iterators or references to the container's data may be invalid ."); if (val) errorPath.insert(errorPath.begin(), val->errorPath.begin(), val->errorPath.end()); std::string msg = "Using " + lifetimeMessage(tok, val, errorPath); @@ -1022,10 +1122,7 @@ void CheckStl::invalidContainerError(const Token *tok, const Token * contTok, co void CheckStl::invalidContainerReferenceError(const Token* tok, const Token* contTok, ErrorPath errorPath) { - std::string method = contTok ? contTok->strAt(2) : "erase"; std::string name = contTok ? contTok->expressionString() : "x"; - errorPath.emplace_back( - contTok, "After calling '" + method + "', iterators or references to the container's data may be invalid ."); std::string msg = "Reference to " + name; errorPath.emplace_back(tok, ""); reportError(errorPath, Severity::error, "invalidContainerReference", msg + " that may be invalid.", CWE664, false); diff --git a/lib/utils.h b/lib/utils.h index 059196427..593f2b4f0 100644 --- a/lib/utils.h +++ b/lib/utils.h @@ -28,6 +28,22 @@ #include #include +struct SelectMapKeys { + template + typename Pair::first_type operator()(const Pair& p) const + { + return p.first; + } +}; + +struct SelectMapValues { + template + typename Pair::second_type operator()(const Pair& p) const + { + return p.second; + } +}; + inline bool endsWith(const std::string &str, char c) { return str[str.size()-1U] == c; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 2daaf5f13..767d5e0a4 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1753,20 +1753,6 @@ static bool bifurcate(const Token* tok, const std::set& varids, cons return false; } -struct SelectMapKeys { - template - typename Pair::first_type operator()(const Pair& p) const { - return p.first; - } -}; - -struct SelectMapValues { - template - typename Pair::second_type operator()(const Pair& p) const { - return p.second; - } -}; - struct ValueFlowAnalyzer : Analyzer { const TokenList* tokenlist; ProgramMemoryState pms; @@ -2470,16 +2456,6 @@ static void valueFlowReverse(Token* tok, } } -static int getArgumentPos(const Variable *var, const Function *f) -{ - auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable &v) { - return v.nameToken() == var->nameToken(); - }); - if (arg_it == f->argumentList.end()) - return -1; - return std::distance(f->argumentList.begin(), arg_it); -} - std::string lifetimeType(const Token *tok, const ValueFlow::Value *val) { std::string result; diff --git a/test/teststl.cpp b/test/teststl.cpp index 1bfc7d7bb..b97a4bab1 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -4446,6 +4446,38 @@ private: " return info.ret;\n" "}\n",true); ASSERT_EQUALS("", errout.str()); + + // #9133 + check("struct Fred {\n" + " std::vector v;\n" + " void foo();\n" + " void bar();\n" + "};\n" + "void Fred::foo() {\n" + " std::vector::iterator it = v.begin();\n" + " bar();\n" + " it++;\n" + "}\n" + "void Fred::bar() {\n" + " v.push_back(0);\n" + "}\n", + true); + ASSERT_EQUALS( + "[test.cpp:7] -> [test.cpp:8] -> [test.cpp:12] -> [test.cpp:2] -> [test.cpp:9]: (error) Using iterator to local container 'v' that may be invalid.\n", + errout.str()); + + check("void foo(std::vector& v) {\n" + " std::vector::iterator it = v.begin();\n" + " bar(v);\n" + " it++;\n" + "}\n" + "void bar(std::vector& v) {\n" + " v.push_back(0);\n" + "}\n", + true); + ASSERT_EQUALS( + "[test.cpp:1] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:7] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'v' that may be invalid.\n", + errout.str()); } void invalidContainerLoop() {