From ba037837c98981d6d5045a3e941951e1c0953911 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 11 Sep 2019 19:25:09 +0200 Subject: [PATCH] Track lifetime across multiple returns This will now warn when doing something like this: ```cpp template const V& get_default(const T& t, const K& k, const V& v) { auto it = t.find(k); if (it == t.end()) return v; return it->second; } const int& bar(const std::unordered_map& m, int k) { auto x = 0; return get_default(m, k, x); } ``` The lifetime warning is considered inconclusive in this case. I also updated valueflow to no tinject inconclusive values unless `--inconclusive` flag is passed. This creates some false negatives because library functions are not configured to not modify their input parameters, and there are some checks that do not check if the value is inconclusive or not. --- lib/astutils.cpp | 31 ++-- lib/astutils.h | 3 + lib/checkautovariables.cpp | 29 ++-- lib/checkautovariables.h | 4 +- lib/checkstl.cpp | 3 +- lib/symboldatabase.cpp | 2 +- lib/valueflow.cpp | 337 +++++++++++++++++++++---------------- lib/valueflow.h | 32 ++++ test/CMakeLists.txt | 19 ++- test/cfg/runtests.sh | 4 +- test/cfg/std.c | 1 + test/cfg/std.cpp | 1 + test/testautovariables.cpp | 30 ++++ test/testio.cpp | 2 +- test/teststl.cpp | 18 +- 15 files changed, 324 insertions(+), 192 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 08b251322..9ff7d4884 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -292,6 +292,23 @@ bool precedes(const Token * tok1, const Token * tok2) return tok1->index() < tok2->index(); } +bool isAliasOf(const Token *tok, nonneg int varid) +{ + if (tok->varId() == varid) + return false; + if (tok->varId() == 0) + return false; + for (const ValueFlow::Value &val : tok->values()) { + if (!val.isLocalLifetimeValue()) + continue; + if (val.isInconclusive()) + continue; + if (val.tokvalue->varId() == varid) + return true; + } + return false; +} + static bool isAliased(const Token *startTok, const Token *endTok, nonneg int varid) { if (!precedes(startTok, endTok)) @@ -299,18 +316,8 @@ static bool isAliased(const Token *startTok, const Token *endTok, nonneg int var for (const Token *tok = startTok; tok != endTok; tok = tok->next()) { if (Token::Match(tok, "= & %varid% ;", varid)) return true; - if (tok->varId() == varid) - continue; - if (tok->varId() == 0) - continue; - if (!astIsPointer(tok)) - continue; - for (const ValueFlow::Value &val : tok->values()) { - if (!val.isLocalLifetimeValue()) - continue; - if (val.tokvalue->varId() == varid) - return true; - } + if (isAliasOf(tok, varid)) + return true; } return false; } diff --git a/lib/astutils.h b/lib/astutils.h index 29619b945..5786fa35b 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -154,6 +154,9 @@ bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp, const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); +/// If token is an alias if another variable +bool isAliasOf(const Token *tok, nonneg int varid); + bool isAliased(const Variable *var); /** Determines the number of arguments - if token is a function call or macro diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index e72294783..0b3e845db 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -548,12 +548,15 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token for (const Token *tok = start; tok && tok != end; tok = tok->next()) { // Return reference from function if (returnRef && Token::simpleMatch(tok->astParent(), "return")) { - ErrorPath errorPath; - const Variable *var = getLifetimeVariable(tok, errorPath); - if (var && !var->isGlobal() && !var->isStatic() && !var->isReference() && !var->isRValueReference() && - isInScope(var->nameToken(), tok->scope())) { - errorReturnReference(tok, errorPath); - continue; + for (const LifetimeToken& lt : getLifetimeTokens(tok)) { + const Variable* var = lt.token->variable(); + if (!var) + continue; + if (!var->isGlobal() && !var->isStatic() && !var->isReference() && !var->isRValueReference() && + isInScope(var->nameToken(), tok->scope())) { + errorReturnReference(tok, lt.errorPath, lt.inconclusive); + break; + } } // Assign reference to non-local variable } else if (Token::Match(tok->previous(), "&|&& %var% =") && tok->astParent() == tok->next() && @@ -635,33 +638,37 @@ void CheckAutoVariables::checkVarLifetime() void CheckAutoVariables::errorReturnDanglingLifetime(const Token *tok, const ValueFlow::Value *val) { + const bool inconclusive = val ? val->isInconclusive() : false; ErrorPath errorPath = val ? val->errorPath : ErrorPath(); std::string msg = "Returning " + lifetimeMessage(tok, val, errorPath); errorPath.emplace_back(tok, ""); - reportError(errorPath, Severity::error, "returnDanglingLifetime", msg + " that will be invalid when returning.", CWE562, false); + reportError(errorPath, Severity::error, "returnDanglingLifetime", msg + " that will be invalid when returning.", CWE562, inconclusive); } void CheckAutoVariables::errorInvalidLifetime(const Token *tok, const ValueFlow::Value* val) { + const bool inconclusive = val ? val->isInconclusive() : false; ErrorPath errorPath = val ? val->errorPath : ErrorPath(); std::string msg = "Using " + lifetimeMessage(tok, val, errorPath); errorPath.emplace_back(tok, ""); - reportError(errorPath, Severity::error, "invalidLifetime", msg + " that is out of scope.", CWE562, false); + reportError(errorPath, Severity::error, "invalidLifetime", msg + " that is out of scope.", CWE562, inconclusive); } void CheckAutoVariables::errorDanglngLifetime(const Token *tok, const ValueFlow::Value *val) { + const bool inconclusive = val ? val->isInconclusive() : false; ErrorPath errorPath = val ? val->errorPath : ErrorPath(); std::string tokName = tok ? tok->expressionString() : "x"; std::string msg = "Non-local variable '" + tokName + "' will use " + lifetimeMessage(tok, val, errorPath); errorPath.emplace_back(tok, ""); - reportError(errorPath, Severity::error, "danglingLifetime", msg + ".", CWE562, false); + reportError(errorPath, Severity::error, "danglingLifetime", msg + ".", CWE562, inconclusive); } -void CheckAutoVariables::errorReturnReference(const Token *tok, ErrorPath errorPath) +void CheckAutoVariables::errorReturnReference(const Token* tok, ErrorPath errorPath, bool inconclusive) { errorPath.emplace_back(tok, ""); - reportError(errorPath, Severity::error, "returnReference", "Reference to local variable returned.", CWE562, false); + reportError( + errorPath, Severity::error, "returnReference", "Reference to local variable returned.", CWE562, inconclusive); } void CheckAutoVariables::errorDanglingReference(const Token *tok, const Variable *var, ErrorPath errorPath) diff --git a/lib/checkautovariables.h b/lib/checkautovariables.h index 32471260f..9bdb1900e 100644 --- a/lib/checkautovariables.h +++ b/lib/checkautovariables.h @@ -85,7 +85,7 @@ private: void errorReturnDanglingLifetime(const Token *tok, const ValueFlow::Value* val); void errorInvalidLifetime(const Token *tok, const ValueFlow::Value* val); void errorDanglngLifetime(const Token *tok, const ValueFlow::Value *val); - void errorReturnReference(const Token *tok, ErrorPath errorPath); + void errorReturnReference(const Token* tok, ErrorPath errorPath, bool inconclusive); void errorDanglingReference(const Token *tok, const Variable *var, ErrorPath errorPath); void errorReturnTempReference(const Token *tok); void errorInvalidDeallocation(const Token *tok, const ValueFlow::Value *val); @@ -99,7 +99,7 @@ private: c.errorAutoVariableAssignment(nullptr, false); c.errorReturnAddressToAutoVariable(nullptr); c.errorReturnPointerToLocalArray(nullptr); - c.errorReturnReference(nullptr, errorPath); + c.errorReturnReference(nullptr, errorPath, false); c.errorDanglingReference(nullptr, nullptr, errorPath); c.errorReturnTempReference(nullptr); c.errorInvalidDeallocation(nullptr, nullptr); diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index a587ad801..2689dc48a 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -881,13 +881,14 @@ void CheckStl::invalidContainer() 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); errorPath.emplace_back(tok, ""); - reportError(errorPath, Severity::error, "invalidContainer", msg + " that may be invalid.", CWE664, false); + reportError(errorPath, Severity::error, "invalidContainer", msg + " that may be invalid.", CWE664, inconclusive); } void CheckStl::invalidContainerReferenceError(const Token* tok, const Token* contTok, ErrorPath errorPath) diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 63fe4d965..dbdca2da5 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -5912,7 +5912,7 @@ ValueType::MatchResult ValueType::matchParameter(const ValueType *call, const Va if (call->type == ValueType::Type::VOID || func->type == ValueType::Type::VOID) return ValueType::MatchResult::FALLBACK1; if (call->pointer > 0 && func->pointer > 0) - return ValueType::MatchResult::NOMATCH; + return func->type == ValueType::UNKNOWN_TYPE ? ValueType::MatchResult::UNKNOWN : ValueType::MatchResult::NOMATCH; if (call->isIntegral() && func->isIntegral()) return call->type < func->type ? ValueType::MatchResult::FALLBACK1 : diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index b5c12d9bc..02d8e8c17 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2165,25 +2165,6 @@ static bool evalAssignment(ValueFlow::Value &lhsValue, const std::string &assign return true; } -static bool isAliasOf(const Token *tok, nonneg int varid) -{ - if (tok->varId() == varid) - return false; - if (tok->varId() == 0) - return false; - if (!astIsPointer(tok)) - return false; - for (const ValueFlow::Value &val : tok->values()) { - if (!val.isLocalLifetimeValue()) - continue; - if (val.lifetimeKind != ValueFlow::Value::LifetimeKind::Address) - continue; - if (val.tokvalue->varId() == varid) - return true; - } - return false; -} - // Check if its an alias of the variable or is being aliased to this variable static bool isAliasOf(const Variable * var, const Token *tok, nonneg int varid, const std::list& values) { @@ -2199,6 +2180,8 @@ static bool isAliasOf(const Variable * var, const Token *tok, nonneg int varid, for (const ValueFlow::Value &val : values) { if (!val.isNonValue()) continue; + if (val.isInconclusive()) + continue; if (val.isLifetimeValue() && !val.isLocalLifetimeValue()) continue; if (val.isLifetimeValue() && val.lifetimeKind != ValueFlow::Value::LifetimeKind::Address) @@ -2897,10 +2880,17 @@ static bool valueFlowForward(Token * const startToken, }); } if (inconclusive) { - for (ValueFlow::Value &v : values) { - if (v.indirect != i) - continue; - v.setInconclusive(); + if (settings->inconclusive) { + for (ValueFlow::Value &v : values) { + if (v.indirect != i) + continue; + v.setInconclusive(); + } + } else { + // If inconclusive flag not enable then remove the values + values.remove_if([&](const ValueFlow::Value &v) { + return v.indirect == i; + }); } } } @@ -2981,6 +2971,30 @@ static const Token *findSimpleReturn(const Function *f) return returnTok; } +static std::vector findReturns(const Function* f) +{ + std::vector result; + const Scope* scope = f->functionScope; + if (!scope) + return result; + for (const Token* tok = scope->bodyStart->next(); tok && tok != scope->bodyEnd; tok = tok->next()) { + if (tok->str() == "{" && tok->scope() && + (tok->scope()->type == Scope::eLambda || tok->scope()->type == Scope::eClass)) { + tok = tok->link(); + continue; + } + if (Token::simpleMatch(tok->astParent(), "return")) { + result.push_back(tok); + } + // Skip lambda functions since the scope may not be set correctly + const Token* lambdaEndToken = findLambdaEndToken(tok); + if (lambdaEndToken) { + tok = lambdaEndToken; + } + } + return result; +} + static int getArgumentPos(const Variable *var, const Function *f) { auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable &v) { @@ -3052,6 +3066,8 @@ ValueFlow::Value getLifetimeObjValue(const Token *tok) auto pred = [](const ValueFlow::Value &v) { if (!v.isLocalLifetimeValue()) return false; + if (v.isInconclusive()) + return false; if (!v.tokvalue->variable()) return false; return true; @@ -3066,69 +3082,67 @@ ValueFlow::Value getLifetimeObjValue(const Token *tok) return result; } -static const Token* getLifetimeToken(const Token* tok, - ValueFlow::Value::ErrorPath& errorPath, - bool* addressOf = nullptr, - int depth = 20) +std::vector getLifetimeTokens(const Token* tok, ValueFlow::Value::ErrorPath errorPath, int depth) { if (!tok) - return nullptr; + return std::vector {}; const Variable *var = tok->variable(); if (depth < 0) - return tok; + return {{tok, std::move(errorPath)}}; if (var && var->declarationId() == tok->varId()) { if (var->isReference() || var->isRValueReference()) { - if (addressOf) - *addressOf = true; if (!var->declEndToken()) - return tok; + return {{tok, true, std::move(errorPath)}}; if (var->isArgument()) { errorPath.emplace_back(var->declEndToken(), "Passed to reference."); - return tok; + return {{tok, true, std::move(errorPath)}}; } else if (Token::simpleMatch(var->declEndToken(), "=")) { errorPath.emplace_back(var->declEndToken(), "Assigned to reference."); const Token *vartok = var->declEndToken()->astOperand2(); if (vartok == tok) - return tok; + return {{tok, true, std::move(errorPath)}}; if (vartok) - return getLifetimeToken(vartok, errorPath, addressOf, depth - 1); + return getLifetimeTokens(vartok, std::move(errorPath), depth - 1); } else { - return nullptr; + return std::vector {}; } } } else if (Token::Match(tok->previous(), "%name% (")) { const Function *f = tok->previous()->function(); if (f) { if (!Function::returnsReference(f)) - return tok; - const Token* returnTok = findSimpleReturn(f); - if (!returnTok) - return tok; - if (returnTok == tok) - return tok; - const Token* argvarTok = getLifetimeToken(returnTok, errorPath, addressOf, depth - 1); - if (!argvarTok) - return tok; - const Variable* argvar = argvarTok->variable(); - if (!argvar) - return tok; - if (argvar->isArgument() && (argvar->isReference() || argvar->isRValueReference())) { - int n = getArgumentPos(argvar, f); - if (n < 0) - return nullptr; - const Token* argTok = getArguments(tok->previous()).at(n); - errorPath.emplace_back(returnTok, "Return reference."); - errorPath.emplace_back(tok->previous(), "Called function passing '" + argTok->str() + "'."); - return getLifetimeToken(argTok, errorPath, addressOf, depth - 1); + return {{tok, std::move(errorPath)}}; + std::vector result; + std::vector returns = findReturns(f); + for (const Token* returnTok : returns) { + if (returnTok == tok) + continue; + for (LifetimeToken& lt : getLifetimeTokens(returnTok, std::move(errorPath), depth - 1)) { + const Token* argvarTok = lt.token; + const Variable* argvar = argvarTok->variable(); + if (!argvar) + continue; + if (argvar->isArgument() && (argvar->isReference() || argvar->isRValueReference())) { + int n = getArgumentPos(argvar, f); + if (n < 0) + return std::vector {}; + const Token* argTok = getArguments(tok->previous()).at(n); + lt.errorPath.emplace_back(returnTok, "Return reference."); + lt.errorPath.emplace_back(tok->previous(), "Called function passing '" + argTok->str() + "'."); + std::vector arglts = LifetimeToken::setInconclusive( + getLifetimeTokens(argTok, std::move(lt.errorPath), depth - 1), returns.size() > 1); + result.insert(result.end(), arglts.begin(), arglts.end()); + } + } } + return result; } else if (Token::Match(tok->tokAt(-2), ". %name% (") && astIsContainer(tok->tokAt(-2)->astOperand1())) { const Library::Container* library = getLibraryContainer(tok->tokAt(-2)->astOperand1()); Library::Container::Yield y = library->getYield(tok->previous()->str()); if (y == Library::Container::Yield::AT_INDEX || y == Library::Container::Yield::ITEM) { errorPath.emplace_back(tok->previous(), "Accessing container."); - if (addressOf) - *addressOf = false; - return getLifetimeToken(tok->tokAt(-2)->astOperand1(), errorPath, nullptr, depth - 1); + return LifetimeToken::setAddressOf( + getLifetimeTokens(tok->tokAt(-2)->astOperand1(), std::move(errorPath), depth - 1), false); } } } else if (Token::Match(tok, ".|::|[")) { @@ -3143,7 +3157,7 @@ static const Token* getLifetimeToken(const Token* tok, } if (!vartok) - return tok; + return {{tok, std::move(errorPath)}}; const Variable *tokvar = vartok->variable(); if (!astIsContainer(vartok) && !(tokvar && tokvar->isArray()) && (Token::Match(vartok->astParent(), "[|*") || vartok->astParent()->originalName() == "->")) { @@ -3151,17 +3165,27 @@ static const Token* getLifetimeToken(const Token* tok, if (!v.isLocalLifetimeValue()) continue; errorPath.insert(errorPath.end(), v.errorPath.begin(), v.errorPath.end()); - return getLifetimeToken(v.tokvalue, errorPath, addressOf); + return getLifetimeTokens(v.tokvalue, std::move(errorPath)); } } else { - if (addressOf && astIsContainer(vartok) && Token::simpleMatch(vartok->astParent(), "[")) { - *addressOf = false; - addressOf = nullptr; - } - return getLifetimeToken(vartok, errorPath, addressOf); + return LifetimeToken::setAddressOf(getLifetimeTokens(vartok, std::move(errorPath)), + !(astIsContainer(vartok) && Token::simpleMatch(vartok->astParent(), "["))); } } - return tok; + return {{tok, std::move(errorPath)}}; +} + +static const Token* getLifetimeToken(const Token* tok, ValueFlow::Value::ErrorPath& errorPath, bool* addressOf = nullptr) +{ + std::vector lts = getLifetimeTokens(tok); + if (lts.size() != 1) + return nullptr; + if (lts.front().inconclusive) + return nullptr; + if (addressOf) + *addressOf = lts.front().addressOf; + errorPath.insert(errorPath.end(), lts.front().errorPath.begin(), lts.front().errorPath.end()); + return lts.front().token; } const Variable* getLifetimeVariable(const Token* tok, ValueFlow::Value::ErrorPath& errorPath, bool* addressOf) @@ -3360,15 +3384,17 @@ struct LifetimeStore { std::string message; ValueFlow::Value::LifetimeKind type; ErrorPath errorPath; + bool inconclusive; LifetimeStore() - : argtok(nullptr), message(), type(), errorPath() + : argtok(nullptr), message(), type(), errorPath(), inconclusive(false) {} LifetimeStore(const Token *argtok, const std::string &message, - ValueFlow::Value::LifetimeKind type = ValueFlow::Value::LifetimeKind::Object) - : argtok(argtok), message(message), type(type), errorPath() + ValueFlow::Value::LifetimeKind type = ValueFlow::Value::LifetimeKind::Object, + bool inconclusive = false) + : argtok(argtok), message(message), type(type), errorPath(), inconclusive(inconclusive) {} static LifetimeStore fromFunctionArg(const Function * f, Token *tok, const Variable *var, TokenList *tokenlist, ErrorLogger *errorLogger) { @@ -3396,27 +3422,34 @@ struct LifetimeStore { template void byRef(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const { + if (!settings->inconclusive && inconclusive) + return; if (!argtok) return; - ErrorPath er = errorPath; - const Token *lifeTok = getLifetimeToken(argtok, er); - if (!lifeTok) - return; - if (!pred(lifeTok)) - return; - er.emplace_back(argtok, message); + for (const LifetimeToken& lt : getLifetimeTokens(argtok)) { + if (!settings->inconclusive && lt.inconclusive) + continue; + ErrorPath er = errorPath; + er.insert(er.end(), lt.errorPath.begin(), lt.errorPath.end()); + if (!lt.token) + return; + if (!pred(lt.token)) + return; + er.emplace_back(argtok, message); - ValueFlow::Value value; - value.valueType = ValueFlow::Value::LIFETIME; - value.lifetimeScope = ValueFlow::Value::LifetimeScope::Local; - value.tokvalue = lifeTok; - value.errorPath = er; - value.lifetimeKind = type; - // Don't add the value a second time - if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end()) - return; - setTokenValue(tok, value, tokenlist->getSettings()); - valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + ValueFlow::Value value; + value.valueType = ValueFlow::Value::LIFETIME; + value.lifetimeScope = ValueFlow::Value::LifetimeScope::Local; + value.tokvalue = lt.token; + value.errorPath = std::move(er); + value.lifetimeKind = type; + value.setInconclusive(lt.inconclusive || inconclusive); + // Don't add the value a second time + if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end()) + return; + setTokenValue(tok, value, tokenlist->getSettings()); + valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + } } void byRef(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) const { @@ -3427,6 +3460,8 @@ struct LifetimeStore { template void byVal(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const { + if (!settings->inconclusive && inconclusive) + return; if (!argtok) return; if (argtok->values().empty()) { @@ -3440,6 +3475,7 @@ struct LifetimeStore { value.tokvalue = var->nameToken(); value.errorPath = er; value.lifetimeKind = type; + value.setInconclusive(inconclusive); // Don't add the value a second time if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end()) return; @@ -3451,26 +3487,31 @@ struct LifetimeStore { if (!v.isLifetimeValue()) continue; const Token *tok3 = v.tokvalue; - ErrorPath er = v.errorPath; - const Token *lifeTok = getLifetimeToken(tok3, er); - if (!lifeTok) - return; - if (!pred(lifeTok)) - return; - er.emplace_back(argtok, message); - er.insert(er.end(), errorPath.begin(), errorPath.end()); + for (const LifetimeToken& lt : getLifetimeTokens(tok3)) { + if (!settings->inconclusive && lt.inconclusive) + continue; + ErrorPath er = v.errorPath; + er.insert(er.end(), lt.errorPath.begin(), lt.errorPath.end()); + if (!lt.token) + return; + if (!pred(lt.token)) + return; + er.emplace_back(argtok, message); + er.insert(er.end(), errorPath.begin(), errorPath.end()); - ValueFlow::Value value; - value.valueType = ValueFlow::Value::LIFETIME; - value.lifetimeScope = v.lifetimeScope; - value.tokvalue = lifeTok; - value.errorPath = er; - value.lifetimeKind = type; - // Don't add the value a second time - if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end()) - continue; - setTokenValue(tok, value, tokenlist->getSettings()); - valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + ValueFlow::Value value; + value.valueType = ValueFlow::Value::LIFETIME; + value.lifetimeScope = v.lifetimeScope; + value.tokvalue = lt.token; + value.errorPath = std::move(er); + value.lifetimeKind = type; + value.setInconclusive(lt.inconclusive || v.isInconclusive() || inconclusive); + // Don't add the value a second time + if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end()) + continue; + setTokenValue(tok, value, tokenlist->getSettings()); + valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + } } } @@ -3482,6 +3523,8 @@ struct LifetimeStore { template void byDerefCopy(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const { + if (!settings->inconclusive && inconclusive) + return; if (!argtok) return; for (const ValueFlow::Value &v : argtok->values()) { @@ -3495,7 +3538,7 @@ struct LifetimeStore { continue; for (const Token *tok3 = tok; tok3 && tok3 != var->declEndToken(); tok3 = tok3->previous()) { if (tok3->varId() == var->declarationId()) { - LifetimeStore{tok3, message, type} .byVal(tok, tokenlist, errorLogger, settings, pred); + LifetimeStore{tok3, message, type, inconclusive} .byVal(tok, tokenlist, errorLogger, settings, pred); break; } } @@ -3541,30 +3584,35 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog const Function *f = tok->function(); if (Function::returnsReference(f)) return; - const Token *returnTok = findSimpleReturn(f); - if (!returnTok) - return; - const Variable *returnVar = returnTok->variable(); - if (returnVar && returnVar->isArgument() && (returnVar->isConst() || !isVariableChanged(returnVar, settings, tokenlist->isCPP()))) { - LifetimeStore ls = LifetimeStore::fromFunctionArg(f, tok, returnVar, tokenlist, errorLogger); - ls.byVal(tok->next(), tokenlist, errorLogger, settings); - } - for (const ValueFlow::Value &v : returnTok->values()) { - if (!v.isLifetimeValue()) + std::vector returns = findReturns(f); + const bool inconclusive = returns.size() > 1; + for (const Token* returnTok : returns) { + if (returnTok == tok) continue; - if (!v.tokvalue) - continue; - const Variable *var = v.tokvalue->variable(); - LifetimeStore ls = LifetimeStore::fromFunctionArg(f, tok, var, tokenlist, errorLogger); - if (!ls.argtok) - continue; - ls.errorPath = v.errorPath; - ls.errorPath.emplace_front(returnTok, "Return " + lifetimeType(returnTok, &v) + "."); - if (var->isReference() || var->isRValueReference()) { - ls.byRef(tok->next(), tokenlist, errorLogger, settings); - } else if (v.isArgumentLifetimeValue()) { + const Variable *returnVar = returnTok->variable(); + if (returnVar && returnVar->isArgument() && (returnVar->isConst() || !isVariableChanged(returnVar, settings, tokenlist->isCPP()))) { + LifetimeStore ls = LifetimeStore::fromFunctionArg(f, tok, returnVar, tokenlist, errorLogger); + ls.inconclusive = inconclusive; ls.byVal(tok->next(), tokenlist, errorLogger, settings); } + for (const ValueFlow::Value &v : returnTok->values()) { + if (!v.isLifetimeValue()) + continue; + if (!v.tokvalue) + continue; + const Variable *var = v.tokvalue->variable(); + LifetimeStore ls = LifetimeStore::fromFunctionArg(f, tok, var, tokenlist, errorLogger); + if (!ls.argtok) + continue; + ls.inconclusive = inconclusive; + ls.errorPath = v.errorPath; + ls.errorPath.emplace_front(returnTok, "Return " + lifetimeType(returnTok, &v) + "."); + if (var->isReference() || var->isRValueReference()) { + ls.byRef(tok->next(), tokenlist, errorLogger, settings); + } else if (v.isArgumentLifetimeValue()) { + ls.byVal(tok->next(), tokenlist, errorLogger, settings); + } + } } } } @@ -3719,23 +3767,24 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger } // address of else if (tok->isUnaryOp("&")) { - ErrorPath errorPath; - const Token *lifeTok = getLifetimeToken(tok->astOperand1(), errorPath); - if (!lifeTok) - continue; + for (const LifetimeToken& lt : getLifetimeTokens(tok->astOperand1())) { + if (!settings->inconclusive && lt.inconclusive) + continue; + ErrorPath errorPath = lt.errorPath; + errorPath.emplace_back(tok, "Address of variable taken here."); - errorPath.emplace_back(tok, "Address of variable taken here."); + ValueFlow::Value value; + value.valueType = ValueFlow::Value::ValueType::LIFETIME; + value.lifetimeScope = ValueFlow::Value::LifetimeScope::Local; + value.tokvalue = lt.token; + value.errorPath = std::move(errorPath); + if (astIsPointer(lt.token) || !Token::Match(lt.token->astParent(), ".|[")) + value.lifetimeKind = ValueFlow::Value::LifetimeKind::Address; + value.setInconclusive(lt.inconclusive); + setTokenValue(tok, value, tokenlist->getSettings()); - ValueFlow::Value value; - value.valueType = ValueFlow::Value::ValueType::LIFETIME; - value.lifetimeScope = ValueFlow::Value::LifetimeScope::Local; - value.tokvalue = lifeTok; - value.errorPath = errorPath; - if (astIsPointer(lifeTok) || !Token::Match(lifeTok->astParent(), ".|[")) - value.lifetimeKind = ValueFlow::Value::LifetimeKind::Address; - setTokenValue(tok, value, tokenlist->getSettings()); - - valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + } } // container lifetimes else if (astIsContainer(tok)) { diff --git a/lib/valueflow.h b/lib/valueflow.h index d9fc94dc1..f164ac4da 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -27,6 +27,7 @@ #include #include #include +#include class ErrorLogger; class Settings; @@ -259,6 +260,37 @@ namespace ValueFlow { std::string eitherTheConditionIsRedundant(const Token *condition); } +struct LifetimeToken { + const Token* token; + bool addressOf; + ValueFlow::Value::ErrorPath errorPath; + bool inconclusive; + + LifetimeToken() : token(nullptr), addressOf(false), errorPath(), inconclusive(false) {} + + LifetimeToken(const Token* token, ValueFlow::Value::ErrorPath errorPath) + : token(token), addressOf(false), errorPath(std::move(errorPath)), inconclusive(false) + {} + + LifetimeToken(const Token* token, bool addressOf, ValueFlow::Value::ErrorPath errorPath) + : token(token), addressOf(addressOf), errorPath(std::move(errorPath)), inconclusive(false) + {} + + static std::vector setAddressOf(std::vector v, bool b) { + for (LifetimeToken& x : v) + x.addressOf = b; + return v; + } + + static std::vector setInconclusive(std::vector v, bool b) { + for (LifetimeToken& x : v) + x.inconclusive = b; + return v; + } +}; + +std::vector getLifetimeTokens(const Token* tok, ValueFlow::Value::ErrorPath errorPath = ValueFlow::Value::ErrorPath{}, int depth = 20); + const Variable* getLifetimeVariable(const Token* tok, ValueFlow::Value::ErrorPath& errorPath, bool* addressOf = nullptr); bool isLifetimeBorrowed(const Token *tok, const Settings *settings); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 7748e52dd..ab38e6154 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -59,13 +59,6 @@ if (BUILD_TESTS) endif() endforeach() - # Set cost of the more expensive tests to help improve parallel scheduling - # of tests - fixture_cost(TestIO 20) - fixture_cost(TestThreadExecutor 5) - fixture_cost(TestLeakAutoVar 4) - fixture_cost(TestTokenizer 4) - function(add_cfg CFG_TEST) set(options INCONCLUSIVE) set(oneValueArgs PLATFORM NAME) @@ -118,10 +111,18 @@ if (BUILD_TESTS) add_cfg(python.c) add_cfg(qt.cpp) add_cfg(sqlite3.c INCONCLUSIVE) - add_cfg(std.c) - add_cfg(std.cpp) + add_cfg(std.c INCONCLUSIVE) + add_cfg(std.cpp INCONCLUSIVE) add_cfg(windows.cpp INCONCLUSIVE NAME windows32A PLATFORM win32A) add_cfg(windows.cpp INCONCLUSIVE NAME windows32W PLATFORM win32W) add_cfg(windows.cpp INCONCLUSIVE NAME windows64 PLATFORM win64) add_cfg(wxwidgets.cpp INCONCLUSIVE) + + # Set cost of the more expensive tests to help improve parallel scheduling + # of tests + fixture_cost(TestIO 20) + fixture_cost(cfg-std_c 8) + fixture_cost(TestThreadExecutor 5) + fixture_cost(TestLeakAutoVar 4) + fixture_cost(TestTokenizer 4) endif() diff --git a/test/cfg/runtests.sh b/test/cfg/runtests.sh index 3118a6f3f..a870090d1 100755 --- a/test/cfg/runtests.sh +++ b/test/cfg/runtests.sh @@ -63,9 +63,9 @@ ${CPPCHECK} ${CPPCHECK_OPT} --library=bsd ${DIR}bsd.c # std.c ${CC} ${CC_OPT} ${DIR}std.c -${CPPCHECK} ${CPPCHECK_OPT} ${DIR}std.c +${CPPCHECK} ${CPPCHECK_OPT} --inconclusive ${DIR}std.c ${CXX} ${CXX_OPT} ${DIR}std.cpp -${CPPCHECK} ${CPPCHECK_OPT} ${DIR}std.cpp +${CPPCHECK} ${CPPCHECK_OPT} --inconclusive ${DIR}std.cpp # windows.cpp # Syntax check via g++ does not work because it can not find a valid windows.h diff --git a/test/cfg/std.c b/test/cfg/std.c index 3dce3c204..6992358a2 100644 --- a/test/cfg/std.c +++ b/test/cfg/std.c @@ -47,6 +47,7 @@ void bufferAccessOutOfBounds(void) // cppcheck-suppress bufferAccessOutOfBounds strcpy_s(a, 10, "abcdefghij"); // TODO cppcheck-suppress redundantCopy + // cppcheck-suppress terminateStrncpy strncpy(a,"abcde",5); // cppcheck-suppress bufferAccessOutOfBounds // TODO cppcheck-suppress redundantCopy diff --git a/test/cfg/std.cpp b/test/cfg/std.cpp index d9cd7b261..409e6c9ef 100644 --- a/test/cfg/std.cpp +++ b/test/cfg/std.cpp @@ -78,6 +78,7 @@ void bufferAccessOutOfBounds(void) // TODO cppcheck-suppress redundantCopy std::strcpy(a, "abcde"); // TODO cppcheck-suppress redundantCopy + // cppcheck-suppress terminateStrncpy std::strncpy(a,"abcde",5); // cppcheck-suppress bufferAccessOutOfBounds // TODO cppcheck-suppress redundantCopy diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index c0d0e96e1..e41f7cd44 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -1228,6 +1228,21 @@ private: "}\n"); ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (error) Reference to local variable returned.\n", errout.str()); + check("template \n" + "const V& get_default(const T& t, const K& k, const V& v) {\n" + " auto it = t.find(k);\n" + " if (it == t.end()) return v;\n" + " return it->second;\n" + "}\n" + "const int& bar(const std::unordered_map& m, int k) {\n" + " auto x = 0;\n" + " return get_default(m, k, x);\n" + "}\n", + true); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:4] -> [test.cpp:9] -> [test.cpp:9]: (error, inconclusive) Reference to local variable returned.\n", + errout.str()); + check("struct A { int foo; };\n" "int& f(std::vector& v) {\n" " auto it = v.begin();\n" @@ -1666,6 +1681,21 @@ private: "[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'ba' that will be invalid when returning.\n", errout.str()); + check("template \n" + "const V* get_default(const T& t, const K& k, const V* v) {\n" + " auto it = t.find(k);\n" + " if (it == t.end()) return v;\n" + " return &it->second;\n" + "}\n" + "const int* bar(const std::unordered_map& m, int k) {\n" + " auto x = 0;\n" + " return get_default(m, k, &x);\n" + "}\n", + true); + ASSERT_EQUALS( + "[test.cpp:9] -> [test.cpp:9] -> [test.cpp:8] -> [test.cpp:9]: (error, inconclusive) Returning pointer to local variable 'x' that will be invalid when returning.\n", + errout.str()); + check("struct A {\n" " std::vector v;\n" " void f() {\n" diff --git a/test/testio.cpp b/test/testio.cpp index 33a7d7c6a..989e77759 100644 --- a/test/testio.cpp +++ b/test/testio.cpp @@ -4454,7 +4454,7 @@ private: " sprintf_s(lineBuffer, 100, format1, \"type\", \"sum\", \"avg\", \"min\", i, 0);\n" " sprintf_s(lineBuffer, 100, format2, \"type\", \"sum\", \"avg\", \"min\", i, 0);\n" " sprintf_s(lineBuffer, 100, format3, \"type\", \"sum\", \"avg\", \"min\", i, 0);\n" - "}\n", false, false, Settings::Win32A); + "}\n", true, false, Settings::Win32A); ASSERT_EQUALS("[test.cpp:6]: (warning) %s in format string (no. 5) requires 'char *' but the argument type is 'signed int'.\n" "[test.cpp:6]: (warning) sprintf_s format string requires 5 parameters but 6 are given.\n" "[test.cpp:7]: (warning) %s in format string (no. 5) requires 'char *' but the argument type is 'signed int'.\n" diff --git a/test/teststl.cpp b/test/teststl.cpp index eb007543f..99e861144 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -1188,8 +1188,8 @@ private: " iter = ints.begin() + 2;\n" " ints.erase(iter);\n" " std::cout << (*iter) << std::endl;\n" - "}"); - 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()); + "}", true); + TODO_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", "[test.cpp:5] -> [test.cpp:6] -> [test.cpp:3] -> [test.cpp:7]: (error, inconclusive) 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" @@ -1259,8 +1259,8 @@ private: " auto iter = ints.begin() + 2;\n" " ints.erase(iter);\n" " std::cout << (*iter) << std::endl;\n" - "}"); - 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()); + "}", true); + TODO_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", "[test.cpp:4] -> [test.cpp:5] -> [test.cpp:3] -> [test.cpp:6]: (error, inconclusive) Using iterator to local container 'ints' that may be invalid.\n", errout.str()); check("void f() {\n" " auto x = *myList.begin();\n" @@ -1766,8 +1766,8 @@ private: " iter = ints.begin() + 2;\n" " ints.erase(iter);\n" " ints.erase(iter);\n" - "}"); - ASSERT_EQUALS("[test.cpp:1] -> [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()); + "}", true); + TODO_ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:4] -> [test.cpp:5] -> [test.cpp:1] -> [test.cpp:6]: (error) Using iterator to local container 'ints' that may be invalid.\n", "[test.cpp:1] -> [test.cpp:4] -> [test.cpp:5] -> [test.cpp:1] -> [test.cpp:6]: (error, inconclusive) Using iterator to local container 'ints' that may be invalid.\n", errout.str()); } void eraseByValue() { @@ -2053,7 +2053,7 @@ private: " *it;\n" " }\n" "}"); - 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()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:4] -> [test.cpp:6] -> [test.cpp:3] -> [test.cpp:4]: (error) Using iterator to local container 'vec' that may be invalid.\n", errout.str()); } void pushback13() { @@ -2089,8 +2089,8 @@ private: " std::vector::iterator iter = ints.begin();\n" " ints.insert(iter, 1);\n" " ints.insert(iter, 2);\n" - "}"); - 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()); + "}", true); + TODO_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", "[test.cpp:4] -> [test.cpp:5] -> [test.cpp:3] -> [test.cpp:6]: (error, inconclusive) 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"