From 8c03be32122ed85af89050eb1ad07ddd7038508e Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 5 May 2019 04:40:59 -0500 Subject: [PATCH] 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 --- lib/checkautovariables.cpp | 5 +-- lib/valueflow.cpp | 85 +++++++++++++++++++++++++++++++++++--- lib/valueflow.h | 2 + test/testautovariables.cpp | 36 ++++++++++++++++ 4 files changed, 119 insertions(+), 9 deletions(-) diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index 853473762..71066d42f 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -590,7 +590,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 + // Return reference from function if (returnRef && Token::simpleMatch(tok->astParent(), "return")) { ErrorPath errorPath; const Variable *var = getLifetimeVariable(tok, errorPath); @@ -619,8 +619,7 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token if (Token::Match(tok->astParent(), "return|throw")) { if (getPointerDepth(tok) < getPointerDepth(val.tokvalue)) continue; - if (tok->astParent()->str() == "return" && !Token::simpleMatch(tok, "{") && !astIsContainer(tok) && - astIsContainer(tok->astParent())) + if (!isLifetimeBorrowed(tok, mSettings)) continue; if (isInScope(val.tokvalue->variable()->nameToken(), scope)) { errorReturnDanglingLifetime(tok, &val); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index f3d1c77cf..e83865f13 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2829,6 +2829,83 @@ static const Variable *getLHSVariable(const Token *tok) return getLHSVariableRecursive(tok->astOperand1()); } +static bool isLifetimeOwned(const ValueType *vt, const ValueType *vtParent) +{ + if (!vtParent) + return false; + if (!vt) { + if (vtParent->type == ValueType::CONTAINER) + return true; + return false; + } + if (vt->type != ValueType::UNKNOWN_TYPE && vtParent->type != ValueType::UNKNOWN_TYPE) { + if (vt->pointer != vtParent->pointer) + return true; + if (vt->type != vtParent->type) { + if (vtParent->type == ValueType::RECORD) + return true; + if (vtParent->type == ValueType::CONTAINER) + return true; + } + } + + return false; +} + +static bool isLifetimeBorrowed(const ValueType *vt, const ValueType *vtParent) +{ + if (!vtParent) + return false; + if (!vt) + return false; + if (vt->type != ValueType::UNKNOWN_TYPE && vtParent->type != ValueType::UNKNOWN_TYPE) { + if (vtParent->pointer > vt->pointer) + return true; + if (vtParent->pointer < vt->pointer && vtParent->isIntegral()) + return true; + } + + return false; +} + +bool isLifetimeBorrowed(const Token *tok, const Settings *settings) +{ + if (!tok) + return true; + if (Token::simpleMatch(tok, ",")) + return true; + if (!tok->astParent()) + return true; + if (!Token::Match(tok->astParent()->previous(), "%name% (") && !Token::simpleMatch(tok->astParent(), ",")) { + if (!Token::simpleMatch(tok, "{")) { + const ValueType *vt = tok->valueType(); + const ValueType *vtParent = tok->astParent()->valueType(); + if (isLifetimeBorrowed(vt, vtParent)) + return true; + if (isLifetimeOwned(vt, vtParent)) + return false; + } + const Type *t = Token::typeOf(tok); + const Type *parentT = Token::typeOf(tok->astParent()); + if (t && parentT && t->classDef && parentT->classDef && t->classDef != parentT->classDef) { + return false; + } + } else if (Token::Match(tok->astParent()->tokAt(-3), "%var% . push_back|push_front|insert|push (") && + astIsContainer(tok->astParent()->tokAt(-3))) { + const ValueType *vt = tok->valueType(); + const ValueType *vtCont = tok->astParent()->tokAt(-3)->valueType(); + if (!vtCont->containerTypeToken) + return true; + ValueType vtParent = ValueType::parseDecl(vtCont->containerTypeToken, settings); + if (isLifetimeBorrowed(vt, &vtParent)) + return true; + if (isLifetimeOwned(vt, &vtParent)) + return false; + } + + return true; +} + static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings); static void valueFlowLifetimeConstructor(Token *tok, @@ -2855,8 +2932,7 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog if (!parent->astOperand2() || parent->astOperand2()->values().empty()) return; - if (astIsPointer(parent->astOperand2()) && !var->isPointer() && - !(var->valueType() && var->valueType()->isIntegral())) + if (!isLifetimeBorrowed(parent->astOperand2(), settings)) return; std::list values = parent->astOperand2()->values(); @@ -3075,9 +3151,6 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog } } else if (Token::Match(tok->tokAt(-2), "%var% . push_back|push_front|insert|push|assign") && astIsContainer(tok->tokAt(-2))) { - const Token *containerTypeTok = tok->tokAt(-2)->valueType()->containerTypeToken; - const Token *endTypeTok = endTemplateArgument(containerTypeTok); - const bool isPointer = endTypeTok && Token::simpleMatch(endTypeTok->previous(), "*"); Token *vartok = tok->tokAt(-2); std::vector args = getArguments(tok); std::size_t n = args.size(); @@ -3086,7 +3159,7 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog (astIsPointer(args[n - 2]) && astIsPointer(args[n - 1]))))) { LifetimeStore{args.back(), "Added to container '" + vartok->str() + "'.", ValueFlow::Value::Object} .byDerefCopy( vartok, tokenlist, errorLogger, settings); - } else if (!args.empty() && astIsPointer(args.back()) == isPointer) { + } else if (!args.empty() && isLifetimeBorrowed(args.back(), settings)) { LifetimeStore{args.back(), "Added to container '" + vartok->str() + "'.", ValueFlow::Value::Object} .byVal( vartok, tokenlist, errorLogger, settings); } diff --git a/lib/valueflow.h b/lib/valueflow.h index 51d1b8713..fc77fe69b 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -238,6 +238,8 @@ namespace ValueFlow { const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath); +bool isLifetimeBorrowed(const Token *tok, const Settings *settings); + std::string lifetimeType(const Token *tok, const ValueFlow::Value *val); #endif // valueflowH diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index ea3635e77..5c19f6b8d 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -124,6 +124,7 @@ private: TEST_CASE(danglingLifetimeFunction); TEST_CASE(danglingLifetimeAggegrateConstructor); TEST_CASE(danglingLifetimeInitList); + TEST_CASE(danglingLifetimeImplicitConversion); TEST_CASE(invalidLifetime); } @@ -1618,6 +1619,14 @@ private: "[test.cpp:2] -> [test.cpp:1] -> [test.cpp:3]: (error) Returning iterator to local container 'v' that will be invalid when returning.\n", errout.str()); + check("const char * f() {\n" + " std::string ba(\"hello\");\n" + " return ba.c_str();\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'ba' that will be invalid when returning.\n", + errout.str()); + check("struct A {\n" " std::vector v;\n" " void f() {\n" @@ -2010,6 +2019,33 @@ private: ASSERT_EQUALS("", errout.str()); } + void danglingLifetimeImplicitConversion() + { + check("struct A { A(const char *a); };\n" + "A f() {\n" + " std::string ba(\"hello\");\n" + " return ba.c_str();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct A { A(const char *a); };\n" + "A f() {\n" + " std::string ba(\"hello\");\n" + " A bp = ba.c_str();\n" + " return bp;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct A { A(const char *a); };\n" + "std::vector f() {\n" + " std::string ba(\"hello\");\n" + " std::vector v;\n" + " v.push_back(ba.c_str());\n" + " return v;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void invalidLifetime() { check("void foo(int a) {\n" " std::function f;\n"