diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index 1848c5714..e95b8689a 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -604,13 +604,22 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token if (val.tokvalue == tok) continue; if (Token::Match(tok->astParent(), "return|throw")) { - if (getPointerDepth(tok) >= getPointerDepth(val.tokvalue) && isInScope(val.tokvalue, scope)) { + if (getPointerDepth(tok) < getPointerDepth(val.tokvalue)) + continue; + if (tok->astParent()->str() == "return" && !astIsContainer(tok) && scope->function && + mSettings->library.detectContainer(scope->function->retDef)) + continue; + if (isInScope(val.tokvalue, scope)) { errorReturnDanglingLifetime(tok, &val); break; } } else if (isDeadScope(val.tokvalue, tok->scope())) { errorInvalidLifetime(tok, &val); break; + } else if (tok->variable() && !tok->variable()->isLocal() && !tok->variable()->isArgument() && + isInScope(val.tokvalue, tok->scope())) { + errorDanglngLifetime(tok, &val); + break; } } const Token *lambdaEndToken = findLambdaEndToken(tok); @@ -627,10 +636,6 @@ void CheckAutoVariables::checkVarLifetime() for (const Scope * scope : symbolDatabase->functionScopes) { if (!scope->function) continue; - // Skip if returning a container - const Library::Container * container = mSettings->library.detectContainer(scope->function->retDef); - if (container) - continue; checkVarLifetimeScope(scope->bodyStart, scope->bodyEnd); } } @@ -657,12 +662,11 @@ static std::string lifetimeType(const Token *tok, const ValueFlow::Value* val) return result; } -void CheckAutoVariables::errorReturnDanglingLifetime(const Token *tok, const ValueFlow::Value* val) +static std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, ErrorPath &errorPath) { const Token *vartok = val ? val->tokvalue : nullptr; - ErrorPath errorPath = val ? val->errorPath : ErrorPath(); std::string type = lifetimeType(tok, val); - std::string msg = "Returning " + type; + std::string msg = type; if (vartok) { errorPath.emplace_back(vartok, "Variable created here."); const Variable * var = vartok->variable(); @@ -684,41 +688,34 @@ void CheckAutoVariables::errorReturnDanglingLifetime(const Token *tok, const Val msg += " '" + var->name() + "'"; } } + return msg; +} + +void CheckAutoVariables::errorReturnDanglingLifetime(const Token *tok, const ValueFlow::Value *val) +{ + ErrorPath errorPath = val ? val->errorPath : ErrorPath(); + std::string msg = "Returning " + lifetimeMessage(tok, val, errorPath); errorPath.emplace_back(tok, ""); reportError(errorPath, Severity::error, "returnDanglingLifetime", msg + " that will be invalid when returning.", CWE562, false); } void CheckAutoVariables::errorInvalidLifetime(const Token *tok, const ValueFlow::Value* val) { - const Token *vartok = val ? val->tokvalue : nullptr; ErrorPath errorPath = val ? val->errorPath : ErrorPath(); - std::string type = lifetimeType(tok, val); - std::string msg = "Using " + type; - if (vartok) { - errorPath.emplace_back(vartok, "Variable created here."); - const Variable * var = vartok->variable(); - if (var) { - switch (val->lifetimeKind) { - case ValueFlow::Value::Object: - if (type == "pointer") - msg += " to local variable"; - else - msg += " that points to local variable"; - break; - case ValueFlow::Value::Lambda: - msg += " that captures local variable"; - break; - case ValueFlow::Value::Iterator: - msg += " to local container"; - break; - } - msg += " '" + var->name() + "'"; - } - } + std::string msg = "Using " + lifetimeMessage(tok, val, errorPath); errorPath.emplace_back(tok, ""); reportError(errorPath, Severity::error, "invalidLifetime", msg + " that is out of scope.", CWE562, false); } +void CheckAutoVariables::errorDanglngLifetime(const Token *tok, const ValueFlow::Value *val) +{ + ErrorPath errorPath = val ? val->errorPath : ErrorPath(); + std::string tokName = tok ? tok->str() : "x"; + std::string msg = "Non-local variable '" + tokName + "' will use " + lifetimeMessage(tok, val, errorPath); + errorPath.emplace_back(tok, ""); + reportError(errorPath, Severity::error, "danglingLifetime", msg + ".", CWE562, false); +} + void CheckAutoVariables::errorReturnReference(const Token *tok) { reportError(tok, Severity::error, "returnReference", "Reference to auto variable returned.", CWE562, false); diff --git a/lib/checkautovariables.h b/lib/checkautovariables.h index 7d043670a..40e03ff38 100644 --- a/lib/checkautovariables.h +++ b/lib/checkautovariables.h @@ -90,6 +90,7 @@ private: void errorAutoVariableAssignment(const Token *tok, bool inconclusive); 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 errorReturnTempReference(const Token *tok); void errorInvalidDeallocation(const Token *tok, const ValueFlow::Value *val); @@ -111,6 +112,7 @@ private: c.errorUselessAssignmentPtrArg(nullptr); c.errorReturnDanglingLifetime(nullptr, nullptr); c.errorInvalidLifetime(nullptr, nullptr); + c.errorDanglngLifetime(nullptr, nullptr); } static std::string myName() { diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 2df92e26d..f72f8902b 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2500,7 +2500,7 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) { const Token *parent = tok->astParent(); - while (parent && parent->isArithmeticalOp()) + while (parent && (parent->isArithmeticalOp() || parent->str() == ",")) parent = parent->astParent(); if (!parent) return; @@ -2547,6 +2547,27 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog // Function call } else if (Token::Match(parent->previous(), "%name% (")) { valueFlowLifetimeFunction(const_cast(parent->previous()), tokenlist, errorLogger, settings); + // Variable + } else if (tok->variable()) { + const Variable *var = tok->variable(); + if (!var->typeStartToken() && !var->typeStartToken()->scope()) + return; + const Token *endOfVarScope = var->typeStartToken()->scope()->bodyEnd; + + std::list values = tok->values(); + const Token *nextExpression = nextAfterAstRightmostLeaf(parent); + // Only forward lifetime values + values.remove_if(&isNotLifetimeValue); + valueFlowForward(const_cast(nextExpression), + endOfVarScope, + var, + var->declarationId(), + values, + false, + false, + tokenlist, + errorLogger, + settings); } } @@ -2615,8 +2636,46 @@ struct LifetimeStore { return true; }); } + + template + void byDerefCopy(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const { + for (const ValueFlow::Value &v : argtok->values()) { + if (!v.isLifetimeValue()) + continue; + const Token *tok2 = v.tokvalue; + ErrorPath errorPath = v.errorPath; + const Variable *var = getLifetimeVariable(tok2, errorPath); + if (!var) + continue; + for (const Token *tok3 = tok; tok != var->declEndToken(); tok3 = tok3->previous()) { + if (tok3->varId() == var->declarationId()) { + LifetimeStore{tok3, message, type} .byVal(tok, tokenlist, errorLogger, settings, pred); + break; + } + } + } + } + + void byDerefCopy(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) const { + byDerefCopy(tok, tokenlist, errorLogger, settings, [](const Variable *) { + return true; + }); + } }; +static const Token *endTemplateArgument(const Token *tok) +{ + for (; tok; tok = tok->next()) { + if (Token::Match(tok, ">|,")) + return tok; + else if (tok->link() && Token::Match(tok, "(|{|[|<")) + tok = tok->link(); + else if (Token::simpleMatch(tok, ";")) + return nullptr; + } + return nullptr; +} + static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) { if (!Token::Match(tok, "%name% (")) @@ -2631,6 +2690,21 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog LifetimeStore{argtok, "Passed to '" + tok->str() + "'.", ValueFlow::Value::Object} .byVal( tok->next(), tokenlist, errorLogger, settings); } + } 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); + if (args.size() == 2 && astCanonicalType(args[0]) == astCanonicalType(args[1]) && + (((astIsIterator(args[0]) && astIsIterator(args[1])) || (astIsPointer(args[0]) && astIsPointer(args[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) { + LifetimeStore{args.back(), "Added to container '" + vartok->str() + "'.", ValueFlow::Value::Object} .byVal( + vartok, tokenlist, errorLogger, settings); + } } } @@ -3369,11 +3443,14 @@ static void execute(const Token *expr, else if (expr->str() == "[" && expr->astOperand1() && expr->astOperand2()) { const Token *tokvalue = nullptr; if (!programMemory->getTokValue(expr->astOperand1()->varId(), &tokvalue)) { - if (expr->astOperand1()->values().size() != 1U || !expr->astOperand1()->values().front().isTokValue()) { + auto tokvalue_it = std::find_if(expr->astOperand1()->values().begin(), + expr->astOperand1()->values().end(), + std::mem_fn(&ValueFlow::Value::isTokValue)); + if (tokvalue_it == expr->astOperand1()->values().end()) { *error = true; return; } - tokvalue = expr->astOperand1()->values().front().tokvalue; + tokvalue = tokvalue_it->tokvalue; } if (!tokvalue || !tokvalue->isLiteral()) { *error = true; diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index 87b2fd165..a2d834137 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -707,7 +707,9 @@ private: " char str[100] = {0};\n" " return str;\n" "}"); - ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3] -> [test.cpp:4]: (error) Returning pointer to local variable 'str' that will be invalid when returning.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:3] -> [test.cpp:4]: (error) Returning pointer to local variable 'str' that will be invalid when returning.\n", + errout.str()); check("char *foo()\n" // use ValueFlow "{\n" @@ -715,7 +717,9 @@ private: " char *p = str;\n" " return p;\n" "}"); - ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3] -> [test.cpp:5]: (error) Returning pointer to local variable 'str' that will be invalid when returning.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:3] -> [test.cpp:5]: (error) Returning pointer to local variable 'str' that will be invalid when returning.\n", + errout.str()); check("class Fred {\n" " char *foo();\n" @@ -725,7 +729,9 @@ private: " char str[100] = {0};\n" " return str;\n" "}"); - ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6] -> [test.cpp:7]: (error) Returning pointer to local variable 'str' that will be invalid when returning.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:6] -> [test.cpp:6] -> [test.cpp:7]: (error) Returning pointer to local variable 'str' that will be invalid when returning.\n", + errout.str()); check("char * format_reg(char *outbuffer_start) {\n" " return outbuffer_start;\n" @@ -779,13 +785,17 @@ private: " char x[10] = {0};\n" " return x+5;\n" "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'x' that will be invalid when returning.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'x' that will be invalid when returning.\n", + errout.str()); check("char *foo(int y) {\n" " char x[10] = {0};\n" " return (x+8)-y;\n" "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'x' that will be invalid when returning.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'x' that will be invalid when returning.\n", + errout.str()); } void returnLocalVariable5() { // cast @@ -793,7 +803,9 @@ private: " int x[10] = {0};\n" " return (char *)x;\n" "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'x' that will be invalid when returning.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'x' that will be invalid when returning.\n", + errout.str()); } void returnLocalVariable6() { // valueflow @@ -1354,6 +1366,68 @@ private: "[test.cpp:3] -> [test.cpp:4] -> [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("std::vector f() {\n" + " int i = 0;\n" + " std::vector v;\n" + " v.push_back(&i);\n" + " return v;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4] -> [test.cpp:4] -> [test.cpp:2] -> [test.cpp:5]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n", + errout.str()); + + check("std::vector f() {\n" + " std::vector r;\n" + " int i = 0;\n" + " std::vector v;\n" + " v.push_back(&i);\n" + " r.assign(v.begin(), v.end());\n" + " return r;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:5] -> [test.cpp:5] -> [test.cpp:5] -> [test.cpp:3] -> [test.cpp:7]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n", + errout.str()); + + check("struct A {\n" + " std::vector v;\n" + " void f() {\n" + " int i;\n" + " v.push_back(&i);\n" + " }\n" + "};\n"); + ASSERT_EQUALS( + "[test.cpp:5] -> [test.cpp:5] -> [test.cpp:4] -> [test.cpp:5]: (error) Non-local variable 'v' will use object that points to local variable 'i'.\n", + errout.str()); + + check("struct A {\n" + " std::vector v;\n" + " void f() {\n" + " int i;\n" + " int * p = &i;\n" + " v.push_back(p);\n" + " }\n" + "};\n"); + ASSERT_EQUALS( + "[test.cpp:5] -> [test.cpp:6] -> [test.cpp:4] -> [test.cpp:6]: (error) Non-local variable 'v' will use object that points to local variable 'i'.\n", + errout.str()); + + check("struct A {\n" + " std::vector v;\n" + " void f() {\n" + " char s[3];\n" + " v.push_back(s);\n" + " }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + check("std::vector f() {\n" + " const char * s = \"hello\";\n" + " std::vector v;\n" + " v.push_back(s);\n" + " return v;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("auto f() {\n" " static std::vector x;\n" " return x.begin();\n" @@ -1488,7 +1562,9 @@ private: " }\n" " x[3];\n" "}\n"); - ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4] -> [test.cpp:7]: (error) Using pointer to local variable 'y' that is out of scope.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:4] -> [test.cpp:4] -> [test.cpp:7]: (error) Using pointer to local variable 'y' that is out of scope.\n", + errout.str()); check("void foo(int a) {\n" " std::function f;\n" diff --git a/test/testsimplifytypedef.cpp b/test/testsimplifytypedef.cpp index 6eabb296d..370b906db 100644 --- a/test/testsimplifytypedef.cpp +++ b/test/testsimplifytypedef.cpp @@ -1604,7 +1604,7 @@ private: // The expected tokens.. const char expected2[] = "void f ( ) { char a [ 256 ] ; a = { 0 } ; char b [ 256 ] ; b = { 0 } ; }"; - ASSERT_EQUALS(expected2, tok(code2, false)); + ASSERT_EQUALS(expected2, tok(code2, false, Settings::Native, false)); ASSERT_EQUALS("", errout.str()); const char code3[] = "typedef char TString[256];\n" @@ -1615,7 +1615,7 @@ private: // The expected tokens.. const char expected3[] = "void f ( ) { char a [ 256 ] ; a = \"\" ; char b [ 256 ] ; b = \"\" ; }"; - ASSERT_EQUALS(expected3, tok(code3, false)); + ASSERT_EQUALS(expected3, tok(code3, false, Settings::Native, false)); ASSERT_EQUALS("", errout.str()); const char code4[] = "typedef char TString[256];\n" @@ -1626,7 +1626,7 @@ private: // The expected tokens.. const char expected4[] = "void f ( ) { char a [ 256 ] ; a = \"1234\" ; char b [ 256 ] ; b = \"5678\" ; }"; - ASSERT_EQUALS(expected4, tok(code4, false)); + ASSERT_EQUALS(expected4, tok(code4, false, Settings::Native, false)); ASSERT_EQUALS("", errout.str()); }