diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 56588ef66..f0aa7b625 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -71,6 +71,11 @@ bool astIsBool(const Token *tok) return tok && (tok->isBoolean() || (tok->valueType() && tok->valueType()->type == ValueType::Type::BOOL && !tok->valueType()->pointer)); } +bool astIsPointer(const Token *tok) +{ + return tok && tok->valueType() && tok->valueType()->pointer; +} + std::string astCanonicalType(const Token *expr) { if (!expr) diff --git a/lib/astutils.h b/lib/astutils.h index 83f7e674c..526d945fc 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -43,6 +43,8 @@ bool astIsFloat(const Token *tok, bool unknown); /** Is expression of boolean type? */ bool astIsBool(const Token *tok); +bool astIsPointer(const Token *tok); + /** * Get canonical type of expression. const/static/etc are not included and neither *&. * For example: diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index 7755bf653..d1293612f 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -281,37 +281,6 @@ void CheckAutoVariables::autoVariables() if (checkRvalueExpression(varTok)) errorAutoVariableAssignment(tok->next(), false); } - // Critical return - else if (Token::Match(tok, "return %var% ;") && isAutoVar(tok->next())) { - const std::list &values = tok->next()->values(); - const ValueFlow::Value *value = nullptr; - for (std::list::const_iterator it = values.begin(); it != values.end(); ++it) { - if (!it->isTokValue()) - continue; - if (!mSettings->inconclusive && it->isInconclusive()) - continue; - if (!Token::Match(it->tokvalue->previous(), "= & %var%")) - continue; - if (!isAutoVar(it->tokvalue->next())) - continue; - if (!value || value->isInconclusive()) - value = &(*it); - } - - if (value) - errorReturnAddressToAutoVariable(tok, value); - } - - else if (Token::Match(tok, "return & %var% ;")) { - const Token* varTok = tok->tokAt(2); - if (isAutoVar(varTok)) - errorReturnAddressToAutoVariable(tok); - else if (varTok->varId()) { - const Variable * var1 = varTok->variable(); - if (var1 && var1->isArgument() && var1->typeEndToken()->str() != "&") - errorReturnAddressOfFunctionParameter(tok, varTok->str()); - } - } // Invalid pointer deallocation else if ((Token::Match(tok, "%name% ( %var% ) ;") && mSettings->library.dealloc(tok)) || (mTokenizer->isCPP() && Token::Match(tok, "delete [| ]| (| %var% !!["))) { @@ -338,28 +307,6 @@ void CheckAutoVariables::autoVariables() //--------------------------------------------------------------------------- -void CheckAutoVariables::returnPointerToLocalArray() -{ - 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 pointer - if (tok->previous() && tok->previous()->str() == "*") { - for (const Token *tok2 = scope->bodyStart->next(); tok2 && tok2 != scope->bodyEnd; tok2 = tok2->next()) { - // Return pointer to local array variable.. - if (tok2 ->str() == "return" && isAutoVarArray(tok2->astOperand1())) { - errorReturnPointerToLocalArray(tok2); - } - } - } - } -} - void CheckAutoVariables::errorReturnAddressToAutoVariable(const Token *tok) { reportError(tok, Severity::error, "returnAddressOfAutoVariable", "Address of an auto-variable returned.", CWE562, false); @@ -643,14 +590,6 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token if (scope->bodyStart != start) return; for (const Token *tok = start; tok && tok != end; tok = tok->next()) { - // Skip duplicate warning from dangling references - if (Token::Match(tok, "& %var%")) - continue; - if (tok->variable() && tok->variable()->isPointer()) - continue; - if (std::any_of(tok->values().begin(), tok->values().end(), std::mem_fn(&ValueFlow::Value::isTokValue))) - continue; - for (const ValueFlow::Value& val:tok->values()) { if (!val.isLifetimeValue()) continue; @@ -686,29 +625,44 @@ void CheckAutoVariables::checkVarLifetime() } } -void CheckAutoVariables::errorReturnDanglingLifetime(const Token *tok, const ValueFlow::Value* val) +static std::string lifetimeType(const Token *tok, const ValueFlow::Value* val) { - const Token *vartok = val->tokvalue; - ErrorPath errorPath = val->errorPath; - std::string msg = ""; + std::string result; + if(!val) + return "object"; switch (val->lifetimeKind) { - case ValueFlow::Value::Object: - msg = "Returning object"; - break; case ValueFlow::Value::Lambda: - msg = "Returning lambda"; + result = "lambda"; break; case ValueFlow::Value::Iterator: - msg = "Returning iterator"; + result = "iterator"; + break; + case ValueFlow::Value::Object: + if(astIsPointer(tok)) + result = "pointer"; + else + result = "object"; break; } + return result; +} + +void CheckAutoVariables::errorReturnDanglingLifetime(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 = "Returning " + type; if (vartok) { errorPath.emplace_back(vartok, "Variable created here."); const Variable * var = vartok->variable(); if (var) { switch (val->lifetimeKind) { case ValueFlow::Value::Object: - msg += " that points to local variable"; + if(type == "pointer") + msg += " to local variable"; + else + msg += " that points to local variable"; break; case ValueFlow::Value::Lambda: msg += " that captures local variable"; @@ -726,27 +680,20 @@ void CheckAutoVariables::errorReturnDanglingLifetime(const Token *tok, const Val void CheckAutoVariables::errorInvalidLifetime(const Token *tok, const ValueFlow::Value* val) { - const Token *vartok = val->tokvalue; - ErrorPath errorPath = val->errorPath; - std::string msg = ""; - switch (val->lifetimeKind) { - case ValueFlow::Value::Object: - msg = "Using object"; - break; - case ValueFlow::Value::Lambda: - msg = "Using lambda"; - break; - case ValueFlow::Value::Iterator: - msg = "Using iterator"; - break; - } + 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: - msg += " that points to local variable"; + if(type == "pointer") + msg += " to local variable"; + else + msg += " that points to local variable"; break; case ValueFlow::Value::Lambda: msg += " that captures local variable"; diff --git a/lib/checkautovariables.h b/lib/checkautovariables.h index 63df288e8..7d043670a 100644 --- a/lib/checkautovariables.h +++ b/lib/checkautovariables.h @@ -59,7 +59,6 @@ public: void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) override { CheckAutoVariables checkAutoVariables(tokenizer, settings, errorLogger); checkAutoVariables.autoVariables(); - checkAutoVariables.returnPointerToLocalArray(); } /** assign function argument */ @@ -68,9 +67,6 @@ public: /** Check auto variables */ void autoVariables(); - /** Returning pointer to local array */ - void returnPointerToLocalArray(); - /** Returning reference to local/temporary variable */ void returnReference(); @@ -113,6 +109,8 @@ private: c.errorReturnAddressOfFunctionParameter(nullptr, "parameter"); c.errorUselessAssignmentArg(nullptr); c.errorUselessAssignmentPtrArg(nullptr); + c.errorReturnDanglingLifetime(nullptr, nullptr); + c.errorInvalidLifetime(nullptr, nullptr); } static std::string myName() { diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 310b3fc4a..7f3fbf07c 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -430,6 +430,16 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti return; } + if(value.isLifetimeValue()) { + if(value.lifetimeKind == ValueFlow::Value::Iterator && parent->isArithmeticalOp()) { + setTokenValue(parent,value,settings); + } + else if(astIsPointer(tok) && astIsPointer(parent) && (parent->isArithmeticalOp() || Token::Match(parent, "( %type%"))) { + setTokenValue(parent,value,settings); + } + return; + } + if (parent->str() == "(" && !parent->astOperand2() && Token::Match(parent,"( %name%")) { const ValueType &valueType = ValueType::parseDecl(parent->next(), settings); if (valueType.pointer) @@ -2950,6 +2960,8 @@ static const Variable * getLifetimeVariable(const Token * tok, ErrorPath& errorP for (const ValueFlow::Value& v:tok->values()) { if (!v.isLifetimeValue() && !v.tokvalue) 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) @@ -3071,7 +3083,9 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger if (!vartok) continue; const Variable * var = getLifetimeVariable(vartok, errorPath); - if (!(var && !var->isPointer())) + if(!var) + continue; + if(var->isPointer() && Token::Match(vartok->astParent(), "[|*")) continue; errorPath.emplace_back(tok, "Address of variable taken here."); @@ -3108,6 +3122,24 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger valueFlowForwardLifetime(tok->tokAt(3), tokenlist, errorLogger, settings); } + // Check variables + else if(tok->variable()) { + ErrorPath errorPath; + const Variable * var = getLifetimeVariable(tok, errorPath); + if(!var) + continue; + if(var->isArray() && tok->astParent() && (astIsPointer(tok->astParent()) || Token::Match(tok->astParent(), "%assign%|return"))) { + errorPath.emplace_back(tok, "Array decayed to pointer here."); + + ValueFlow::Value value; + value.valueType = ValueFlow::Value::LIFETIME; + value.tokvalue = var->nameToken(); + value.errorPath = errorPath; + setTokenValue(tok, value, tokenlist->getSettings()); + + valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + } + } } } diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index 1b7d9b2b5..28902fc12 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -52,7 +52,6 @@ private: // Check auto variables checkAutoVariables.autoVariables(); - checkAutoVariables.returnPointerToLocalArray(); } } @@ -474,7 +473,7 @@ private: " int num=2;" " return #" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Address of an auto-variable returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3] -> [test.cpp:3]: (error) Returning pointer to local variable 'num' that will be invalid when returning.\n", errout.str()); } void testautovar_return2() { @@ -486,7 +485,7 @@ private: " int num=2;" " return #" "}"); - ASSERT_EQUALS("[test.cpp:6]: (error) Address of an auto-variable returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:6] -> [test.cpp:6]: (error) Returning pointer to local variable 'num' that will be invalid when returning.\n", errout.str()); } void testautovar_return3() { @@ -707,7 +706,7 @@ private: " char str[100] = {0};\n" " return str;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Pointer to local array variable returned.\n", errout.str()); + 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()); check("char *foo()\n" // use ValueFlow "{\n" @@ -715,7 +714,7 @@ private: " char *p = str;\n" " return p;\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Pointer to local array variable returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [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()); check("class Fred {\n" " char *foo();\n" @@ -725,7 +724,7 @@ private: " char str[100] = {0};\n" " return str;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (error) Pointer to local array variable returned.\n", errout.str()); + 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()); check("char * format_reg(char *outbuffer_start) {\n" " return outbuffer_start;\n" @@ -764,7 +763,7 @@ private: " char q[] = \"AAAAAAAAAAAA\";\n" " return &q[1];\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Pointer to local array variable returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning pointer to local variable 'q' that will be invalid when returning.\n", errout.str()); check("char *foo()\n" "{\n" @@ -779,13 +778,13 @@ private: " char x[10] = {0};\n" " return x+5;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Pointer to local array variable returned.\n", errout.str()); + 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()); check("char *foo(int y) {\n" " char x[10] = {0};\n" " return (x+8)-y;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Pointer to local array variable returned.\n", errout.str()); + 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()); } void returnLocalVariable5() { // cast @@ -793,7 +792,7 @@ private: " int x[10] = {0};\n" " return (char *)x;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Pointer to local array variable returned.\n", errout.str()); + 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()); } void returnLocalVariable6() { // valueflow @@ -802,7 +801,7 @@ private: " int p = &x;\n" " return p;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Address of auto-variable 'x' returned\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'x' that will be invalid when returning.\n", errout.str()); } void returnReference1() { @@ -1177,14 +1176,14 @@ private: " return &y;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Address of function parameter 'y' returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:1] -> [test.cpp:3]: (error) Returning pointer to local variable 'y' that will be invalid when returning.\n", errout.str()); check("int ** foo(int * y)\n" "{\n" " return &y;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Address of function parameter 'y' returned.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:1] -> [test.cpp:3]: (error) Returning pointer to local variable 'y' that will be invalid when returning.\n", errout.str()); check("const int * foo(const int & y)\n" "{\n" @@ -1395,6 +1394,16 @@ private: "}\n"); ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4] -> [test.cpp:7]: (error) Using lambda that captures local variable 'b' that is out of scope.\n", errout.str()); + check("void f(bool b) {\n" + " int* x;\n" + " if(b) {\n" + " int y[6] = {0,1,2,3,4,5};\n" + " x = y;\n" + " }\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()); + check("void foo(int a) {\n" " std::function f;\n" " if (a > 0) {\n" @@ -1421,6 +1430,10 @@ private: " }\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + check("int &a[];\n" + "void b(){int *c = a};\n"); + ASSERT_EQUALS("", errout.str()); } };