From e1377467a4783b9c7881f80cdc985926ae32833d Mon Sep 17 00:00:00 2001 From: Paul Date: Fri, 1 Jan 2021 18:41:09 -0600 Subject: [PATCH 1/8] Analyze invalid container across multiple functions --- lib/astutils.cpp | 10 ++ lib/astutils.h | 3 + lib/checkstl.cpp | 246 +++++++++++++++++++++++++++++++--------------- lib/utils.h | 14 +++ lib/valueflow.cpp | 24 ----- 5 files changed, 196 insertions(+), 101 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 1d9136560b7..5f3b4331855 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 a4f6d83a911..c637cc7654b 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 efd15cf45f4..6f51e9fda73 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -20,6 +20,7 @@ #include "check.h" #include "checknullpointer.h" +#include "errortypes.h" #include "library.h" #include "mathlib.h" #include "settings.h" @@ -38,6 +39,8 @@ #include #include #include +#include +#include #include // Register this check class (by creating a static instance of it) @@ -843,6 +846,96 @@ 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->next()->function(); + if (!f) + return {}; + 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_back(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)) + result.push_back(Info::Reference{tok, ErrorPath{}}); + } + 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 +954,90 @@ 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)) { - - 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))) { + 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() == 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(), r.errorPath.begin(), r.errorPath.end()); + errorPath.insert(errorPath.end(), info.errorPath.begin(), info.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); } } } diff --git a/lib/utils.h b/lib/utils.h index 059196427fd..b7479687953 100644 --- a/lib/utils.h +++ b/lib/utils.h @@ -28,6 +28,20 @@ #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 8ece5b7fa6b..879852770f9 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1955,20 +1955,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; @@ -2636,16 +2622,6 @@ static void valueFlowReverse(TokenList* tokenlist, } } -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; From f28b8875a46f6bdf503ab08b7a59149722606104 Mon Sep 17 00:00:00 2001 From: Paul Date: Fri, 1 Jan 2021 18:49:24 -0600 Subject: [PATCH 2/8] Fix function --- lib/checkstl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 6f51e9fda73..38350ec02e2 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -877,7 +877,7 @@ struct InvalidContainerAnalyzer std::vector invalidatesContainer(const Token* tok) const { std::vector result; if (Token::Match(tok, "%name% (")) { - const Function* f = tok->next()->function(); + const Function* f = tok->function(); if (!f) return {}; ErrorPathItem epi = std::make_pair(tok, "Calling function " + tok->str()); From 72d44ba8aee9bacd27fe13111bcbc0fedbc9a0bf Mon Sep 17 00:00:00 2001 From: Paul Date: Fri, 1 Jan 2021 19:05:50 -0600 Subject: [PATCH 3/8] Improve error path order --- lib/checkstl.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 38350ec02e2..406b1d4828d 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -899,7 +899,7 @@ struct InvalidContainerAnalyzer }); std::vector args = getArguments(tok); for(Info::Reference& r:result) { - r.errorPath.push_back(epi); + r.errorPath.push_front(epi); const Variable* var = r.tok->variable(); if (!var) continue; @@ -913,8 +913,11 @@ struct InvalidContainerAnalyzer } } } else if (astIsContainer(tok)) { - if (isInvalidMethod(tok)) - result.push_back(Info::Reference{tok, ErrorPath{}}); + 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; } @@ -1031,8 +1034,8 @@ void CheckStl::invalidContainer() }); if (!info.tok) continue; - errorPath.insert(errorPath.end(), r.errorPath.begin(), r.errorPath.end()); 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 { @@ -1103,8 +1106,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); @@ -1114,10 +1115,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); From 61ea1c5e60cbdfce8d49719f342f699b2739fb18 Mon Sep 17 00:00:00 2001 From: Paul Date: Sun, 3 Jan 2021 14:02:28 -0600 Subject: [PATCH 4/8] Add tests --- test/teststl.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/teststl.cpp b/test/teststl.cpp index 7db9d5c4f4f..6e6bddda330 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -4423,6 +4423,32 @@ class TestStl : public TestFixture { " 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() { From 8912f17715c754f7057c803bbf20cee7f4dc64a6 Mon Sep 17 00:00:00 2001 From: Paul Date: Sun, 3 Jan 2021 14:12:20 -0600 Subject: [PATCH 5/8] Format --- lib/astutils.cpp | 4 +- lib/astutils.h | 2 +- lib/checkstl.cpp | 127 +++++++++++++++++++++++++---------------------- lib/utils.h | 10 ++-- test/teststl.cpp | 14 ++++-- 5 files changed, 86 insertions(+), 71 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 5f3b4331855..d01bf404740 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1877,9 +1877,9 @@ std::vector getArguments(const Token *ftok) return astFlatten(startTok, ","); } -int getArgumentPos(const Variable *var, const Function *f) +int getArgumentPos(const Variable* var, const Function* f) { - auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable &v) { + 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()) diff --git a/lib/astutils.h b/lib/astutils.h index c637cc7654b..d506606e113 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -235,7 +235,7 @@ int numberOfArguments(const Token *start); */ std::vector getArguments(const Token *ftok); -int getArgumentPos(const Variable *var, const Function *f); +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 406b1d4828d..9359826164f 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -18,18 +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 @@ -846,8 +846,7 @@ static bool isInvalidMethod(const Token * tok) return false; } -struct InvalidContainerAnalyzer -{ +struct InvalidContainerAnalyzer { struct Info { struct Reference { const Token* tok; @@ -855,18 +854,21 @@ struct InvalidContainerAnalyzer }; std::unordered_map expressions; ErrorPath errorPath; - void add(const std::vector& refs) { - for(const Reference r:refs) { + void add(const std::vector& refs) + { + for (const Reference r : refs) { add(r); } } - void add(const Reference& 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 invalidTokens() const + { std::vector result; std::transform(expressions.begin(), expressions.end(), std::back_inserter(result), SelectMapValues{}); return result; @@ -874,7 +876,8 @@ struct InvalidContainerAnalyzer }; std::unordered_map invalidMethods; - std::vector invalidatesContainer(const Token* tok) const { + std::vector invalidatesContainer(const Token* tok) const + { std::vector result; if (Token::Match(tok, "%name% (")) { const Function* f = tok->function(); @@ -898,7 +901,7 @@ struct InvalidContainerAnalyzer return true; }); std::vector args = getArguments(tok); - for(Info::Reference& r:result) { + for (Info::Reference& r : result) { r.errorPath.push_front(epi); const Variable* var = r.tok->variable(); if (!var) @@ -915,19 +918,22 @@ struct InvalidContainerAnalyzer } 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 ."); + 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) { + 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()) { + 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); @@ -961,7 +967,7 @@ void CheckStl::invalidContainer() analyzer.analyze(symbolDatabase); for (const Scope * scope : symbolDatabase->functionScopes) { for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { - for(const InvalidContainerAnalyzer::Info::Reference& r:analyzer.invalidatesContainer(tok)) { + for (const InvalidContainerAnalyzer::Info::Reference& r : analyzer.invalidatesContainer(tok)) { if (!astIsContainer(r.tok)) continue; std::set skipVarIds; @@ -978,60 +984,61 @@ void CheckStl::invalidContainer() } if (Token::Match(assignExpr, "%assign%") && Token::Match(assignExpr->astOperand1(), "%var%")) skipVarIds.insert(assignExpr->astOperand1()->varId()); - const Token * endToken = nextAfterAstRightmostLeaf(tok->next()->astParent()); + 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() == r.tok->varId() && !addressOf) { - // An argument always reaches - if (var->isArgument() || (!var->isReference() && !var->isRValueReference() && - !isVariableDecl(tok) && reaches(var->nameToken(), tok, library, &ep))) { + 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() == 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; } } - } - 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; - }); + return false; + }); if (!info.tok) continue; errorPath.insert(errorPath.end(), info.errorPath.begin(), info.errorPath.end()); diff --git a/lib/utils.h b/lib/utils.h index b7479687953..593f2b4f020 100644 --- a/lib/utils.h +++ b/lib/utils.h @@ -29,15 +29,17 @@ #include struct SelectMapKeys { - template - typename Pair::first_type operator()(const Pair& p) const { + 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 { + template + typename Pair::second_type operator()(const Pair& p) const + { return p.second; } }; diff --git a/test/teststl.cpp b/test/teststl.cpp index 6e6bddda330..5faded1406c 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -4437,8 +4437,11 @@ class TestStl : public TestFixture { "}\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()); + "}\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" @@ -4447,8 +4450,11 @@ class TestStl : public TestFixture { "}\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()); + "}\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() { From 2d60c287659d193128dbddbc1dbc381cce606e32 Mon Sep 17 00:00:00 2001 From: Paul Date: Sun, 3 Jan 2021 20:59:53 -0600 Subject: [PATCH 6/8] Fix error on gcc 4.8 --- lib/checkstl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 9359826164f..1302f569b70 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -882,7 +882,7 @@ struct InvalidContainerAnalyzer { if (Token::Match(tok, "%name% (")) { const Function* f = tok->function(); if (!f) - return {}; + return result; ErrorPathItem epi = std::make_pair(tok, "Calling function " + tok->str()); const bool dependsOnThis = exprDependsOnThis(tok->next()); auto it = invalidMethods.find(f); From 0c2b83d5d9c5f23e8d32a35f2882af9e0aa7fb74 Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 4 Jan 2021 11:20:17 -0600 Subject: [PATCH 7/8] Add a reference --- lib/checkstl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 1302f569b70..241e2414c04 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -856,7 +856,7 @@ struct InvalidContainerAnalyzer { ErrorPath errorPath; void add(const std::vector& refs) { - for (const Reference r : refs) { + for (const Reference& r : refs) { add(r); } } From b46520c5d474c9bd51ef05f41b97a26a72b2a400 Mon Sep 17 00:00:00 2001 From: Paul Date: Sat, 9 Jan 2021 12:34:08 -0600 Subject: [PATCH 8/8] Remove function --- lib/valueflow.cpp | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index f091a7b079b..49220b7d592 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2548,16 +2548,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;