walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Extend lifetime checking to temporaries (#2242) 

* Use lifetimes to check for returning reference to temporaries

* Check for dangling temporaries

* Check for unknown types for returining by reference

* Remove old returnTemporary check

* Format

* Check for deref op

* Ternary operator return an lvalue reference

* Warn when returning temporaries from member functions

* Improve handling of pointer to function

* Extend lifetimes of const references
This commit is contained in:
Paul Fultz II 2019-10-08 02:28:39 -05:00 committed by Daniel Marjamäki
parent 21774cbdc4
commit 4eb4762d95
8 changed files with 262 additions and 123 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
lib/astutils.cpp
View File

@ -220,6 +220,25 @@ const Token * astIsVariableComparison(const Token *tok, const std::string &comp,
return ret;
}
bool isTemporary(bool cpp, const Token* tok)
{
if (!tok)
return false;
if (Token::simpleMatch(tok, "."))
return isTemporary(cpp, tok->astOperand1()) || isTemporary(cpp, tok->astOperand2());
if (Token::Match(tok, ",|::"))
return isTemporary(cpp, tok->astOperand2());
if (Token::Match(tok, "?|.|[|++|--|%name%|%assign%"))
return false;
if (tok->isUnaryOp("*"))
return false;
if (Token::Match(tok, "&|<<|>>") && isLikelyStream(cpp, tok->astOperand1()))
return false;
if (Token::Match(tok->previous(), "%name% ("))
return tok->previous()->function() && !Function::returnsReference(tok->previous()->function(), true);
return true;
}
static bool isFunctionCall(const Token* tok)
{
if (Token::Match(tok, "%name% ("))

2
lib/astutils.h
View File

@ -87,6 +87,8 @@ std::string astCanonicalType(const Token *expr);
/** Is given syntax tree a variable comparison against value */
const Token * astIsVariableComparison(const Token *tok, const std::string &comp, const std::string &rhs, const Token **vartok=nullptr);
bool isTemporary(bool cpp, const Token* tok);
const Token * nextAfterAstRightmostLeaf(const Token * tok);
Token* astParentSkipParens(Token* tok);

137
lib/checkautovariables.cpp
View File

@ -357,48 +357,6 @@ void CheckAutoVariables::errorUselessAssignmentPtrArg(const Token *tok)
//---------------------------------------------------------------------------
// return temporary?
bool CheckAutoVariables::returnTemporary(const Token *tok)
{
bool func = false; // Might it be a function call?
bool retref = false; // is there such a function that returns a reference?
bool retvalue = false; // is there such a function that returns a value?
const Function *function = tok->function();
if (function) {
// Ticket #5478: Only functions or operator equal might return a temporary
if (function->type != Function::eOperatorEqual && function->type != Function::eFunction)
return false;
retref = function->tokenDef->strAt(-1) == "&";
if (!retref) {
const Token *start = function->retDef;
if (start->str() == "const")
start = start->next();
if (start->str() == "::")
start = start->next();
if (Token::simpleMatch(start, "std ::")) {
if (start->strAt(3) != "<" || !Token::simpleMatch(start->linkAt(3), "> ::"))
retvalue = true;
else
retref = true; // Assume that a reference is returned
} else {
if (start->type())
retvalue = true;
else
retref = true;
}
}
func = true;
}
if (!func && tok->type())
return true;
return (!retref && retvalue);
}
//---------------------------------------------------------------------------
static bool astHasAutoResult(const Token *tok)
{
if (tok->astOperand1() && !astHasAutoResult(tok->astOperand1()))
@ -436,58 +394,6 @@ static bool astHasAutoResult(const Token *tok)
return false;
}
void CheckAutoVariables::returnReference()
{
if (mTokenizer->isC())
return;
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
for (const Scope * scope : symbolDatabase->functionScopes) {
if (!scope->function)
continue;
const Token *tok = scope->function->tokenDef;
// have we reached a function that returns a reference?
if (tok->previous() && tok->previous()->str() == "&") {
for (const Token *tok2 = scope->bodyStart->next(); tok2 && tok2 != scope->bodyEnd; tok2 = tok2->next()) {
if (!tok2->scope()->isExecutable()) {
tok2 = tok2->scope()->bodyEnd;
if (!tok2)
break;
continue;
}
// Skip over lambdas
const Token *lambdaEndToken = findLambdaEndToken(tok2);
if (lambdaEndToken)
tok2 = lambdaEndToken->next();
if (tok2->str() == "(")
tok2 = tok2->link();
if (tok2->str() != "return")
continue;
// return reference to temporary..
if (Token::Match(tok2, "return %name% (") && Token::simpleMatch(tok2->linkAt(2), ") ;")) {
if (returnTemporary(tok2->next())) {
// report error..
errorReturnTempReference(tok2);
}
}
// Return reference to a literal or the result of a calculation
else if (tok2->astOperand1() && (tok2->astOperand1()->isCalculation() || tok2->next()->isLiteral()) &&
astHasAutoResult(tok2->astOperand1())) {
errorReturnTempReference(tok2);
}
}
}
}
}
static bool isInScope(const Token * tok, const Scope * scope)
{
if (!tok)
@ -534,6 +440,15 @@ static int getPointerDepth(const Token *tok)
return tok->valueType() ? tok->valueType()->pointer : 0;
}
static bool isDeadTemporary(bool cpp, const Token* tok, const Token* expr)
{
if (!isTemporary(cpp, tok))
return false;
if (expr && !precedes(nextAfterAstRightmostLeaf(tok->astTop()), nextAfterAstRightmostLeaf(expr->astTop())))
return false;
return true;
}
void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token * end)
{
if (!start)
@ -550,12 +465,13 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token
if (returnRef && Token::simpleMatch(tok->astParent(), "return")) {
for (const LifetimeToken& lt : getLifetimeTokens(tok)) {
const Variable* var = lt.token->variable();
if (!var)
continue;
if (!var->isGlobal() && !var->isStatic() && !var->isReference() && !var->isRValueReference() &&
if (var && !var->isGlobal() && !var->isStatic() && !var->isReference() && !var->isRValueReference() &&
isInScope(var->nameToken(), tok->scope())) {
errorReturnReference(tok, lt.errorPath, lt.inconclusive);
break;
} else if (isDeadTemporary(mTokenizer->isCPP(), lt.token, nullptr)) {
errorReturnTempReference(tok, lt.errorPath, lt.inconclusive);
break;
}
}
// Assign reference to non-local variable
@ -573,21 +489,23 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token
for (const ValueFlow::Value& val:tok->values()) {
if (!val.isLocalLifetimeValue())
continue;
if (!val.tokvalue->variable())
continue;
if (Token::Match(tok->astParent(), "return|throw")) {
if (getPointerDepth(tok) < getPointerDepth(val.tokvalue))
continue;
if (!isLifetimeBorrowed(tok, mSettings))
continue;
if (isInScope(val.tokvalue->variable()->nameToken(), scope)) {
if ((val.tokvalue->variable() && isInScope(val.tokvalue->variable()->nameToken(), scope)) ||
isDeadTemporary(mTokenizer->isCPP(), val.tokvalue, tok)) {
errorReturnDanglingLifetime(tok, &val);
break;
}
} else if (isDeadScope(val.tokvalue->variable()->nameToken(), tok->scope())) {
} else if (val.tokvalue->variable() && isDeadScope(val.tokvalue->variable()->nameToken(), tok->scope())) {
errorInvalidLifetime(tok, &val);
break;
} else if (isInScope(val.tokvalue->variable()->nameToken(), tok->scope())) {
} else if (!val.tokvalue->variable() && isDeadTemporary(mTokenizer->isCPP(), val.tokvalue, tok)) {
errorDanglingTemporaryLifetime(tok, &val);
break;
} else if (val.tokvalue->variable() && isInScope(val.tokvalue->variable()->nameToken(), tok->scope())) {
const Variable * var = nullptr;
const Token * tok2 = tok;
if (Token::simpleMatch(tok->astParent(), "=")) {
@ -654,6 +572,15 @@ void CheckAutoVariables::errorInvalidLifetime(const Token *tok, const ValueFlow:
reportError(errorPath, Severity::error, "invalidLifetime", msg + " that is out of scope.", CWE562, inconclusive);
}
void CheckAutoVariables::errorDanglingTemporaryLifetime(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, "danglingTemporaryLifetime", msg + " to temporary.", CWE562, inconclusive);
}
void CheckAutoVariables::errorDanglngLifetime(const Token *tok, const ValueFlow::Value *val)
{
const bool inconclusive = val ? val->isInconclusive() : false;
@ -680,9 +607,11 @@ void CheckAutoVariables::errorDanglingReference(const Token *tok, const Variable
reportError(errorPath, Severity::error, "danglingReference", msg, CWE562, false);
}
void CheckAutoVariables::errorReturnTempReference(const Token *tok)
void CheckAutoVariables::errorReturnTempReference(const Token* tok, ErrorPath errorPath, bool inconclusive)
{
reportError(tok, Severity::error, "returnTempReference", "Reference to temporary returned.", CWE562, false);
errorPath.emplace_back(tok, "");
reportError(
errorPath, Severity::error, "returnTempReference", "Reference to temporary returned.", CWE562, inconclusive);
}
void CheckAutoVariables::errorInvalidDeallocation(const Token *tok, const ValueFlow::Value *val)

17
lib/checkautovariables.h
View File

@ -52,7 +52,6 @@ public:
void runChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) OVERRIDE {
CheckAutoVariables checkAutoVariables(tokenizer, settings, errorLogger);
checkAutoVariables.assignFunctionArg();
checkAutoVariables.returnReference();
checkAutoVariables.checkVarLifetime();
checkAutoVariables.autoVariables();
}
@ -63,21 +62,11 @@ public:
/** Check auto variables */
void autoVariables();
/** Returning reference to local/temporary variable */
void returnReference();
void checkVarLifetime();
void checkVarLifetimeScope(const Token * start, const Token * end);
private:
/**
* Returning a temporary object?
* @param tok pointing at the "return" token
* @return true if a temporary object is returned
*/
static bool returnTemporary(const Token *tok);
void errorReturnAddressToAutoVariable(const Token *tok);
void errorReturnAddressToAutoVariable(const Token *tok, const ValueFlow::Value *value);
void errorReturnPointerToLocalArray(const Token *tok);
@ -85,9 +74,10 @@ 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 errorDanglingTemporaryLifetime(const Token* tok, const ValueFlow::Value* val);
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 errorReturnTempReference(const Token* tok, ErrorPath errorPath, bool inconclusive);
void errorInvalidDeallocation(const Token *tok, const ValueFlow::Value *val);
void errorReturnAddressOfFunctionParameter(const Token *tok, const std::string &varname);
void errorUselessAssignmentArg(const Token *tok);
@ -101,7 +91,7 @@ private:
c.errorReturnPointerToLocalArray(nullptr);
c.errorReturnReference(nullptr, errorPath, false);
c.errorDanglingReference(nullptr, nullptr, errorPath);
c.errorReturnTempReference(nullptr);
c.errorReturnTempReference(nullptr, errorPath, false);
c.errorInvalidDeallocation(nullptr, nullptr);
c.errorReturnAddressOfFunctionParameter(nullptr, "parameter");
c.errorUselessAssignmentArg(nullptr);
@ -109,6 +99,7 @@ private:
c.errorReturnDanglingLifetime(nullptr, nullptr);
c.errorInvalidLifetime(nullptr, nullptr);
c.errorDanglngLifetime(nullptr, nullptr);
c.errorDanglingTemporaryLifetime(nullptr, nullptr);
}
static std::string myName() {

13
lib/symboldatabase.cpp
View File

@ -2186,13 +2186,22 @@ bool Function::argsMatch(const Scope *scope, const Token *first, const Token *se
return false;
}
bool Function::returnsReference(const Function *function)
bool Function::returnsReference(const Function* function, bool unknown)
{
if (!function)
return false;
if (function->type != Function::eFunction)
return false;
return function->tokenDef->strAt(-1) == "&";
const Token* defEnd = function->returnDefEnd();
if (defEnd->strAt(-1) == "&")
return true;
// Check for unknown types, which could be a reference
const Token* start = function->retDef;
while (Token::Match(start, "const|volatile"))
start = start->next();
if (start->tokAt(1) == defEnd && !start->type() && !start->isStandardType())
return unknown;
return false;
}
const Token * Function::constructorMemberInitialization() const

2
lib/symboldatabase.h
View File

@ -868,7 +868,7 @@ public:
static bool argsMatch(const Scope *scope, const Token *first, const Token *second, const std::string &path, nonneg int path_length);
static bool returnsReference(const Function *function);
static bool returnsReference(const Function* function, bool unknown = false);
const Token* returnDefEnd() const {
if (this->hasTrailingReturnType()) {

2
lib/valueflow.cpp
View File

@ -3346,7 +3346,7 @@ std::vector<LifetimeToken> getLifetimeTokens(const Token* tok, ValueFlow::Value:
} else if (Token::simpleMatch(var->declEndToken(), "=")) {
errorPath.emplace_back(var->declEndToken(), "Assigned to reference.");
const Token *vartok = var->declEndToken()->astOperand2();
if (vartok == tok)
if (vartok == tok || (var->isConst() && isTemporary(true, vartok)))
return {{tok, true, std::move(errorPath)}};
if (vartok)
return getLifetimeTokens(vartok, std::move(errorPath), depth - 1);

193
test/testautovariables.cpp
View File

@ -100,6 +100,11 @@ private:
TEST_CASE(returnReference5);
TEST_CASE(returnReference6);
TEST_CASE(returnReference7);
TEST_CASE(returnReference8);
TEST_CASE(returnReference9);
TEST_CASE(returnReference10);
TEST_CASE(returnReference11);
TEST_CASE(returnReference12);
TEST_CASE(returnReferenceFunction);
TEST_CASE(returnReferenceContainer);
TEST_CASE(returnReferenceLiteral);
@ -107,6 +112,7 @@ private:
TEST_CASE(returnReferenceLambda);
TEST_CASE(returnReferenceInnerScope);
TEST_CASE(returnReferenceRecursive);
TEST_CASE(extendedLifetime);
TEST_CASE(danglingReference);
@ -126,6 +132,7 @@ private:
TEST_CASE(danglingLifetimeAggegrateConstructor);
TEST_CASE(danglingLifetimeInitList);
TEST_CASE(danglingLifetimeImplicitConversion);
TEST_CASE(danglingTemporaryLifetime);
TEST_CASE(invalidLifetime);
TEST_CASE(deadPointer);
}
@ -1139,6 +1146,73 @@ private:
ASSERT_EQUALS("", errout.str());
}
void returnReference8() {
check("int& f(std::vector<int> &v) {\n"
" std::vector<int>::iterator it = v.begin();\n"
" int& value = *it;\n"
" return value;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void returnReference9() {
check("int& f(bool b, int& x, int& y) {\n"
" return b ? x : y;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void returnReference10() {
check("class A { int f() const; };\n"
"int& g() {\n"
" A a;\n"
" return a.f();\n"
"}\n");
ASSERT_EQUALS("[test.cpp:4]: (error) Reference to temporary returned.\n", errout.str());
check("class A { int& f() const; };\n"
"int& g() {\n"
" A a;\n"
" return a.f();\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void returnReference11() {
check("class A { static int f(); };\n"
"int& g() {\n"
" return A::f();\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (error) Reference to temporary returned.\n", errout.str());
check("class A { static int& f(); };\n"
"int& g() {\n"
" return A::f();\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("namespace A { int& f(); }\n"
"int& g() {\n"
" return A::f();\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void returnReference12() {
check("class A { static int& f(); };\n"
"auto g() {\n"
" return &A::f;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("class A { static int& f(); };\n"
"auto g() {\n"
" auto x = &A::f;\n"
" return x;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void returnReferenceFunction() {
check("int& f(int& a) {\n"
" return a;\n"
@ -1186,7 +1260,7 @@ private:
" int x = 0;\n"
" return f(x);\n"
"}\n");
ASSERT_EQUALS("", errout.str());
ASSERT_EQUALS("[test.cpp:6]: (error) Reference to temporary returned.\n", errout.str());
check("int& f(int a) {\n"
" return a;\n"
@ -1204,7 +1278,7 @@ private:
" int x = 0;\n"
" return f(x);\n"
"}\n");
ASSERT_EQUALS("", errout.str());
ASSERT_EQUALS("[test.cpp:6]: (error) Reference to temporary returned.\n", errout.str());
check("template<class T>\n"
"int& f(int& x, T y) {\n"
@ -1221,6 +1295,23 @@ private:
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (error) Reference to local variable returned.\n", errout.str());
check("auto& f() {\n"
" std::vector<int> x;\n"
" return x.front();\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (error) Reference to local variable returned.\n", errout.str());
check("std::vector<int> g();\n"
"auto& f() {\n"
" return g().front();\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (error) Reference to temporary returned.\n", errout.str());
check("auto& f() {\n"
" return std::vector<int>{1}.front();\n"
"}\n");
TODO_ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (error) Reference to temporary returned.\n", "", errout.str());
check("struct A { int foo; };\n"
"int& f(std::vector<A> v) {\n"
" auto it = v.begin();\n"
@ -1243,6 +1334,20 @@ private:
"[test.cpp:2] -> [test.cpp:4] -> [test.cpp:9] -> [test.cpp:9]: (error, inconclusive) Reference to local variable returned.\n",
errout.str());
check("template <class T, class K, class V>\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<int, int>& m, int k) {\n"
" return get_default(m, k, 0);\n"
"}\n",
true);
ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:4] -> [test.cpp:8] -> [test.cpp:8]: (error, inconclusive) Reference to temporary returned.\n",
errout.str());
check("struct A { int foo; };\n"
"int& f(std::vector<A>& v) {\n"
" auto it = v.begin();\n"
@ -1261,6 +1366,20 @@ private:
" return \"foo\";\n"
"}");
ASSERT_EQUALS("", errout.str());
check("const std::string& f(const std::string& x) { return x; }\n"
"const std::string &a() {\n"
" return f(\"foo\");\n"
"}");
ASSERT_EQUALS(
"[test.cpp:1] -> [test.cpp:1] -> [test.cpp:3] -> [test.cpp:3]: (error) Reference to temporary returned.\n",
errout.str());
check("const char * f(const char * x) { return x; }\n"
"const std::string &a() {\n"
" return f(\"foo\");\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (error) Reference to temporary returned.\n", errout.str());
}
void returnReferenceCalculation() {
@ -1364,6 +1483,40 @@ private:
ASSERT_EQUALS("", errout.str());
}
void extendedLifetime() {
check("void g(int*);\n"
"int h();\n"
"auto f() {\n"
" const int& x = h();\n"
" return [&] { return x; };\n"
"}\n");
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5] -> [test.cpp:4] -> [test.cpp:5]: (error) Returning lambda that captures local variable 'x' that will be invalid when returning.\n", errout.str());
check("void g(int*);\n"
"int h();\n"
"int* f() {\n"
" const int& x = h();\n"
" return &x;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5] -> [test.cpp:4] -> [test.cpp:5]: (error) Returning pointer to local variable 'x' that will be invalid when returning.\n", errout.str());
check("void g(int*);\n"
"int h();\n"
"void f() {\n"
" int& x = h();\n"
" g(&x);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5] -> [test.cpp:5]: (error) Using pointer to temporary.\n", errout.str());
check("void g(int*);\n"
"int h();\n"
"void f() {\n"
" const int& x = h();\n"
" g(&x);\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void danglingReference() {
check("int f( int k )\n"
"{\n"
@ -1696,6 +1849,28 @@ private:
"[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("std::vector<int> g();\n"
"auto f() {\n"
" return g().begin();\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:3] -> [test.cpp:3]: (error) Returning iterator that will be invalid when returning.\n",
"",
errout.str());
check("std::vector<int> f();\n"
"auto f() {\n"
" auto it = g().begin();\n"
" return it;\n"
"}\n");
TODO_ASSERT_EQUALS("error", "", errout.str());
check("std::vector<int> f();\n"
"int& f() {\n"
" return *g().begin();\n"
"}\n");
TODO_ASSERT_EQUALS("error", "", errout.str());
check("struct A {\n"
" std::vector<std::string> v;\n"
" void f() {\n"
@ -2174,6 +2349,20 @@ private:
ASSERT_EQUALS("", errout.str());
}
void danglingTemporaryLifetime()
{
check("const int& g(const int& x) {\n"
" return x;\n"
"}\n"
"void f(int& i) {\n"
" int* x = &g(0);\n"
" i += *x;\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:1] -> [test.cpp:2] -> [test.cpp:5] -> [test.cpp:5] -> [test.cpp:6]: (error) Using pointer to temporary.\n",
errout.str());
}
void invalidLifetime() {
check("void foo(int a) {\n"
" std::function<void()> f;\n"