From 9b973e652c804bdf8db7580577080a9ece4e603c Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Mon, 17 Dec 2018 06:04:24 +0100 Subject: [PATCH] Issue 8830: New check: Function argument evaluates to constant value Add a check for function arguments that can be constant: ```cpp extern void bar(int); void f(int x) { bar((x & 0x01) >> 7); // function 'bar' is always called with a '0'-argument } ``` --- lib/astutils.cpp | 27 +++++++++++++++++++++ lib/astutils.h | 2 ++ lib/checkcondition.cpp | 20 ---------------- lib/checkother.cpp | 39 ++++++++++++++++++++++++++++++ lib/checkother.h | 6 ++++- test/testother.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 127 insertions(+), 21 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index ecd372d7d..c8831f743 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1040,6 +1040,33 @@ bool isLikelyStreamRead(bool cpp, const Token *op) return (!parent->astOperand1()->valueType() || !parent->astOperand1()->valueType()->isIntegral()); } +bool isConstVarExpression(const Token *tok) +{ + if (!tok) + return false; + if (Token::Match(tok->previous(), "sizeof (")) + return true; + if (Token::Match(tok->previous(), "%name% (")) { + std::vector args = getArguments(tok); + return std::all_of(args.begin(), args.end(), &isConstVarExpression); + } + if (Token::Match(tok, "( %type%")) + return isConstVarExpression(tok->astOperand1()); + if (Token::Match(tok, "%cop%")) { + if (tok->astOperand1() && !isConstVarExpression(tok->astOperand1())) + return false; + if (tok->astOperand2() && !isConstVarExpression(tok->astOperand2())) + return false; + return true; + } + if (Token::Match(tok, "%bool%|%num%|%str%|%char%|nullptr|NULL")) + return true; + if (tok->isEnumerator()) + return true; + if (tok->variable()) + return tok->variable()->isConst(); + return false; +} static bool nonLocal(const Variable* var) { diff --git a/lib/astutils.h b/lib/astutils.h index c711f5d69..eb225c560 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -160,6 +160,8 @@ const Token *findLambdaEndToken(const Token *first); */ bool isLikelyStreamRead(bool cpp, const Token *op); +bool isConstVarExpression(const Token *tok); + class FwdAnalysis { public: FwdAnalysis(bool cpp, const Library &library) : mCpp(cpp), mLibrary(library), mWhat(What::Reassign) {} diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 581146a80..e17d7d5c9 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1238,26 +1238,6 @@ void CheckCondition::clarifyConditionError(const Token *tok, bool assign, bool b errmsg, CWE398, false); } -static bool isConstVarExpression(const Token * tok) -{ - if (!tok) - return false; - if (Token::Match(tok, "%cop%")) { - if (tok->astOperand1() && !isConstVarExpression(tok->astOperand1())) - return false; - if (tok->astOperand2() && !isConstVarExpression(tok->astOperand2())) - return false; - return true; - } - if (Token::Match(tok, "%bool%|%num%|%str%|%char%|nullptr|NULL")) - return true; - if (tok->isEnumerator()) - return true; - if (tok->variable()) - return tok->variable()->isConst(); - return false; -} - void CheckCondition::alwaysTrueFalse() { if (!mSettings->isEnabled(Settings::STYLE)) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index afced8552..9edf9dcfd 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2798,3 +2798,42 @@ void CheckOther::shadowError(const Token *var, const Token *shadowed, bool shado std::string message = "$symbol:" + varname + "\nLocal variable $symbol shadows outer " + (shadowVar ? "variable" : "function"); reportError(errorPath, Severity::style, id, message, CWE398, false); } + +void CheckOther::checkConstArgument() +{ + if (!mSettings->isEnabled(Settings::STYLE)) + return; + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope *functionScope : symbolDatabase->functionScopes) { + for (const Token *tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { + if (!Token::simpleMatch(tok->astParent(), "(")) + continue; + if (!Token::Match(tok->astParent()->previous(), "%name%")) + continue; + if (Token::Match(tok->astParent()->previous(), "if|while|switch|sizeof")) + continue; + if (tok == tok->astParent()->previous()) + continue; + if (!tok->hasKnownIntValue()) + continue; + if (Token::Match(tok, "%var%")) + continue; + if (Token::Match(tok, "++|--")) + continue; + if (isConstVarExpression(tok)) + continue; + constArgumentError(tok, tok->astParent()->previous(), &tok->values().front()); + } + } +} + +void CheckOther::constArgumentError(const Token *tok, const Token *ftok, const ValueFlow::Value *value) +{ + MathLib::bigint intvalue = value ? value->intvalue : 0; + const std::string expr = tok ? tok->expressionString() : std::string("x"); + const std::string fun = ftok ? ftok->str() : std::string("f"); + + const std::string errmsg = "Argument '" + expr + "' to function " + fun + " is always " + std::to_string(intvalue); + const ErrorPath errorPath = getErrorPath(tok, value, errmsg); + reportError(errorPath, Severity::style, "constArgument", errmsg, CWE570, false); +} diff --git a/lib/checkother.h b/lib/checkother.h index f4600f78c..64c23da53 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -82,6 +82,7 @@ public: checkOther.checkEvaluationOrder(); checkOther.checkFuncArgNamesDifferent(); checkOther.checkShadowVariables(); + checkOther.checkConstArgument(); } /** @brief Run checks against the simplified token list */ @@ -215,8 +216,9 @@ public: /** @brief %Check for shadow variables. Less noisy than gcc/clang -Wshadow. */ void checkShadowVariables(); -private: + void checkConstArgument(); +private: // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, const bool result); void checkCastIntToCharAndBackError(const Token *tok, const std::string &strFunctionName); @@ -269,6 +271,7 @@ private: void funcArgNamesDifferent(const std::string & functionName, size_t index, const Token* declaration, const Token* definition); void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector & declarations, const std::vector & definitions); void shadowError(const Token *var, const Token *shadowed, bool shadowVar); + void constArgumentError(const Token *tok, const Token *ftok, const ValueFlow::Value *value); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override { CheckOther c(nullptr, settings, errorLogger); @@ -333,6 +336,7 @@ private: c.redundantBitwiseOperationInSwitchError(nullptr, "varname"); c.shadowError(nullptr, nullptr, false); c.shadowError(nullptr, nullptr, true); + c.constArgumentError(nullptr, nullptr, nullptr); const std::vector nullvec; c.funcArgOrderDifferent("function", nullptr, nullptr, nullvec, nullvec); diff --git a/test/testother.cpp b/test/testother.cpp index fe4028c47..02639e870 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -221,6 +221,7 @@ private: TEST_CASE(cpp11FunctionArgInit); // #7846 - "void foo(int declaration = {}) {" TEST_CASE(shadowVariables); + TEST_CASE(constArgument); } void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) { @@ -7523,6 +7524,59 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); } + + void constArgument() { + check("void g(int);\n" + "void f(int x) {\n" + " g((x & 0x01) >> 7);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Argument '(x&1)>>7' to function g is always 0\n", errout.str()); + + check("void g(int);\n" + "void f(int x) {\n" + " g((int)((x & 0x01) >> 7));\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Argument '(int)((x&1)>>7)' to function g is always 0\n", errout.str()); + + check("void g(int);\n" + "void f(int x) {\n" + " g(0);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void g(int);\n" + "void h() { return 1; }\n" + "void f(int x) {\n" + " g(h());\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void g(int);\n" + "void f(int x) {\n" + " g(std::strlen(\"a\"));\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void g(int);\n" + "void f(int x) {\n" + " g((int)0);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void g(int);\n" + "void f(int x) {\n" + " x = 0;\n" + " g(x);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void g(int);\n" + "void f() {\n" + " const int x = 0;\n" + " g(x + 1);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)