diff --git a/lib/checkother.cpp b/lib/checkother.cpp index ac4e34b37..413d406e2 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2836,3 +2836,69 @@ void CheckOther::constArgumentError(const Token *tok, const Token *ftok, const V const ErrorPath errorPath = getErrorPath(tok, value, errmsg); reportError(errorPath, Severity::style, "constArgument", errmsg, CWE570, false); } + +static ValueFlow::Value getLifetimeObjValue(const Token *tok) +{ + ValueFlow::Value result; + auto pred = [](const ValueFlow::Value &v) { + if (!v.isLocalLifetimeValue()) + return false; + if (!v.tokvalue->variable()) + return false; + return true; + }; + auto it = std::find_if(tok->values().begin(), tok->values().end(), pred); + if (it == tok->values().end()) + return result; + result = *it; + // There should only be one lifetime + if (std::find_if(std::next(it), tok->values().end(), pred) != tok->values().end()) + return result; + return result; +} + +void CheckOther::checkComparePointers() +{ + 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::Match(tok, "<|>|<=|>=")) + continue; + const Token *tok1 = tok->astOperand1(); + const Token *tok2 = tok->astOperand2(); + if (!astIsPointer(tok1) || !astIsPointer(tok2)) + continue; + ValueFlow::Value v1 = getLifetimeObjValue(tok1); + ValueFlow::Value v2 = getLifetimeObjValue(tok2); + if (!v1.isLocalLifetimeValue() || !v2.isLocalLifetimeValue()) + continue; + const Variable *var1 = v1.tokvalue->variable(); + const Variable *var2 = v2.tokvalue->variable(); + if (!var1 || !var2) + continue; + if (v1.tokvalue->varId() == v2.tokvalue->varId()) + continue; + if (var1->isReference() || var2->isReference()) + continue; + if (var1->isRValueReference() || var2->isRValueReference()) + continue; + comparePointersError(tok, &v1, &v2); + } + } +} + +void CheckOther::comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2) +{ + ErrorPath errorPath; + if (v1) { + errorPath.emplace_back(v1->tokvalue->variable()->nameToken(), "Variable declared here."); + errorPath.insert(errorPath.end(), v1->errorPath.begin(), v1->errorPath.end()); + } + if (v2) { + errorPath.emplace_back(v2->tokvalue->variable()->nameToken(), "Variable declared here."); + errorPath.insert(errorPath.end(), v2->errorPath.begin(), v2->errorPath.end()); + } + errorPath.emplace_back(tok, ""); + reportError( + errorPath, Severity::error, "comparePointers", "Comparing pointers that point to different objects", CWE570, false); +} diff --git a/lib/checkother.h b/lib/checkother.h index c6d52491d..136bebf7a 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -83,6 +83,7 @@ public: checkOther.checkFuncArgNamesDifferent(); checkOther.checkShadowVariables(); checkOther.checkConstArgument(); + checkOther.checkComparePointers(); checkOther.checkIncompleteStatement(); } @@ -214,6 +215,8 @@ public: void checkConstArgument(); + void checkComparePointers(); + private: // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, const bool result); @@ -267,6 +270,7 @@ private: 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 comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE { CheckOther c(nullptr, settings, errorLogger); @@ -331,6 +335,7 @@ private: c.shadowError(nullptr, nullptr, false); c.shadowError(nullptr, nullptr, true); c.constArgumentError(nullptr, nullptr, nullptr); + c.comparePointersError(nullptr, nullptr, nullptr); const std::vector nullvec; c.funcArgOrderDifferent("function", nullptr, nullptr, nullvec, nullvec); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 4be32e994..aac84e519 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2727,8 +2727,8 @@ const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPath &err if (!vartok) return tok; const Variable *tokvar = vartok->variable(); - if (!astIsContainer(vartok) && !(tokvar && tokvar->isArray()) && Token::Match(vartok->astParent(), "[|*|.") && - vartok->astParent()->originalName() != ".") { + if (!astIsContainer(vartok) && !(tokvar && tokvar->isArray()) && + (Token::Match(vartok->astParent(), "[|*") || vartok->astParent()->originalName() == "->")) { for (const ValueFlow::Value &v : vartok->values()) { if (!v.isLocalLifetimeValue()) continue; @@ -3090,6 +3090,8 @@ static bool isDecayedPointer(const Token *tok, const Settings *settings) return false; if (astIsPointer(tok->astParent()) && !Token::simpleMatch(tok->astParent(), "return")) return true; + if (Token::Match(tok->astParent(), "%cop%")) + return true; if (!Token::simpleMatch(tok->astParent(), "return")) return false; if (!tok->scope()) diff --git a/test/testother.cpp b/test/testother.cpp index ba56b2a93..49c2c1b44 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -223,6 +223,7 @@ private: TEST_CASE(shadowVariables); TEST_CASE(constArgument); + TEST_CASE(checkComparePointers); } void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, bool verbose=false, Settings* settings = 0) { @@ -7590,6 +7591,110 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); } + + void checkComparePointers() { + check("int f() {\n" + " int foo[1] = {0};\n" + " int bar[1] = {0};\n" + " int diff = 0;\n" + " if(foo > bar) {\n" + " diff = 1;\n" + " }\n" + " return diff;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:5] -> [test.cpp:3] -> [test.cpp:5] -> [test.cpp:5]: (error) Comparing pointers that point to different objects\n", + errout.str()); + + check("bool f() {\n" + " int x = 0;\n" + " int y = 0;\n" + " int* xp = &x;\n" + " int* yp = &y;\n" + " return xp > yp;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:4] -> [test.cpp:3] -> [test.cpp:5] -> [test.cpp:6]: (error) Comparing pointers that point to different objects\n", + errout.str()); + + check("bool f() {\n" + " int x = 0;\n" + " int y = 1;\n" + " return &x > &y;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:4] -> [test.cpp:3] -> [test.cpp:4] -> [test.cpp:4]: (error) Comparing pointers that point to different objects\n", + errout.str()); + + check("struct A {int data;};\n" + "bool f() {\n" + " A x;\n" + " A y;\n" + " int* xp = &x.data;\n" + " int* yp = &y.data;\n" + " return xp > yp;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:1] -> [test.cpp:5] -> [test.cpp:1] -> [test.cpp:6] -> [test.cpp:7]: (error) Comparing pointers that point to different objects\n", + errout.str()); + + check("struct A {int data;};\n" + "bool f(A ix, A iy) {\n" + " A* x = &ix;\n" + " A* y = &iy;\n" + " int* xp = &x->data;\n" + " int* yp = &y->data;\n" + " return xp > yp;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:3] -> [test.cpp:5] -> [test.cpp:2] -> [test.cpp:4] -> [test.cpp:6] -> [test.cpp:7]: (error) Comparing pointers that point to different objects\n", + errout.str()); + + check("bool f(int * xp, int* yp) {\n" + " return &xp > &yp;\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:1] -> [test.cpp:2] -> [test.cpp:1] -> [test.cpp:2] -> [test.cpp:2]: (error) Comparing pointers that point to different objects\n", + errout.str()); + + check("bool f() {\n" + " int x[2] = {1, 2}m;\n" + " int* xp = &x[0];\n" + " int* yp = &x[1];\n" + " return xp > yp;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("bool f(int * xp, int* yp) {\n" + " return xp > yp;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("bool f(int & x, int& y) {\n" + " return &x > &y;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("int& g();\n" + "bool f() {\n" + " int& x = g();\n" + " int& y = g();\n" + " int* xp = &x;\n" + " int* yp = &y;\n" + " return xp > yp;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct A {int data;};\n" + "bool f(A ix) {\n" + " A* x = &ix;\n" + " A* y = x;\n" + " int* xp = &x->data;\n" + " int* yp = &y->data;\n" + " return xp > yp;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)