diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 9661f70ec..b0c2ae2b7 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1172,6 +1172,165 @@ static bool hasFunctionCall(const Token *tok) return hasFunctionCall(tok->astOperand1()) || hasFunctionCall(tok->astOperand2()); } +const Scope* PathAnalysis::findOuterScope(const Scope * scope) +{ + if (!scope) + return nullptr; + if (scope->isLocal() && scope->type != Scope::eSwitch) + return findOuterScope(scope->nestedIn); + return scope; +} + +static const Token* getCondTok(const Token* tok) +{ + if (!tok) + return nullptr; + if (Token::simpleMatch(tok, "(")) + return getCondTok(tok->previous()); + if (Token::simpleMatch(tok, "for") && Token::simpleMatch(tok->next()->astOperand2(), ";") && tok->next()->astOperand2()->astOperand2()) + return tok->next()->astOperand2()->astOperand2()->astOperand1(); + if (Token::simpleMatch(tok->next()->astOperand2(), ";")) + return tok->next()->astOperand2()->astOperand1(); + return tok->next()->astOperand2(); +} + +std::pair PathAnalysis::checkCond(const Token * tok, bool& known) +{ + if (tok->hasKnownIntValue()) { + known = true; + return std::make_pair(tok->values().front().intvalue, !tok->values().front().intvalue); + } + auto it = std::find_if(tok->values().begin(), tok->values().end(), [](const ValueFlow::Value& v) { + return v.isIntValue(); + }); + // If all possible values are the same, then assume all paths have the same value + if (it != tok->values().end() && std::all_of(it, tok->values().end(), [&](const ValueFlow::Value& v) { + if (v.isIntValue()) + return v.intvalue == it->intvalue; + return true; + })) { + known = false; + return std::make_pair(it->intvalue, !it->intvalue); + } + return std::make_pair(true, true); +} + +PathAnalysis::Progress PathAnalysis::forwardRecursive(const Token* tok, Info info, const std::function& f) const +{ + if (!tok) + return Progress::Continue; + if (tok->astOperand1() && forwardRecursive(tok->astOperand1(), info, f) == Progress::Break) + return Progress::Break; + info.tok = tok; + if (f(info) == Progress::Break) + return Progress::Break; + if (tok->astOperand2() && forwardRecursive(tok->astOperand2(), info, f) == Progress::Break) + return Progress::Break; + return Progress::Continue; +} + +PathAnalysis::Progress PathAnalysis::forwardRange(const Token* startToken, const Token* endToken, Info info, const std::function& f) const +{ + for (const Token *tok = startToken; tok && tok != endToken; tok = tok->next()) { + if (Token::Match(tok, "asm|goto|break|continue")) + return Progress::Break; + if (Token::Match(tok, "return|throw")) { + forwardRecursive(tok, info, f); + return Progress::Break; + } + if (Token::simpleMatch(tok, "}") && Token::simpleMatch(tok->link()->previous(), ") {") && Token::Match(tok->link()->linkAt(-1)->previous(), "if|while|for (")) { + const Token * blockStart = tok->link()->linkAt(-1)->previous(); + const Token * condTok = getCondTok(blockStart); + if (!condTok) + continue; + info.errorPath.emplace_back(condTok, "Assuming condition is true."); + // Traverse a loop a second time + if (Token::Match(blockStart, "for|while (")) { + const Token* endCond = blockStart->linkAt(1); + bool traverseLoop = true; + // Only traverse simple for loops + if (Token::simpleMatch(blockStart, "for") && !Token::Match(endCond->tokAt(-3), "; ++|--|%var% %var%|++|-- ) {")) + traverseLoop = false; + // Traverse loop a second time + if (traverseLoop) { + // Traverse condition + if (forwardRecursive(condTok, info, f) == Progress::Break) + return Progress::Break; + // TODO: Should we traverse the body: forwardRange(tok->link(), tok, info, f)? + } + } + } + if (Token::Match(tok, "if|while|for (") && Token::simpleMatch(tok->next()->link(), ") {")) { + const Token * endCond = tok->next()->link(); + const Token * endBlock = endCond->next()->link(); + const Token * condTok = getCondTok(tok); + if (!condTok) + continue; + // Traverse condition + if (forwardRange(tok->next(), tok->next()->link(), info, f) == Progress::Break) + return Progress::Break; + Info i = info; + i.known = false; + i.errorPath.emplace_back(condTok, "Assuming condition is true."); + + // Check if condition is true or false + bool checkThen = false; + bool checkElse = false; + std::tie(checkThen, checkElse) = checkCond(condTok, i.known); + + // Traverse then block + if (checkThen) { + if (forwardRange(endCond->next(), endBlock, i, f) == Progress::Break) + return Progress::Break; + } + // Traverse else block + if (Token::simpleMatch(endBlock, "} else {")) { + if (checkElse) { + i.errorPath.back().second = "Assuming condition is false."; + Progress result = forwardRange(endCond->next(), endBlock, i, f); + if (result == Progress::Break) + return Progress::Break; + } + tok = endBlock->linkAt(2); + } else { + tok = endBlock; + } + } else if (Token::simpleMatch(tok, "} else {")) { + tok = tok->linkAt(2); + } else { + info.tok = tok; + if (f(info) == Progress::Break) + return Progress::Break; + } + // Prevent inifinite recursion + if (tok->next() == start) + break; + } + return Progress::Continue; +} + +void PathAnalysis::forward(const std::function& f) const +{ + const Scope * endScope = findOuterScope(start->scope()); + if (!endScope) + return; + const Token * endToken = endScope->bodyEnd; + Info info{start, ErrorPath{}, true}; + forwardRange(start, endToken, info, f); +} + +bool reaches(const Token * start, const Token * dest, const Library& library, ErrorPath* errorPath) +{ + PathAnalysis::Info info = PathAnalysis{start, library} .forwardFind([&](const PathAnalysis::Info& i) { + return (i.tok == dest); + }); + if (!info.tok) + return false; + if (errorPath) + errorPath->insert(errorPath->end(), info.errorPath.begin(), info.errorPath.end()); + return true; +} + struct FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *expr, const Token *startToken, const Token *endToken, const std::set &exprVarIds, bool local, bool inInnerClass) { // Parse the given tokens diff --git a/lib/astutils.h b/lib/astutils.h index 2d702ed43..34a3e93ca 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -31,6 +31,7 @@ class Library; class Settings; +class Scope; class Token; class Variable; @@ -169,6 +170,62 @@ bool isConstVarExpression(const Token *tok); const Variable *getLHSVariable(const Token *tok); +struct PathAnalysis { + enum Progress { + Continue, + Break + }; + PathAnalysis(const Token* start, const Library& library) + : start(start), library(&library) + {} + const Token * start; + const Library * library; + + struct Info { + const Token* tok; + ErrorPath errorPath; + bool known; + }; + + void forward(const std::function& f) const; + template + void forwardAll(F f) { + forward([&](const Info& info) { + f(info); + return Progress::Continue; + }); + } + template + Info forwardFind(Predicate pred) { + Info result{}; + forward([&](const Info& info) { + if (pred(info)) { + result = info; + return Progress::Break; + } + return Progress::Continue; + }); + return result; + } +private: + + Progress forwardRecursive(const Token* tok, Info info, const std::function& f) const; + Progress forwardRange(const Token* startToken, const Token* endToken, Info info, const std::function& f) const; + + static const Scope* findOuterScope(const Scope * scope); + + static std::pair checkCond(const Token * tok, bool& known); +}; + +/** + * @brief Returns true if there is a path between the two tokens + * + * @param start Starting point of the path + * @param dest The path destination + * @param errorPath Adds the path traversal to the errorPath + */ +bool reaches(const Token * start, const Token * dest, const Library& library, ErrorPath* errorPath); + /** * Forward data flow analysis for checks * - unused value diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index 54bb07a47..50d2e7a7f 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -631,38 +631,6 @@ void CheckAutoVariables::checkVarLifetime() } } -static std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, ErrorPath &errorPath) -{ - const Token *tokvalue = val ? val->tokvalue : nullptr; - const Variable *tokvar = tokvalue ? tokvalue->variable() : nullptr; - const Token *vartok = tokvar ? tokvar->nameToken() : nullptr; - std::string type = lifetimeType(tok, val); - std::string msg = type; - if (vartok) { - errorPath.emplace_back(vartok, "Variable created here."); - const Variable * var = vartok->variable(); - if (var) { - switch (val->lifetimeKind) { - case ValueFlow::Value::LifetimeKind::Object: - case ValueFlow::Value::LifetimeKind::Address: - if (type == "pointer") - msg += " to local variable"; - else - msg += " that points to local variable"; - break; - case ValueFlow::Value::LifetimeKind::Lambda: - msg += " that captures local variable"; - break; - case ValueFlow::Value::LifetimeKind::Iterator: - msg += " to local container"; - break; - } - msg += " '" + var->name() + "'"; - } - } - return msg; -} - void CheckAutoVariables::errorReturnDanglingLifetime(const Token *tok, const ValueFlow::Value *val) { ErrorPath errorPath = val ? val->errorPath : ErrorPath(); diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index f62770e0f..e09c06ee9 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -377,6 +377,15 @@ static const Token* findIteratorContainer(const Token* start, const Token* end, return containerToken; } +static bool isVector(const Token* tok) +{ + if (!tok) + return false; + const Variable *var = tok->variable(); + const Token *decltok = var ? var->typeStartToken() : nullptr; + return Token::simpleMatch(decltok, "std :: vector"); +} + void CheckStl::iterators() { const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); @@ -439,7 +448,7 @@ void CheckStl::iterators() } // Is the iterator used in a insert/erase operation? - else if (Token::Match(tok2, "%name% . insert|erase ( *| %varid% )|,", iteratorId)) { + else if (Token::Match(tok2, "%name% . insert|erase ( *| %varid% )|,", iteratorId) && !isVector(tok2)) { const Token* itTok = tok2->tokAt(4); if (itTok->str() == "*") { if (tok2->strAt(2) == "insert") @@ -772,6 +781,83 @@ void CheckStl::mismatchingContainers() } } +static bool isInvalidMethod(const Token * tok) +{ + if (Token::Match(tok->next(), ". assign|clear")) + return true; + if (Token::Match(tok->next(), "%assign%")) + return true; + if (isVector(tok) && Token::Match(tok->next(), ". insert|emplace|emplace_back|push_back|erase|pop_back|reserve (")) + return true; + return false; +} + +void CheckStl::invalidContainer() +{ + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + const Library& library = mSettings->library; + 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; + // Skip if the variable is assigned to + unsigned int skipVarId = 0; + if (Token::Match(tok->astTop(), "%assign%") && Token::Match(tok->astTop()->previous(), "%var%")) + skipVarId = tok->astTop()->previous()->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() == skipVarId) + return false; + for (const ValueFlow::Value& val:info.tok->values()) { + if (!val.isLocalLifetimeValue()) + continue; + if (!val.tokvalue->variable()) + continue; + if (val.tokvalue->varId() != tok->varId()) + continue; + // Skip possible temporaries + if (val.tokvalue == tok) + continue; + ErrorPath ep; + // Check the iterator is created before the change + if (reaches(val.tokvalue, tok, library, &ep)) { + v = &val; + errorPath = ep; + return true; + } + } + return false; + }); + if (!info.tok || !v) + continue; + errorPath.insert(errorPath.end(), info.errorPath.begin(), info.errorPath.end()); + invalidContainerError(info.tok, tok, v, errorPath); + } + } +} + +void CheckStl::invalidContainerError(const Token *tok, const Token * contTok, const ValueFlow::Value *val, ErrorPath errorPath) +{ + 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); + errorPath.emplace_back(tok, ""); + reportError(errorPath, Severity::error, "invalidContainer", msg + " that may be invalid.", CWE664, false); +} void CheckStl::stlOutOfBounds() { @@ -921,6 +1007,9 @@ void CheckStl::eraseCheckLoopVar(const Scope &scope, const Variable *var) continue; if (!Token::Match(tok->tokAt(-2), ". erase ( ++| %varid% )", var->declarationId())) continue; + // Vector erases are handled by invalidContainer check + if (isVector(tok->tokAt(-3))) + continue; if (Token::simpleMatch(tok->astParent(), "=")) continue; // Iterator is invalid.. @@ -952,162 +1041,6 @@ void CheckStl::eraseCheckLoopVar(const Scope &scope, const Variable *var) } } -void CheckStl::pushback() -{ - // Pointer can become invalid after push_back, push_front, reserve or resize.. - const SymbolDatabase* const symbolDatabase = mTokenizer->getSymbolDatabase(); - for (const Scope * scope : symbolDatabase->functionScopes) { - for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { - if (Token::Match(tok, "%var% = & %var% [")) { - // Skip it directly if it is a pointer or an array - const Token* containerTok = tok->tokAt(3); - if (containerTok->variable() && containerTok->variable()->isArrayOrPointer()) - continue; - - // Variable id for pointer - const int pointerId(tok->varId()); - - bool invalidPointer = false; - const Token* function = nullptr; - const Token* end2 = tok->scope()->bodyEnd; - for (const Token *tok2 = tok; tok2 != end2; tok2 = tok2->next()) { - // push_back on vector.. - if (Token::Match(tok2, "%varid% . push_front|push_back|insert|reserve|resize|clear", containerTok->varId())) { - invalidPointer = true; - function = tok2->tokAt(2); - } - - // Using invalid pointer.. - if (invalidPointer && tok2->varId() == pointerId) { - bool unknown = false; - if (CheckNullPointer::isPointerDeRef(tok2, unknown, mSettings)) - invalidPointerError(tok2, function->str(), tok2->str()); - break; - } - } - } - } - } - - // Iterator becomes invalid after reserve, resize, insert, push_back or push_front.. - for (const Variable* var : symbolDatabase->variableList()) { - // Check that its an iterator - if (!var || !var->isLocal() || !Token::Match(var->typeEndToken(), "iterator|const_iterator|reverse_iterator|const_reverse_iterator")) - continue; - - const int iteratorId = var->declarationId(); - - // ... on std::vector - if (!Token::Match(var->typeStartToken(), "std| ::| vector <")) - continue; - - // the variable id for the vector - int vectorid = 0; - - const Token* validatingToken = nullptr; - - std::string invalidIterator; - const Token* end2 = var->scope()->bodyEnd; - for (const Token *tok2 = var->nameToken(); tok2 != end2; tok2 = tok2->next()) { - - if (validatingToken == tok2) { - invalidIterator.clear(); - validatingToken = nullptr; - } - - // Using push_back or push_front inside a loop.. - if (Token::simpleMatch(tok2, "for (")) { - tok2 = tok2->tokAt(2); - } - - if (Token::Match(tok2, "%varid% = %var% . begin|rbegin|cbegin|crbegin ( ) ; %varid% != %var% . end|rend|cend|crend ( ) ; ++| %varid% ++| ) {", iteratorId)) { - // variable id for the loop iterator - const int varId(tok2->tokAt(2)->varId()); - - const Token *pushbackTok = nullptr; - - // Count { and } for tok3 - const Token *tok3 = tok2->tokAt(20); - for (const Token* const end3 = tok3->linkAt(-1); tok3 != end3; tok3 = tok3->next()) { - if (tok3->str() == "break" || tok3->str() == "return") { - pushbackTok = nullptr; - break; - } else if (Token::Match(tok3, "%varid% . push_front|push_back|insert|reserve|resize|clear|erase (", varId) && !tok3->previous()->isAssignmentOp()) { - if (tok3->strAt(2) != "erase" || (tok3->tokAt(4)->varId() != iteratorId && tok3->tokAt(5)->varId() != iteratorId)) // This case is handled in: CheckStl::iterators() - pushbackTok = tok3->tokAt(2); - } - } - - if (pushbackTok) - invalidIteratorError(pushbackTok, pushbackTok->str(), tok2->str()); - } - - // Assigning iterator.. - if (Token::Match(tok2, "%varid% =", iteratorId)) { - if (Token::Match(tok2->tokAt(2), "%var% . begin|end|rbegin|rend|cbegin|cend|crbegin|crend|insert|erase|find (")) { - if (!invalidIterator.empty() && Token::Match(tok2->tokAt(4), "insert|erase ( *| %varid% )|,", iteratorId)) { - invalidIteratorError(tok2, invalidIterator, var->name()); - break; - } - vectorid = tok2->tokAt(2)->varId(); - tok2 = tok2->linkAt(5); - } else { - vectorid = 0; - } - invalidIterator.clear(); - } - - // push_back on vector.. - if (vectorid > 0 && Token::Match(tok2, "%varid% . push_front|push_back|insert|reserve|resize|clear|erase (", vectorid)) { - if (!invalidIterator.empty() && Token::Match(tok2->tokAt(2), "insert|erase ( *| %varid% ,|)", iteratorId)) { - invalidIteratorError(tok2, invalidIterator, var->name()); - break; - } - - if (tok2->strAt(2) != "erase" || (tok2->tokAt(4)->varId() != iteratorId && tok2->tokAt(5)->varId() != iteratorId)) // This case is handled in: CheckStl::iterators() - invalidIterator = tok2->strAt(2); - tok2 = tok2->linkAt(3); - } - - else if (tok2->str() == "return" || tok2->str() == "throw") - validatingToken = Token::findsimplematch(tok2->next(), ";"); - - // TODO: instead of bail out for 'else' try to check all execution paths. - else if (tok2->str() == "break" || tok2->str() == "else") - invalidIterator.clear(); - - // Using invalid iterator.. - if (!invalidIterator.empty()) { - if (Token::Match(tok2, "++|--|*|+|-|(|,|=|!= %varid%", iteratorId)) - invalidIteratorError(tok2, invalidIterator, tok2->strAt(1)); - if (Token::Match(tok2, "%varid% ++|--|+|-|.", iteratorId)) - invalidIteratorError(tok2, invalidIterator, tok2->str()); - } - } - } -} - - -// Error message for bad iterator usage.. -void CheckStl::invalidIteratorError(const Token *tok, const std::string &func, const std::string &iterator_name) -{ - reportError(tok, Severity::error, "invalidIterator2", - "$symbol:" + func + "\n" - "$symbol:" + iterator_name + "\n" - "After " + func + "(), the iterator '" + iterator_name + "' may be invalid.", CWE664, false); -} - - -// Error message for bad iterator usage.. -void CheckStl::invalidPointerError(const Token *tok, const std::string &func, const std::string &pointer_name) -{ - reportError(tok, Severity::error, "invalidPointer", - "$symbol:" + func + "\n" - "$symbol:" + pointer_name + "\n" - "Invalid pointer '" + pointer_name + "' after " + func + "().", CWE664, false); -} - - void CheckStl::stlBoundaries() { const SymbolDatabase* const symbolDatabase = mTokenizer->getSymbolDatabase(); diff --git a/lib/checkstl.h b/lib/checkstl.h index af0f38c68..78565b2d1 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -68,7 +68,6 @@ public: checkStl.missingComparison(); checkStl.outOfBounds(); checkStl.outOfBoundsIndexExpression(); - checkStl.pushback(); checkStl.redundantCondition(); checkStl.string_c_str(); checkStl.uselessCalls(); @@ -76,6 +75,10 @@ public: checkStl.stlOutOfBounds(); checkStl.negativeIndex(); + + checkStl.invalidContainer(); + checkStl.mismatchingContainers(); + checkStl.stlBoundaries(); checkStl.checkDereferenceInvalidIterator(); @@ -106,6 +109,8 @@ public: */ void iterators(); + void invalidContainer(); + /** * Mismatching containers: * std::find(foo.begin(), bar.end(), x) @@ -119,12 +124,6 @@ public: void erase(); void eraseCheckLoopVar(const Scope& scope, const Variable* var); - - /** - * Dangerous usage of push_back and insert - */ - void pushback(); - /** * bad condition.. "it < alist.end()" */ @@ -201,13 +200,12 @@ private: void mismatchingContainersError(const Token* tok); void mismatchingContainerExpressionError(const Token *tok1, const Token *tok2); void sameIteratorExpressionError(const Token *tok); - void invalidIteratorError(const Token* tok, const std::string& func, const std::string& iterator_name); - void invalidPointerError(const Token* tok, const std::string& func, const std::string& pointer_name); void stlBoundariesError(const Token* tok); void if_findError(const Token* tok, bool str); void checkFindInsertError(const Token *tok); void sizeError(const Token* tok); void redundantIfRemoveError(const Token* tok); + void invalidContainerError(const Token *tok, const Token * contTok, const ValueFlow::Value *val, ErrorPath errorPath); void uselessCallsReturnValueError(const Token* tok, const std::string& varname, const std::string& function); void uselessCallsSwapError(const Token* tok, const std::string& varname); @@ -224,6 +222,7 @@ private: bool compareIteratorAgainstDifferentContainer(const Token* operatorTok, const Token* containerTok, const nonneg int iteratorId, const std::map& iteratorScopeBeginInfo); void getErrorMessages(ErrorLogger* errorLogger, const Settings* settings) const OVERRIDE { + ErrorPath errorPath; CheckStl c(nullptr, settings, errorLogger); c.outOfBoundsError(nullptr, "container", nullptr, "x", nullptr); c.invalidIteratorError(nullptr, "iterator"); @@ -232,14 +231,13 @@ private: c.iteratorsError(nullptr, nullptr, "container"); c.iteratorsCmpError(nullptr, nullptr, nullptr, "container1", "container2"); c.iteratorsCmpError(nullptr, nullptr, nullptr, "container"); + c.invalidContainerError(nullptr, nullptr, nullptr, errorPath); c.mismatchingContainersError(nullptr); c.mismatchingContainerExpressionError(nullptr, nullptr); c.sameIteratorExpressionError(nullptr); c.dereferenceErasedError(nullptr, nullptr, "iter", false); c.stlOutOfBoundsError(nullptr, "i", "foo", false); c.negativeIndexError(nullptr, ValueFlow::Value(-1)); - c.invalidIteratorError(nullptr, "push_back|push_front|insert", "iterator"); - c.invalidPointerError(nullptr, "push_back", "pointer"); c.stlBoundariesError(nullptr); c.if_findError(nullptr, false); c.if_findError(nullptr, true); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index b8749cc8f..381bd0e57 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2706,6 +2706,38 @@ std::string lifetimeType(const Token *tok, const ValueFlow::Value *val) return result; } +std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, ErrorPath &errorPath) +{ + const Token *tokvalue = val ? val->tokvalue : nullptr; + const Variable *tokvar = tokvalue ? tokvalue->variable() : nullptr; + const Token *vartok = tokvar ? tokvar->nameToken() : nullptr; + std::string type = lifetimeType(tok, val); + std::string msg = type; + if (vartok) { + errorPath.emplace_back(vartok, "Variable created here."); + const Variable * var = vartok->variable(); + if (var) { + switch (val->lifetimeKind) { + case ValueFlow::Value::LifetimeKind::Object: + case ValueFlow::Value::LifetimeKind::Address: + if (type == "pointer") + msg += " to local variable"; + else + msg += " that points to local variable"; + break; + case ValueFlow::Value::LifetimeKind::Lambda: + msg += " that captures local variable"; + break; + case ValueFlow::Value::LifetimeKind::Iterator: + msg += " to local container"; + break; + } + msg += " '" + var->name() + "'"; + } + } + return msg; +} + ValueFlow::Value getLifetimeObjValue(const Token *tok) { ValueFlow::Value result; @@ -3339,7 +3371,7 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); } // container lifetimes - else if (tok->variable() && Token::Match(tok, "%var% . begin|cbegin|rbegin|crbegin|end|cend|rend|crend|data|c_str|find (")) { + else if (tok->variable() && Token::Match(tok, "%var% . begin|cbegin|rbegin|crbegin|end|cend|rend|crend|data|c_str|find|insert (")) { if (Token::simpleMatch(tok->tokAt(2), "find") && !astIsIterator(tok->tokAt(3))) continue; ErrorPath errorPath; diff --git a/lib/valueflow.h b/lib/valueflow.h index 3fc77585f..54e46b65d 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -243,6 +243,8 @@ bool isLifetimeBorrowed(const Token *tok, const Settings *settings); std::string lifetimeType(const Token *tok, const ValueFlow::Value *val); +std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, ValueFlow::Value::ErrorPath &errorPath); + ValueFlow::Value getLifetimeObjValue(const Token *tok); #endif // valueflowH diff --git a/samples/erase/out.txt b/samples/erase/out.txt index d89d1f8ab..0cf04d01b 100644 --- a/samples/erase/out.txt +++ b/samples/erase/out.txt @@ -1 +1 @@ -[samples\erase\bad.cpp:9] -> [samples\erase\bad.cpp:11]: (error) Iterator 'iter' used after element has been erased. +[samples\erase\bad.cpp:9] -> [samples\erase\bad.cpp:10] -> [samples\erase\bad.cpp:10] -> [samples\erase\bad.cpp:9] -> [samples\erase\bad.cpp:11] -> [samples\erase\bad.cpp:4] -> [samples\erase\bad.cpp:9]: (error) Using iterator to local container 'items' that may be invalid. diff --git a/test/teststl.cpp b/test/teststl.cpp index c2b05d6e9..282be664b 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -156,6 +156,8 @@ private: TEST_CASE(loopAlgoIncrement); TEST_CASE(loopAlgoConditional); TEST_CASE(loopAlgoMinMax); + + TEST_CASE(invalidContainer); TEST_CASE(findInsert); } @@ -1149,7 +1151,7 @@ private: " ints.erase(iter);\n" " std::cout << (*iter) << std::endl;\n" "}"); - ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (error) Iterator 'iter' used after element has been erased.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:6] -> [test.cpp:3] -> [test.cpp:7]: (error) Using iterator to local container 'ints' that may be invalid.\n", errout.str()); // #6554 "False positive eraseDereference - erase in while() loop" check("typedef std::map packetMap;\n" @@ -1220,7 +1222,7 @@ private: " ints.erase(iter);\n" " std::cout << (*iter) << std::endl;\n" "}"); - ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:5]: (error) Iterator 'iter' used after element has been erased.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5] -> [test.cpp:3] -> [test.cpp:6]: (error) Using iterator to local container 'ints' that may be invalid.\n", errout.str()); check("void f() {\n" " auto x = *myList.begin();\n" @@ -1631,7 +1633,7 @@ private: " }\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:9]: (error) Iterator 'it' used after element has been erased.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:7] -> [test.cpp:8] -> [test.cpp:8] -> [test.cpp:7] -> [test.cpp:5] -> [test.cpp:9] -> [test.cpp:3] -> [test.cpp:5]: (error) Using iterator to local container 'foo' that may be invalid.\n", errout.str()); } void eraseGoto() { @@ -1719,7 +1721,7 @@ private: " ints.erase(iter);\n" " ints.erase(iter);\n" "}"); - ASSERT_EQUALS("[test.cpp:6]: (error) Invalid iterator: iter\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5] -> [test.cpp:1] -> [test.cpp:6]: (error) Using iterator to local container 'ints' that may be invalid.\n", errout.str()); } void eraseByValue() { @@ -1757,15 +1759,15 @@ private: ASSERT_EQUALS("", errout.str()); check("void f(const std::list& m_ImplementationMap) {\n" - " std::list::iterator aIt = m_ImplementationMap.find( xEle );\n" + " std::list::iterator aIt = m_ImplementationMap.begin();\n" " m_ImplementationMap.erase(*aIt);\n" " m_ImplementationMap.erase(aIt);\n" "}"); ASSERT_EQUALS("[test.cpp:4]: (error) Invalid iterator: aIt\n", errout.str()); check("void f(const std::list& m_ImplementationMap) {\n" - " std::list::iterator aIt = m_ImplementationMap.find( xEle1 );\n" - " std::list::iterator bIt = m_ImplementationMap.find( xEle2 );\n" + " std::list::iterator aIt = m_ImplementationMap.begin();\n" + " std::list::iterator bIt = m_ImplementationMap.begin();\n" " m_ImplementationMap.erase(*bIt);\n" " m_ImplementationMap.erase(aIt);\n" "}"); @@ -1788,26 +1790,26 @@ private: void eraseOnVector() { check("void f(const std::vector& m_ImplementationMap) {\n" - " std::vector::iterator aIt = m_ImplementationMap.find( xEle );\n" + " std::vector::iterator aIt = m_ImplementationMap.begin();\n" " m_ImplementationMap.erase(something(unknown));\n" // All iterators become invalidated when erasing from std::vector " m_ImplementationMap.erase(aIt);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) After erase(), the iterator 'aIt' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'm_ImplementationMap' that may be invalid.\n", errout.str()); check("void f(const std::vector& m_ImplementationMap) {\n" - " std::vector::iterator aIt = m_ImplementationMap.find( xEle );\n" + " std::vector::iterator aIt = m_ImplementationMap.begin();\n" " m_ImplementationMap.erase(*aIt);\n" // All iterators become invalidated when erasing from std::vector " m_ImplementationMap.erase(aIt);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Invalid iterator: aIt\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'm_ImplementationMap' that may be invalid.\n", errout.str()); check("void f(const std::vector& m_ImplementationMap) {\n" - " std::vector::iterator aIt = m_ImplementationMap.find( xEle1 );\n" - " std::vector::iterator bIt = m_ImplementationMap.find( xEle2 );\n" + " std::vector::iterator aIt = m_ImplementationMap.begin();\n" + " std::vector::iterator bIt = m_ImplementationMap.begin();\n" " m_ImplementationMap.erase(*bIt);\n" // All iterators become invalidated when erasing from std::vector " aIt = m_ImplementationMap.erase(aIt);\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) After erase(), the iterator 'aIt' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4] -> [test.cpp:1] -> [test.cpp:5]: (error) Using iterator to local container 'm_ImplementationMap' that may be invalid.\n", errout.str()); } void pushback1() { @@ -1817,7 +1819,7 @@ private: " foo.push_back(123);\n" " *it;\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) After push_back(), the iterator 'it' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4] -> [test.cpp:1] -> [test.cpp:5]: (error) Using iterator to local container 'foo' that may be invalid.\n", errout.str()); } void pushback2() { @@ -1844,7 +1846,7 @@ private: " foo.push_back(123);\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:8]: (error) After push_back(), the iterator 'it' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:6] -> [test.cpp:8] -> [test.cpp:3] -> [test.cpp:6]: (error) Using iterator to local container 'foo' that may be invalid.\n", errout.str()); } void pushback4() { @@ -1856,7 +1858,7 @@ private: " ints.push_back(2);\n" " *first;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (error) Invalid pointer 'first' after push_back().\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:6] -> [test.cpp:3] -> [test.cpp:7]: (error) Using pointer to local variable 'ints' that may be invalid.\n", errout.str()); } void pushback5() { @@ -1880,16 +1882,16 @@ private: // ticket #735 check("void f()\n" "{\n" - " vector v;\n" + " std::vector v;\n" " v.push_back(1);\n" " v.push_back(2);\n" - " for (vector::iterator it = v.begin(); it != v.end(); ++it)\n" + " for (std::vector::iterator it = v.begin(); it != v.end(); ++it)\n" " {\n" " if (*it == 1)\n" " v.push_back(10);\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:9]: (error) After push_back(), the iterator 'it' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:8] -> [test.cpp:8] -> [test.cpp:6] -> [test.cpp:9] -> [test.cpp:3] -> [test.cpp:6]: (error) Using iterator to local container 'v' that may be invalid.\n", errout.str()); check("void f()\n" "{\n" @@ -1902,7 +1904,7 @@ private: " v.push_back(10);\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:9]: (error) After push_back(), the iterator 'it' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:8] -> [test.cpp:8] -> [test.cpp:6] -> [test.cpp:9] -> [test.cpp:3] -> [test.cpp:6]: (error) Using iterator to local container 'v' that may be invalid.\n", errout.str()); } void pushback7() { @@ -1916,7 +1918,7 @@ private: " foo.push_back(123);\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:8]: (error) After push_back(), the iterator 'it' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:6] -> [test.cpp:8] -> [test.cpp:3] -> [test.cpp:6]: (error) Using iterator to local container 'foo' that may be invalid.\n", errout.str()); } void pushback8() { @@ -1932,7 +1934,7 @@ private: " sum += *it;\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:8]: (error) After push_back(), the iterator 'end' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5] -> [test.cpp:3] -> [test.cpp:8]: (error) Using iterator to local container 'ints' that may be invalid.\n", errout.str()); } void pushback9() { @@ -1962,7 +1964,7 @@ private: " foo.reserve(100);\n" " *it = 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) After reserve(), the iterator 'it' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4] -> [test.cpp:1] -> [test.cpp:5]: (error) Using iterator to local container 'foo' that may be invalid.\n", errout.str()); // in loop check("void f()\n" @@ -1975,7 +1977,7 @@ private: " foo.reserve(123);\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:8]: (error) After reserve(), the iterator 'it' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:6] -> [test.cpp:8] -> [test.cpp:3] -> [test.cpp:6]: (error) Using iterator to local container 'foo' that may be invalid.\n", errout.str()); } void pushback11() { @@ -2005,8 +2007,7 @@ private: " *it;\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:6]: (error) After insert(), the iterator 'it' may be invalid.\n" - "[test.cpp:9]: (error) After insert(), the iterator 'it' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:6] -> [test.cpp:3] -> [test.cpp:9]: (error) Using iterator to local container 'vec' that may be invalid.\n", errout.str()); } void pushback13() { @@ -2026,7 +2027,7 @@ private: " ints.insert(ints.begin(), 1);\n" " ++iter;\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) After insert(), the iterator 'iter' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4] -> [test.cpp:1] -> [test.cpp:5]: (error) Using iterator to local container 'ints' that may be invalid.\n", errout.str()); check("void f()\n" "{\n" @@ -2043,7 +2044,7 @@ private: " ints.insert(iter, 1);\n" " ints.insert(iter, 2);\n" "}"); - ASSERT_EQUALS("[test.cpp:6]: (error) After insert(), the iterator 'iter' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5] -> [test.cpp:3] -> [test.cpp:6]: (error) Using iterator to local container 'ints' that may be invalid.\n", errout.str()); check("void* f(const std::vector& bars) {\n" " std::vector::iterator i = bars.begin();\n" @@ -2051,14 +2052,14 @@ private: " void* v = &i->foo;\n" " return v;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) After insert(), the iterator 'i' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'bars' that may be invalid.\n", errout.str()); check("Foo f(const std::vector& bars) {\n" " std::vector::iterator i = bars.begin();\n" " bars.insert(Bar());\n" " return i->foo;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) After insert(), the iterator 'i' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'bars' that may be invalid.\n", errout.str()); check("void f(const std::vector& bars) {\n" " for(std::vector::iterator i = bars.begin(); i != bars.end(); ++i) {\n" @@ -2073,8 +2074,7 @@ private: " i = bars.insert(i, bar);\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) After insert(), the iterator 'i' may be invalid.\n" - "[test.cpp:4]: (error) After insert(), the iterator 'i' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:2]: (error) Using iterator to local container 'bars' that may be invalid.\n", errout.str()); check("void* f(const std::vector& bars) {\n" " std::vector::iterator i = bars.begin();\n" @@ -2083,7 +2083,8 @@ private: " void* v = &i->foo;\n" " return v;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) After insert(), the iterator 'i' may be invalid.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:1] -> [test.cpp:5] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:6]: (error) Using pointer to local variable 'bars' that may be invalid.\n" + "[test.cpp:4] -> [test.cpp:1] -> [test.cpp:5] -> [test.cpp:4] -> [test.cpp:1] -> [test.cpp:6]: (error) Using pointer to local variable 'bars' that may be invalid.\n", errout.str()); } void insert2() { @@ -3846,6 +3847,28 @@ private: ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::accumulate algorithm instead of a raw loop.\n", errout.str()); } + void invalidContainer() { + check("void f(std::vector &v) {\n" + " auto v0 = v.begin();\n" + " v.push_back(123);\n" + " std::cout << *v0 << std::endl;\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'v' that may be invalid.\n", errout.str()); + + check("std::string e();\n" + "void a() {\n" + " std::vector b;\n" + " for (std::vector::const_iterator c; c != b.end(); ++c) {\n" + " std::string f = e();\n" + " std::string::const_iterator d = f.begin();\n" + " if (d != f.end()) {}\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + } + void findInsert() { check("void f1(std::set& s, unsigned x) {\n" " if (s.find(x) == s.end()) {\n"