Skip to content

Commit 8c03be3

Browse files
pfultz2danmar
pfultz2
authored and
danmar
committed
Fix issue 9077: False positive: Returning pointer to local variable (#1821)
* Avoid implicit conversion for lifetimes * Fix issue 9077 * Add more tests * Rename function * Fix implicit conversion with containers * Format * Fix crash
1 parent 5b07901 commit 8c03be3

4 files changed

Lines changed: 119 additions & 9 deletions

File tree

‎lib/checkautovariables.cpp‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -590,7 +590,7 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token
590590
return;
591591
bool returnRef = Function::returnsReference(scope->function);
592592
for (const Token *tok = start; tok && tok != end; tok = tok->next()) {
593-
// Return reference form function
593+
// Return reference from function
594594
if (returnRef && Token::simpleMatch(tok->astParent(), "return")) {
595595
ErrorPath errorPath;
596596
const Variable *var = getLifetimeVariable(tok, errorPath);
@@ -619,8 +619,7 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token
619619
if (Token::Match(tok->astParent(), "return|throw")) {
620620
if (getPointerDepth(tok) < getPointerDepth(val.tokvalue))
621621
continue;
622-
if (tok->astParent()->str() == "return" && !Token::simpleMatch(tok, "{") && !astIsContainer(tok) &&
623-
astIsContainer(tok->astParent()))
622+
if (!isLifetimeBorrowed(tok, mSettings))
624623
continue;
625624
if (isInScope(val.tokvalue->variable()->nameToken(), scope)) {
626625
errorReturnDanglingLifetime(tok, &val);

‎lib/valueflow.cpp‎

Lines changed: 79 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2829,6 +2829,83 @@ static const Variable *getLHSVariable(const Token *tok)
28292829
return getLHSVariableRecursive(tok->astOperand1());
28302830
}
28312831

2832+
static bool isLifetimeOwned(const ValueType *vt, const ValueType *vtParent)
2833+
{
2834+
if (!vtParent)
2835+
return false;
2836+
if (!vt) {
2837+
if (vtParent->type == ValueType::CONTAINER)
2838+
return true;
2839+
return false;
2840+
}
2841+
if (vt->type != ValueType::UNKNOWN_TYPE && vtParent->type != ValueType::UNKNOWN_TYPE) {
2842+
if (vt->pointer != vtParent->pointer)
2843+
return true;
2844+
if (vt->type != vtParent->type) {
2845+
if (vtParent->type == ValueType::RECORD)
2846+
return true;
2847+
if (vtParent->type == ValueType::CONTAINER)
2848+
return true;
2849+
}
2850+
}
2851+
2852+
return false;
2853+
}
2854+
2855+
static bool isLifetimeBorrowed(const ValueType *vt, const ValueType *vtParent)
2856+
{
2857+
if (!vtParent)
2858+
return false;
2859+
if (!vt)
2860+
return false;
2861+
if (vt->type != ValueType::UNKNOWN_TYPE && vtParent->type != ValueType::UNKNOWN_TYPE) {
2862+
if (vtParent->pointer > vt->pointer)
2863+
return true;
2864+
if (vtParent->pointer < vt->pointer && vtParent->isIntegral())
2865+
return true;
2866+
}
2867+
2868+
return false;
2869+
}
2870+
2871+
bool isLifetimeBorrowed(const Token *tok, const Settings *settings)
2872+
{
2873+
if (!tok)
2874+
return true;
2875+
if (Token::simpleMatch(tok, ","))
2876+
return true;
2877+
if (!tok->astParent())
2878+
return true;
2879+
if (!Token::Match(tok->astParent()->previous(), "%name% (") && !Token::simpleMatch(tok->astParent(), ",")) {
2880+
if (!Token::simpleMatch(tok, "{")) {
2881+
const ValueType *vt = tok->valueType();
2882+
const ValueType *vtParent = tok->astParent()->valueType();
2883+
if (isLifetimeBorrowed(vt, vtParent))
2884+
return true;
2885+
if (isLifetimeOwned(vt, vtParent))
2886+
return false;
2887+
}
2888+
const Type *t = Token::typeOf(tok);
2889+
const Type *parentT = Token::typeOf(tok->astParent());
2890+
if (t && parentT && t->classDef && parentT->classDef && t->classDef != parentT->classDef) {
2891+
return false;
2892+
}
2893+
} else if (Token::Match(tok->astParent()->tokAt(-3), "%var% . push_back|push_front|insert|push (") &&
2894+
astIsContainer(tok->astParent()->tokAt(-3))) {
2895+
const ValueType *vt = tok->valueType();
2896+
const ValueType *vtCont = tok->astParent()->tokAt(-3)->valueType();
2897+
if (!vtCont->containerTypeToken)
2898+
return true;
2899+
ValueType vtParent = ValueType::parseDecl(vtCont->containerTypeToken, settings);
2900+
if (isLifetimeBorrowed(vt, &vtParent))
2901+
return true;
2902+
if (isLifetimeOwned(vt, &vtParent))
2903+
return false;
2904+
}
2905+
2906+
return true;
2907+
}
2908+
28322909
static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings);
28332910

28342911
static void valueFlowLifetimeConstructor(Token *tok,
@@ -2855,8 +2932,7 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog
28552932
if (!parent->astOperand2() || parent->astOperand2()->values().empty())
28562933
return;
28572934

2858-
if (astIsPointer(parent->astOperand2()) && !var->isPointer() &&
2859-
!(var->valueType() && var->valueType()->isIntegral()))
2935+
if (!isLifetimeBorrowed(parent->astOperand2(), settings))
28602936
return;
28612937

28622938
std::list<ValueFlow::Value> values = parent->astOperand2()->values();
@@ -3075,9 +3151,6 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog
30753151
}
30763152
} else if (Token::Match(tok->tokAt(-2), "%var% . push_back|push_front|insert|push|assign") &&
30773153
astIsContainer(tok->tokAt(-2))) {
3078-
const Token *containerTypeTok = tok->tokAt(-2)->valueType()->containerTypeToken;
3079-
const Token *endTypeTok = endTemplateArgument(containerTypeTok);
3080-
const bool isPointer = endTypeTok && Token::simpleMatch(endTypeTok->previous(), "*");
30813154
Token *vartok = tok->tokAt(-2);
30823155
std::vector<const Token *> args = getArguments(tok);
30833156
std::size_t n = args.size();
@@ -3086,7 +3159,7 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog
30863159
(astIsPointer(args[n - 2]) && astIsPointer(args[n - 1]))))) {
30873160
LifetimeStore{args.back(), "Added to container '" + vartok->str() + "'.", ValueFlow::Value::Object} .byDerefCopy(
30883161
vartok, tokenlist, errorLogger, settings);
3089-
} else if (!args.empty() && astIsPointer(args.back()) == isPointer) {
3162+
} else if (!args.empty() && isLifetimeBorrowed(args.back(), settings)) {
30903163
LifetimeStore{args.back(), "Added to container '" + vartok->str() + "'.", ValueFlow::Value::Object} .byVal(
30913164
vartok, tokenlist, errorLogger, settings);
30923165
}

