From d376e9f24524882c235b67165e222fdabea423a5 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 15 Nov 2018 23:12:28 -0600 Subject: [PATCH] Track variable lifetime through function calls (#1481) --- lib/valueflow.cpp | 256 ++++++++++++++++++++++++------------- test/testautovariables.cpp | 53 ++++++++ 2 files changed, 219 insertions(+), 90 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index ec5b89e49..74f9a2187 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2468,61 +2468,19 @@ static bool valueFlowForward(Token * const startToken, return true; } -static bool isNotLifetimeValue(const ValueFlow::Value& val) +static const Variable *getLifetimeVariable(const Token *tok, ErrorPath &errorPath) { - return !val.isLifetimeValue(); -} - -static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) -{ - const Token * assignTok = tok->astParent(); - // Assignment - if (!assignTok || (assignTok->str() != "=") || assignTok->astParent()) - return; - - // Lhs should be a variable - if (!assignTok->astOperand1() || !assignTok->astOperand1()->varId()) - return; - const Variable *var = assignTok->astOperand1()->variable(); - if (!var || (!var->isLocal() && !var->isGlobal() && !var->isArgument())) - return; - - const Token * const endOfVarScope = var->typeStartToken()->scope()->bodyEnd; - - // Rhs values.. - if (!assignTok->astOperand2() || assignTok->astOperand2()->values().empty()) - return; - - if (astIsPointer(assignTok->astOperand2()) && !var->isPointer() && !(var->valueType() && var->valueType()->isIntegral())) - return; - - std::list values = assignTok->astOperand2()->values(); - - // Static variable initialisation? - if (var->isStatic() && var->nameToken() == assignTok->astOperand1()) - changeKnownToPossible(values); - - // Skip RHS - const Token * nextExpression = nextAfterAstRightmostLeaf(assignTok); - - // Only forward lifetime values - values.remove_if(&isNotLifetimeValue); - valueFlowForward(const_cast(nextExpression), endOfVarScope, var, var->declarationId(), values, false, false, tokenlist, errorLogger, settings); -} - -static const Variable * getLifetimeVariable(const Token * tok, ErrorPath& errorPath) -{ - const Variable * var = tok->variable(); + const Variable *var = tok->variable(); if (!var) return nullptr; if (var->isReference() || var->isRValueReference()) { - for (const ValueFlow::Value& v:tok->values()) { + for (const ValueFlow::Value &v : tok->values()) { if (!v.isLifetimeValue()) continue; if (v.tokvalue == tok) continue; errorPath.insert(errorPath.end(), v.errorPath.begin(), v.errorPath.end()); - const Variable * var2 = getLifetimeVariable(v.tokvalue, errorPath); + const Variable *var2 = getLifetimeVariable(v.tokvalue, errorPath); if (var2) return var2; } @@ -2531,6 +2489,150 @@ static const Variable * getLifetimeVariable(const Token * tok, ErrorPath& errorP return var; } +static bool isNotLifetimeValue(const ValueFlow::Value& val) +{ + return !val.isLifetimeValue(); +} + +static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings); + +static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) +{ + const Token *parent = tok->astParent(); + while (parent && parent->isArithmeticalOp()) + parent = parent->astParent(); + if (!parent) + return; + // Assignment + if (parent->str() == "=" && !parent->astParent()) { + // Lhs should be a variable + if (!parent->astOperand1() || !parent->astOperand1()->varId()) + return; + const Variable *var = parent->astOperand1()->variable(); + if (!var || (!var->isLocal() && !var->isGlobal() && !var->isArgument())) + return; + + const Token *const endOfVarScope = var->typeStartToken()->scope()->bodyEnd; + + // Rhs values.. + if (!parent->astOperand2() || parent->astOperand2()->values().empty()) + return; + + if (astIsPointer(parent->astOperand2()) && !var->isPointer() && + !(var->valueType() && var->valueType()->isIntegral())) + return; + + std::list values = parent->astOperand2()->values(); + + // Static variable initialisation? + if (var->isStatic() && var->nameToken() == parent->astOperand1()) + changeKnownToPossible(values); + + // Skip RHS + const Token *nextExpression = nextAfterAstRightmostLeaf(parent); + + // Only forward lifetime values + values.remove_if(&isNotLifetimeValue); + valueFlowForward(const_cast(nextExpression), + endOfVarScope, + var, + var->declarationId(), + values, + false, + false, + tokenlist, + errorLogger, + settings); + // Function call + } else if (Token::Match(parent->previous(), "%name% (")) { + valueFlowLifetimeFunction(const_cast(parent->previous()), tokenlist, errorLogger, settings); + } +} + +struct LifetimeStore { + const Token *argtok; + std::string message; + ValueFlow::Value::LifetimeKind type; + + template + void byRef(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const { + ErrorPath errorPath; + const Variable *var = getLifetimeVariable(argtok, errorPath); + if (!var) + return; + if (!pred(var)) + return; + errorPath.emplace_back(argtok, message); + + ValueFlow::Value value; + value.valueType = ValueFlow::Value::LIFETIME; + value.tokvalue = var->nameToken(); + value.errorPath = errorPath; + value.lifetimeKind = type; + // Dont 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 { + byRef(tok, tokenlist, errorLogger, settings, [](const Variable *) { + return true; + }); + } + + template + void byVal(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const { + for (const ValueFlow::Value &v : argtok->values()) { + if (!v.isLifetimeValue()) + continue; + const Token *tok3 = v.tokvalue; + ErrorPath errorPath = v.errorPath; + const Variable *var = getLifetimeVariable(tok3, errorPath); + if (!var) + continue; + if (!pred(var)) + return; + errorPath.emplace_back(argtok, message); + + ValueFlow::Value value; + value.valueType = ValueFlow::Value::LIFETIME; + value.tokvalue = var->nameToken(); + value.errorPath = errorPath; + value.lifetimeKind = type; + // Dont 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); + } + } + + void byVal(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) const { + byVal(tok, tokenlist, errorLogger, settings, [](const Variable *) { + return true; + }); + } +}; + +static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) +{ + if (!Token::Match(tok, "%name% (")) + return; + if (Token::Match(tok->tokAt(-2), "std :: ref|cref|tie|front_inserter|back_inserter")) { + for (const Token *argtok : getArguments(tok)) { + LifetimeStore{argtok, "Passed to '" + tok->str() + "'.", ValueFlow::Value::Object} .byRef( + tok->next(), tokenlist, errorLogger, settings); + } + } else if (Token::Match(tok->tokAt(-2), "std :: make_tuple|tuple_cat|make_pair|make_reverse_iterator|next|prev|move")) { + for (const Token *argtok : getArguments(tok)) { + LifetimeStore{argtok, "Passed to '" + tok->str() + "'.", ValueFlow::Value::Object} .byVal( + tok->next(), tokenlist, errorLogger, settings); + } + } +} + struct Lambda { explicit Lambda(const Token * tok) : capture(nullptr), arguments(nullptr), returnTok(nullptr), bodyTok(nullptr) { @@ -2570,6 +2672,16 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger std::set scopes; + auto isCapturingVariable = [&](const Variable *var) { + const Scope *scope = var->scope(); + if (scopes.count(scope) > 0) + return false; + if (scope->isNestedIn(bodyScope)) + return false; + scopes.insert(scope); + return true; + }; + // TODO: Handle explicit capture bool captureByRef = Token::Match(lam.capture, "[ & ]"); bool captureByValue = Token::Match(lam.capture, "[ = ]"); @@ -2577,51 +2689,11 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger for (const Token * tok2 = lam.bodyTok; tok2 != lam.bodyTok->link(); tok2 = tok2->next()) { ErrorPath errorPath; if (captureByRef) { - const Variable * var = getLifetimeVariable(tok2, errorPath); - if (!var) - continue; - const Scope * scope = var->scope(); - if (scopes.count(scope) > 0) - continue; - if (scope->isNestedIn(bodyScope)) - continue; - scopes.insert(scope); - errorPath.emplace_back(tok2, "Lambda captures variable by reference here."); - - ValueFlow::Value value; - value.valueType = ValueFlow::Value::LIFETIME; - value.tokvalue = var->nameToken(); - value.errorPath = errorPath; - value.lifetimeKind = ValueFlow::Value::Lambda; - setTokenValue(tok, value, tokenlist->getSettings()); - - valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + LifetimeStore{tok2, "Lambda captures variable by reference here.", ValueFlow::Value::Lambda} .byRef( + tok, tokenlist, errorLogger, settings, isCapturingVariable); } else if (captureByValue) { - for (const ValueFlow::Value& v:tok2->values()) { - if (!v.isLifetimeValue() && !v.tokvalue) - continue; - const Token * tok3 = v.tokvalue; - errorPath = v.errorPath; - const Variable * var = getLifetimeVariable(tok3, errorPath); - if (!var) - continue; - const Scope * scope = var->scope(); - if (scopes.count(scope) > 0) - continue; - if (scope->isNestedIn(bodyScope)) - continue; - scopes.insert(scope); - errorPath.emplace_back(tok2, "Lambda captures variable by value here."); - - ValueFlow::Value value; - value.valueType = ValueFlow::Value::LIFETIME; - value.tokvalue = var->nameToken(); - value.errorPath = errorPath; - value.lifetimeKind = ValueFlow::Value::Lambda; - setTokenValue(tok, value, tokenlist->getSettings()); - - valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); - } + LifetimeStore{tok2, "Lambda captures variable by value here.", ValueFlow::Value::Lambda} .byVal( + tok, tokenlist, errorLogger, settings, isCapturingVariable); } } } @@ -2681,6 +2753,10 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger valueFlowForwardLifetime(tok->tokAt(3), tokenlist, errorLogger, settings); } + // Check function calls + else if (Token::Match(tok, "%name% (")) { + valueFlowLifetimeFunction(tok, tokenlist, errorLogger, settings); + } // Check variables else if (tok->variable()) { ErrorPath errorPath; diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index 0334cc4f9..835adecb6 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -123,6 +123,7 @@ private: TEST_CASE(danglingLifetimeLambda); TEST_CASE(danglingLifetimeContainer); TEST_CASE(danglingLifetime); + TEST_CASE(danglingLifetimeFunction); TEST_CASE(invalidLifetime); } @@ -1319,6 +1320,33 @@ private: "}\n"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:1] -> [test.cpp:3]: (error) Returning iterator to local container 'x' that will be invalid when returning.\n", errout.str()); + check("auto f() {\n" + " std::vector x;\n" + " auto it = x.begin();\n" + " return std::next(it);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:4] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'x' that will be invalid when returning.\n", + errout.str()); + + check("auto f() {\n" + " std::vector x;\n" + " auto it = x.begin();\n" + " return it + 1;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning iterator to local container 'x' that will be invalid when returning.\n", + errout.str()); + + check("auto f() {\n" + " std::vector x;\n" + " auto it = x.begin();\n" + " return std::next(it + 1);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:4] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'x' that will be invalid when returning.\n", + errout.str()); + check("auto f() {\n" " static std::vector x;\n" " return x.begin();\n" @@ -1383,6 +1411,31 @@ private: ASSERT_EQUALS("", errout.str()); } + void danglingLifetimeFunction() { + check("auto f() {\n" + " int a;\n" + " return std::ref(a);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning object that points to local variable 'a' that will be invalid when returning.\n", + errout.str()); + + check("auto f() {\n" + " int a;\n" + " return std::make_tuple(std::ref(a));\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning object that points to local variable 'a' that will be invalid when returning.\n", + errout.str()); + + check("auto f(int x) {\n" + " int a;\n" + " std::tie(a) = x;\n" + " return a;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void invalidLifetime() { check("void foo(int a) {\n" " std::function f;\n"