diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index 45aa69646..c99d94232 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -527,31 +527,8 @@ void CheckAutoVariables::returnReference() if (tok2->str() != "return") continue; - // return.. - if (Token::Match(tok2, "return %var% ;")) { - // is the returned variable a local variable? - if (isAutoVar(tok2->next())) { - const Variable *var1 = tok2->next()->variable(); - // If reference variable is used, check what it references - if (Token::Match(var1->nameToken(), "%var% [=(]")) { - const Token *tok3 = var1->nameToken()->tokAt(2); - if (!Token::Match(tok3, "%var% [);.]")) - continue; - - // Only report error if variable that is referenced is - // a auto variable - if (!isAutoVar(tok3)) - continue; - } - - // report error.. - errorReturnReference(tok2); - } - } - // return reference to temporary.. - else if (Token::Match(tok2, "return %name% (") && - Token::simpleMatch(tok2->linkAt(2), ") ;")) { + if (Token::Match(tok2, "return %name% (") && Token::simpleMatch(tok2->linkAt(2), ") ;")) { if (returnTemporary(tok2->next())) { // report error.. errorReturnTempReference(tok2); @@ -559,7 +536,8 @@ void CheckAutoVariables::returnReference() } // Return reference to a literal or the result of a calculation - else if (tok2->astOperand1() && (tok2->astOperand1()->isCalculation() || tok2->next()->isLiteral()) && astHasAutoResult(tok2->astOperand1())) { + else if (tok2->astOperand1() && (tok2->astOperand1()->isCalculation() || tok2->next()->isLiteral()) && + astHasAutoResult(tok2->astOperand1())) { errorReturnTempReference(tok2); } } @@ -623,7 +601,25 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token // If the scope is not set correctly then skip checking it if (scope->bodyStart != start) return; + bool returnRef = Function::returnsReference(scope->function); for (const Token *tok = start; tok && tok != end; tok = tok->next()) { + if (returnRef && Token::simpleMatch(tok->astParent(), "return")) { + ErrorPath errorPath; + const Variable *var = getLifetimeVariable(tok, errorPath); + if (var && var->isLocal() && !var->isArgument() && isInScope(var->nameToken(), tok->scope())) { + errorReturnReference(tok, errorPath); + continue; + } + } else if (Token::Match(tok->astParent(), "&|&&") && Token::simpleMatch(tok->astParent()->astParent(), "=") && + tok->variable() && tok->variable()->declarationId() == tok->varId() && tok->variable()->isStatic() && + !tok->variable()->isArgument()) { + ErrorPath errorPath; + const Variable *var = getLifetimeVariable(tok, errorPath); + if (var && isInScope(var->nameToken(), tok->scope())) { + errorDanglingReference(tok, var, errorPath); + continue; + } + } for (const ValueFlow::Value& val:tok->values()) { if (!val.isLifetimeValue()) continue; @@ -756,9 +752,19 @@ void CheckAutoVariables::errorDanglngLifetime(const Token *tok, const ValueFlow: reportError(errorPath, Severity::error, "danglingLifetime", msg + ".", CWE562, false); } -void CheckAutoVariables::errorReturnReference(const Token *tok) +void CheckAutoVariables::errorReturnReference(const Token *tok, ErrorPath errorPath) { - reportError(tok, Severity::error, "returnReference", "Reference to auto variable returned.", CWE562, false); + errorPath.emplace_back(tok, ""); + reportError(errorPath, Severity::error, "returnReference", "Reference to local variable returned.", CWE562, false); +} + +void CheckAutoVariables::errorDanglingReference(const Token *tok, const Variable *var, ErrorPath errorPath) +{ + std::string tokName = tok ? tok->str() : "x"; + std::string varName = var ? var->name() : "y"; + std::string msg = "Non-local reference variable '" + tokName + "' to local variable '" + varName + "'"; + errorPath.emplace_back(tok, ""); + reportError(errorPath, Severity::error, "danglingReference", msg, CWE562, false); } void CheckAutoVariables::errorReturnTempReference(const Token *tok) diff --git a/lib/checkautovariables.h b/lib/checkautovariables.h index ce98bb95d..04913a164 100644 --- a/lib/checkautovariables.h +++ b/lib/checkautovariables.h @@ -91,7 +91,8 @@ 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); + void errorReturnReference(const Token *tok, ErrorPath errorPath); + void errorDanglingReference(const Token *tok, const Variable *var, ErrorPath errorPath); void errorReturnTempReference(const Token *tok); void errorInvalidDeallocation(const Token *tok, const ValueFlow::Value *val); void errorReturnAddressOfFunctionParameter(const Token *tok, const std::string &varname); @@ -99,12 +100,14 @@ private: void errorUselessAssignmentPtrArg(const Token *tok); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE { + ErrorPath errorPath; CheckAutoVariables c(nullptr,settings,errorLogger); c.errorAutoVariableAssignment(nullptr, false); c.errorReturnAddressToAutoVariable(nullptr); c.errorAssignAddressOfLocalArrayToGlobalPointer(nullptr, nullptr); c.errorReturnPointerToLocalArray(nullptr); - c.errorReturnReference(nullptr); + c.errorReturnReference(nullptr, errorPath); + c.errorDanglingReference(nullptr, nullptr, errorPath); c.errorReturnTempReference(nullptr); c.errorInvalidDeallocation(nullptr, nullptr); c.errorReturnAddressOfFunctionParameter(nullptr, "parameter"); diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 24ae0428c..53aee0707 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -2049,6 +2049,15 @@ bool Function::argsMatch(const Scope *scope, const Token *first, const Token *se return false; } +bool Function::returnsReference(const Function *function) +{ + if (!function) + return false; + if (function->type != Function::eFunction) + return false; + return function->tokenDef->strAt(-1) == "&"; +} + const Token * Function::constructorMemberInitialization() const { if (!isConstructor() || !functionScope || !functionScope->bodyStart) diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 70fed24d0..dcb4dd01c 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -845,6 +845,8 @@ public: static bool argsMatch(const Scope *scope, const Token *first, const Token *second, const std::string &path, unsigned int path_length); + static bool returnsReference(const Function *function); + /** * @return token to ":" if the function is a constructor * and it contains member initialization otherwise a nullptr is returned diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index e94c7bb7a..ac0220e15 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2603,23 +2603,21 @@ static bool valueFlowForward(Token * const startToken, return true; } -static const Variable *getLifetimeVariable(const Token *tok, ErrorPath &errorPath) +const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath) { + if (!tok) + return nullptr; const Variable *var = tok->variable(); if (!var) return nullptr; if (var->isReference() || var->isRValueReference()) { - 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); - if (var2) - return var2; - } - return nullptr; + if (!var->declEndToken()) + return nullptr; + errorPath.emplace_back(var->declEndToken(), "Assigned to reference."); + const Token *vartok = var->declEndToken()->astOperand2(); + if (vartok == tok) + return nullptr; + return getLifetimeVariable(vartok, errorPath); } return var; } diff --git a/lib/valueflow.h b/lib/valueflow.h index 083377a7c..c329bb9eb 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -32,6 +32,7 @@ class Settings; class SymbolDatabase; class Token; class TokenList; +class Variable; namespace ValueFlow { class CPPCHECKLIB Value { @@ -204,4 +205,6 @@ namespace ValueFlow { std::string eitherTheConditionIsRedundant(const Token *condition); } +const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath); + #endif // valueflowH diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index aa2389d9c..c00bff926 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -112,6 +112,8 @@ private: TEST_CASE(returnReferenceLambda); TEST_CASE(returnReferenceInnerScope); + TEST_CASE(danglingReference); + // global namespace TEST_CASE(testglobalnamespace); @@ -837,19 +839,27 @@ private: } void returnReference1() { + check("int &foo()\n" + "{\n" + " int s = 0;\n" + " int& x = s;\n" + " return x;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5]: (error) Reference to local variable returned.\n", errout.str()); + check("std::string &foo()\n" "{\n" " std::string s;\n" " return s;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Reference to auto variable returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Reference to local variable returned.\n", errout.str()); check("std::vector &foo()\n" "{\n" " std::vector v;\n" " return v;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Reference to auto variable returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Reference to local variable returned.\n", errout.str()); check("std::vector &foo()\n" "{\n" @@ -942,7 +952,7 @@ private: " std::string s;\n" " return s;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (error) Reference to auto variable returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (error) Reference to local variable returned.\n", errout.str()); check("class Fred {\n" " std::vector &foo();\n" @@ -952,7 +962,7 @@ private: " std::vector v;\n" " return v;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (error) Reference to auto variable returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (error) Reference to local variable returned.\n", errout.str()); check("class Fred {\n" " std::vector &foo();\n" @@ -1188,6 +1198,23 @@ private: ASSERT_EQUALS("", errout.str()); } + void danglingReference() { + check("int &f( int k )\n" + "{\n" + " static int &r = k;\n" + " return r;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (error) Non-local reference variable 'r' to local variable 'k'\n", + errout.str()); + + check("int &f( int & k )\n" + "{\n" + " static int &r = k;\n" + " return r;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void testglobalnamespace() { check("class SharedPtrHolder\n" "{\n"