‎lib/valueflow.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,8 @@ namespace ValueFlow {
238238

239239
const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath);
240240

241+
bool isLifetimeBorrowed(const Token *tok, const Settings *settings);
242+
241243
std::string lifetimeType(const Token *tok, const ValueFlow::Value *val);
242244

243245
#endif // valueflowH

‎test/testautovariables.cpp‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ class TestAutoVariables : public TestFixture {
124124
TEST_CASE(danglingLifetimeFunction);
125125
TEST_CASE(danglingLifetimeAggegrateConstructor);
126126
TEST_CASE(danglingLifetimeInitList);
127+
TEST_CASE(danglingLifetimeImplicitConversion);
127128
TEST_CASE(invalidLifetime);
128129
}
129130

@@ -1618,6 +1619,14 @@ class TestAutoVariables : public TestFixture {
16181619
"[test.cpp:2] -> [test.cpp:1] -> [test.cpp:3]: (error) Returning iterator to local container 'v' that will be invalid when returning.\n",
16191620
errout.str());
16201621

1622+
check("const char * f() {\n"
1623+
" std::string ba(\"hello\");\n"
1624+
" return ba.c_str();\n"
1625+
"}\n");
1626+
ASSERT_EQUALS(
1627+
"[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'ba' that will be invalid when returning.\n",
1628+
errout.str());
1629+
16211630
check("struct A {\n"
16221631
" std::vector<std::string> v;\n"
16231632
" void f() {\n"
@@ -2010,6 +2019,33 @@ class TestAutoVariables : public TestFixture {
20102019
ASSERT_EQUALS("", errout.str());
20112020
}
20122021

2022+
void danglingLifetimeImplicitConversion()
2023+
{
2024+
check("struct A { A(const char *a); };\n"
2025+
"A f() {\n"
2026+
" std::string ba(\"hello\");\n"
2027+
" return ba.c_str();\n"
2028+
"}\n");
2029+
ASSERT_EQUALS("", errout.str());
2030+
2031+
check("struct A { A(const char *a); };\n"
2032+
"A f() {\n"
2033+
" std::string ba(\"hello\");\n"
2034+
" A bp = ba.c_str();\n"
2035+
" return bp;\n"
2036+
"}\n");
2037+
ASSERT_EQUALS("", errout.str());
2038+
2039+
check("struct A { A(const char *a); };\n"
2040+
"std::vector<A> f() {\n"
2041+
" std::string ba(\"hello\");\n"
2042+
" std::vector<A> v;\n"
2043+
" v.push_back(ba.c_str());\n"
2044+
" return v;\n"
2045+
"}\n");
2046+
ASSERT_EQUALS("", errout.str());
2047+
}
2048+
20132049
void invalidLifetime() {
20142050
check("void foo(int a) {\n"
20152051
" std::function<void()> f;\n"

0 commit comments

Comments
 (0)