diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index 4b435d73f..286dcc3d9 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -603,6 +603,7 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token return; bool returnRef = Function::returnsReference(scope->function); for (const Token *tok = start; tok && tok != end; tok = tok->next()) { + // Return reference form function if (returnRef && Token::simpleMatch(tok->astParent(), "return")) { ErrorPath errorPath; const Variable *var = getLifetimeVariable(tok, errorPath); @@ -611,6 +612,7 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token errorReturnReference(tok, errorPath); continue; } + // Assign reference to non-local variable } else if (Token::Match(tok->astParent(), "&|&&") && Token::simpleMatch(tok->astParent()->astParent(), "=") && tok->variable() && tok->variable()->declarationId() == tok->varId() && tok->variable()->isStatic() && !tok->variable()->isArgument()) { @@ -624,8 +626,7 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token for (const ValueFlow::Value& val:tok->values()) { if (!val.isLocalLifetimeValue()) continue; - // Skip temporaries for now - if (val.tokvalue == tok) + if (!val.tokvalue->variable()) continue; if (Token::Match(tok->astParent(), "return|throw")) { if (getPointerDepth(tok) < getPointerDepth(val.tokvalue)) @@ -633,11 +634,11 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token if (tok->astParent()->str() == "return" && !astIsContainer(tok) && scope->function && mSettings->library.detectContainer(scope->function->retDef)) continue; - if (isInScope(val.tokvalue, scope)) { + if (isInScope(val.tokvalue->variable()->nameToken(), scope)) { errorReturnDanglingLifetime(tok, &val); break; } - } else if (isDeadScope(val.tokvalue, tok->scope())) { + } else if (isDeadScope(val.tokvalue->variable()->nameToken(), tok->scope())) { errorInvalidLifetime(tok, &val); break; } else if (tok->variable() && tok->variable()->declarationId() == tok->varId() && @@ -679,7 +680,9 @@ void CheckAutoVariables::checkVarLifetime() static std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, ErrorPath &errorPath) { - const Token *vartok = val ? val->tokvalue : nullptr; + 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) { diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index e43f73fff..4be32e994 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2662,27 +2662,27 @@ std::string lifetimeType(const Token *tok, const ValueFlow::Value *val) return result; } -const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath, int depth) +const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPath &errorPath, int depth = 20) { if (!tok) return nullptr; const Variable *var = tok->variable(); if (depth < 0) - return var; + return tok; if (var && var->declarationId() == tok->varId()) { if (var->isReference() || var->isRValueReference()) { if (!var->declEndToken()) - return nullptr; + return tok; if (var->isArgument()) { errorPath.emplace_back(var->declEndToken(), "Passed to reference."); - return var; + return var->nameToken(); } else if (Token::simpleMatch(var->declEndToken(), "=")) { errorPath.emplace_back(var->declEndToken(), "Assigned to reference."); const Token *vartok = var->declEndToken()->astOperand2(); if (vartok == tok) - return nullptr; + return tok; if (vartok) - return getLifetimeVariable(vartok, errorPath, depth-1); + return getLifetimeToken(vartok, errorPath, depth - 1); } else { return nullptr; } @@ -2690,17 +2690,20 @@ const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPat } else if (Token::Match(tok->previous(), "%name% (")) { const Function *f = tok->previous()->function(); if (!f) - return nullptr; + return tok; if (!Function::returnsReference(f)) - return nullptr; + return tok; const Token *returnTok = findSimpleReturn(f); if (!returnTok) - return nullptr; + return tok; if (returnTok == tok) - return var; - const Variable *argvar = getLifetimeVariable(returnTok, errorPath, depth-1); + return tok; + const Token *argvarTok = getLifetimeToken(returnTok, errorPath, depth - 1); + if (!argvarTok) + return tok; + const Variable *argvar = argvarTok->variable(); if (!argvar) - return nullptr; + return tok; if (argvar->isArgument() && (argvar->isReference() || argvar->isRValueReference())) { int n = getArgumentPos(argvar, f); if (n < 0) @@ -2708,10 +2711,43 @@ const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPat 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 getLifetimeVariable(argTok, errorPath, depth-1); + return getLifetimeToken(argTok, errorPath, depth - 1); + } + } else if (Token::Match(tok, ".|::|[")) { + const Token *vartok = tok; + while (vartok) { + if (vartok->str() == "[" || vartok->originalName() == "->") + vartok = vartok->astOperand1(); + else if (vartok->str() == "." || vartok->str() == "::") + vartok = vartok->astOperand2(); + else + break; + } + + if (!vartok) + return tok; + const Variable *tokvar = vartok->variable(); + if (!astIsContainer(vartok) && !(tokvar && tokvar->isArray()) && Token::Match(vartok->astParent(), "[|*|.") && + vartok->astParent()->originalName() != ".") { + for (const ValueFlow::Value &v : vartok->values()) { + if (!v.isLocalLifetimeValue()) + continue; + errorPath.insert(errorPath.end(), v.errorPath.begin(), v.errorPath.end()); + return getLifetimeToken(v.tokvalue, errorPath); + } + } else { + return getLifetimeToken(vartok, errorPath); } } - return var; + return tok; +} + +const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath) +{ + const Token *tok2 = getLifetimeToken(tok, errorPath); + if (tok2 && tok2->variable()) + return tok2->variable(); + return nullptr; } static bool isNotLifetimeValue(const ValueFlow::Value& val) @@ -2825,17 +2861,17 @@ struct LifetimeStore { template void byRef(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const { ErrorPath er = errorPath; - const Variable *var = getLifetimeVariable(argtok, er); - if (!var) + const Token *lifeTok = getLifetimeToken(argtok, er); + if (!lifeTok) return; - if (!pred(var)) + if (!pred(lifeTok)) return; er.emplace_back(argtok, message); ValueFlow::Value value; value.valueType = ValueFlow::Value::LIFETIME; value.lifetimeScope = ValueFlow::Value::Local; - value.tokvalue = var->nameToken(); + value.tokvalue = lifeTok; value.errorPath = er; value.lifetimeKind = type; // Don't add the value a second time @@ -2846,7 +2882,7 @@ struct LifetimeStore { } void byRef(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) const { - byRef(tok, tokenlist, errorLogger, settings, [](const Variable *) { + byRef(tok, tokenlist, errorLogger, settings, [](const Token *) { return true; }); } @@ -2876,10 +2912,10 @@ struct LifetimeStore { continue; const Token *tok3 = v.tokvalue; ErrorPath er = v.errorPath; - const Variable *var = getLifetimeVariable(tok3, er); - if (!var) - continue; - if (!pred(var)) + 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()); @@ -2887,7 +2923,7 @@ struct LifetimeStore { ValueFlow::Value value; value.valueType = ValueFlow::Value::LIFETIME; value.lifetimeScope = v.lifetimeScope; - value.tokvalue = var->nameToken(); + value.tokvalue = lifeTok; value.errorPath = er; value.lifetimeKind = type; // Don't add the value a second time @@ -2899,7 +2935,7 @@ struct LifetimeStore { } void byVal(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) const { - byVal(tok, tokenlist, errorLogger, settings, [](const Variable *) { + byVal(tok, tokenlist, errorLogger, settings, [](const Token *) { return true; }); } @@ -2925,7 +2961,7 @@ struct LifetimeStore { } void byDerefCopy(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) const { - byDerefCopy(tok, tokenlist, errorLogger, settings, [](const Variable *) { + byDerefCopy(tok, tokenlist, errorLogger, settings, [](const Token *) { return true; }); } @@ -3083,7 +3119,10 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger std::set scopes; - auto isCapturingVariable = [&](const Variable *var) { + auto isCapturingVariable = [&](const Token *varTok) { + const Variable *var = varTok->variable(); + if (!var) + return false; const Scope *scope = var->scope(); if (!scope) return false; @@ -3113,23 +3152,8 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger // address of else if (tok->isUnaryOp("&")) { ErrorPath errorPath; - // child should be some buffer or variable - const Token *vartok = tok->astOperand1(); - while (vartok) { - if (vartok->str() == "[") - vartok = vartok->astOperand1(); - else if (vartok->str() == "." || vartok->str() == "::") - vartok = vartok->astOperand2(); - else - break; - } - - if (!vartok) - continue; - const Variable * var = getLifetimeVariable(vartok, errorPath); - if (!var) - continue; - if (var->isPointer() && Token::Match(vartok->astParent(), "[|*")) + const Token *lifeTok = getLifetimeToken(tok->astOperand1(), errorPath); + if (!lifeTok) continue; errorPath.emplace_back(tok, "Address of variable taken here."); @@ -3137,7 +3161,7 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger ValueFlow::Value value; value.valueType = ValueFlow::Value::LIFETIME; value.lifetimeScope = ValueFlow::Value::Local; - value.tokvalue = var->nameToken(); + value.tokvalue = lifeTok; value.errorPath = errorPath; setTokenValue(tok, value, tokenlist->getSettings()); @@ -3149,7 +3173,6 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger const Library::Container * container = settings->library.detectContainer(tok->variable()->typeStartToken()); if (!container) continue; - const Variable * var = tok->variable(); bool isIterator = !Token::Match(tok->tokAt(2), "data|c_str"); if (isIterator) @@ -3160,7 +3183,7 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger ValueFlow::Value value; value.valueType = ValueFlow::Value::LIFETIME; value.lifetimeScope = ValueFlow::Value::Local; - value.tokvalue = var->nameToken(); + value.tokvalue = tok; value.errorPath = errorPath; value.lifetimeKind = isIterator ? ValueFlow::Value::Iterator : ValueFlow::Value::Object; setTokenValue(tok->tokAt(3), value, tokenlist->getSettings()); diff --git a/lib/valueflow.h b/lib/valueflow.h index 89a6b6bf6..9b8caf0fa 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -229,7 +229,7 @@ namespace ValueFlow { std::string eitherTheConditionIsRedundant(const Token *condition); } -const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath, int depth=20); +const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath); std::string lifetimeType(const Token *tok, const ValueFlow::Value *val); diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index 449151d84..b187cf98a 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -108,6 +108,7 @@ private: TEST_CASE(returnReference6); TEST_CASE(returnReference7); TEST_CASE(returnReferenceFunction); + TEST_CASE(returnReferenceContainer); TEST_CASE(returnReferenceLiteral); TEST_CASE(returnReferenceCalculation); TEST_CASE(returnReferenceLambda); @@ -1108,6 +1109,17 @@ private: "[test.cpp:1] -> [test.cpp:2] -> [test.cpp:6] -> [test.cpp:6]: (error) Reference to local variable returned.\n", errout.str()); + check("int& f(int& a) {\n" + " return a;\n" + "}\n" + "int* hello() {\n" + " int x = 0;\n" + " return &f(x);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:1] -> [test.cpp:2] -> [test.cpp:6] -> [test.cpp:6] -> [test.cpp:5] -> [test.cpp:6]: (error) Returning pointer to local variable 'x' that will be invalid when returning.\n", + errout.str()); + check("int f(int& a) {\n" " return a;\n" "}\n" @@ -1143,6 +1155,28 @@ private: ASSERT_EQUALS("", errout.str()); } + void returnReferenceContainer() { + check("auto& f() {\n" + " std::vector x;\n" + " return x[0];\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) 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" + " return it->foo;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (error) 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" + " return it->foo;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void returnReferenceLiteral() { check("const std::string &a() {\n" " return \"foo\";\n" @@ -1447,6 +1481,24 @@ private: "}\n"); ASSERT_EQUALS("[test.cpp:3] -> [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 p = &x[0];\n" + " return p;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning pointer to local variable 'x' that will be invalid when returning.\n", + errout.str()); + + check("struct A { int foo; };\n" + "int* f(std::vector v) {\n" + " auto it = v.begin();\n" + " return &it->foo;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:4] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'v' that will be invalid when returning.\n", + errout.str()); + check("auto f(std::vector x) {\n" " auto it = x.begin();\n" " return it;\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index b4aefe613..09f4970b1 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -115,6 +115,10 @@ private: TEST_CASE(valueFlowContainerSize); } + static bool isNotTokValue(const ValueFlow::Value &val) { + return !val.isTokValue(); + } + bool testValueOfXKnown(const char code[], unsigned int linenr, int value) { // Tokenize.. Tokenizer tokenizer(&settings, this); @@ -321,6 +325,7 @@ private: void valueFlowPointerAlias() { const char *code; + std::list values; code = "const char * f() {\n" " static const char *x;\n" @@ -342,8 +347,13 @@ private: " struct X *x;\n" " x = &x[1];\n" "}"; - ASSERT_EQUALS(true, tokenValues(code, "&").empty()); - ASSERT_EQUALS(true, tokenValues(code, "x [").empty()); + values = tokenValues(code, "&"); + values.remove_if(&isNotTokValue); + ASSERT_EQUALS(true, values.empty()); + + values = tokenValues(code, "x ["); + values.remove_if(&isNotTokValue); + ASSERT_EQUALS(true, values.empty()); } void valueFlowLifetime() { @@ -356,7 +366,7 @@ private: " auto x = [&]() { return a + 1; };\n" " auto b = x;\n" "}\n"; - ASSERT_EQUALS(true, testValueOfX(code, 4, "a ;", ValueFlow::Value::LIFETIME)); + ASSERT_EQUALS(true, testValueOfX(code, 4, "a + 1", ValueFlow::Value::LIFETIME)); code = "void f() {\n" " int a = 1;\n" @@ -378,7 +388,7 @@ private: " auto x = v.begin();\n" " auto it = x;\n" "}\n"; - ASSERT_EQUALS(true, testValueOfX(code, 4, "v ;", ValueFlow::Value::LIFETIME)); + ASSERT_EQUALS(true, testValueOfX(code, 4, "v . begin", ValueFlow::Value::LIFETIME)); } void valueFlowArrayElement() {