Fix issue 9133: Invalid iterator; vector::push_back, functions #3008
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 "valueflow.h" | ||
|
|
||
| #include <algorithm> | ||
|
|
@@ -38,6 +39,8 @@ | |
| #include <map> | ||
| #include <set> | ||
| #include <sstream> | ||
| #include <unordered_map> | ||
| #include <unordered_set> | ||
| #include <utility> | ||
|
|
||
| // 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<int, Reference> expressions; | ||
| ErrorPath errorPath; | ||
| void add(const std::vector<Reference>& 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<Reference> invalidTokens() const | ||
| { | ||
| std::vector<Reference> result; | ||
| std::transform(expressions.begin(), expressions.end(), std::back_inserter(result), SelectMapValues{}); | ||
| return result; | ||
| } | ||
| }; | ||
| std::unordered_map<const Function*, Info> invalidMethods; | ||
|
|
||
| std::vector<Info::Reference> invalidatesContainer(const Token* tok) const | ||
| { | ||
| std::vector<Info::Reference> 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<Info::Reference> refs = it->second.invalidTokens(); | ||
| std::copy_if(refs.begin(), refs.end(), std::back_inserter(result), [&](const Info::Reference& r) { | ||
|
Collaborator
There was a problem hiding this comment. I wonder if it would be a good idea to capture dependsOnThis by value. Does it actually capture everything in the context or are compilers smart enough and captures only dependsOnThis.
Contributor
Author
There was a problem hiding this comment. It only captures the variables used, but compilers can optimize it further and just capture the stack pointer instead of each variable used. Usually, its inlined so there is no copy(or copy to pointer) or function call(see this example). There was a problem hiding this comment. It's good practice to limit the amount of variable captured. |
||
| 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<const Token*> args = getArguments(tok); | ||
| for (Info::Reference& r : result) { | ||
|
Collaborator
There was a problem hiding this comment. so I guess you will fix something here before we merge it?
Contributor
Author
There was a problem hiding this comment. What needs to be fixed?
Collaborator
There was a problem hiding this comment. I wonder .. if
Contributor
Author
There was a problem hiding this comment. Its not always empty. The
Collaborator
There was a problem hiding this comment. ok thanks for that clarification I somehow missed it. |
||
| 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<Info::Reference> 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()) { | ||
| for (const InvalidContainerAnalyzer::Info::Reference& r : analyzer.invalidatesContainer(tok)) { | ||
| if (!astIsContainer(r.tok)) | ||
| continue; | ||
| std::set<nonneg int> 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() == 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); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -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; | ||
| 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 name = contTok ? contTok->expressionString() : "x"; | ||
| std::string msg = "Reference to " + name; | ||
| errorPath.emplace_back(tok, ""); | ||
| reportError(errorPath, Severity::error, "invalidContainerReference", msg + " that may be invalid.", CWE664, false); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be a good idea to capture var by value not by reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It seems simpler and faster to capture by ref as it has lexical scoping and avoids creating copies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the motivation would be if we get some const safety. if it doesn't have a performance penalty.. that would be good. for a trivial example I made the assembler output from gcc is identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variables are already declared const.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that in the lambda you could write
var=nullptr;and that will happily compile. well.. maybe that compiles happily with a=also?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, it can be modified with
var=nullptr. I can make the pointer const.