diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index d0c464d49f5..3ccf758419a 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1505,27 +1505,41 @@ void CheckClass::operatorEqToSelf() if (Token::Match(func.retDef, "%type% &") && func.retDef->str() == scope->className) { // find the parameter name const Token *rhs = func.argumentList.begin()->nameToken(); - - if (!hasAssignSelf(&func, rhs)) { + const Token* out_ifStatementScopeStart = nullptr; + if (!hasAssignSelf(&func, rhs, &out_ifStatementScopeStart)) { if (hasAllocation(&func, scope)) operatorEqToSelfError(func.token); } + else if (out_ifStatementScopeStart != nullptr) { + if(hasAllocationInIfScope(&func, scope, out_ifStatementScopeStart)) + operatorEqToSelfError(func.token); + } } } } } } +bool CheckClass::hasAllocationInIfScope(const Function *func, const Scope* scope, const Token *ifStatementScopeStart) const +{ + const Token *end; + if (ifStatementScopeStart->str() == "{") + end = ifStatementScopeStart->link(); + else + end = func->functionScope->bodyEnd; + return hasAllocation(func, scope, ifStatementScopeStart, end); +} + bool CheckClass::hasAllocation(const Function *func, const Scope* scope) const { - // This function is called when no simple check was found for assignment - // to self. We are currently looking for: - // - deallocate member ; ... member = - // - alloc member - // That is not ideal because it can cause false negatives but its currently - // necessary to prevent false positives. - const Token *last = func->functionScope->bodyEnd; - for (const Token *tok = func->functionScope->bodyStart; tok && (tok != last); tok = tok->next()) { + return hasAllocation(func, scope, func->functionScope->bodyStart, func->functionScope->bodyEnd); +} + +bool CheckClass::hasAllocation(const Function *func, const Scope* scope, const Token *start, const Token *end) const +{ + if (!end) + end = func->functionScope->bodyEnd; + for (const Token *tok = start; tok && (tok != end); tok = tok->next()) { if (Token::Match(tok, "%var% = malloc|realloc|calloc|new") && isMemberVar(scope, tok)) return true; @@ -1541,7 +1555,7 @@ bool CheckClass::hasAllocation(const Function *func, const Scope* scope) const continue; // Check for assignment to the deleted pointer (only if its a member of the class) if (isMemberVar(scope, var)) { - for (const Token *tok1 = var->next(); tok1 && (tok1 != last); tok1 = tok1->next()) { + for (const Token *tok1 = var->next(); tok1 && (tok1 != end); tok1 = tok1->next()) { if (Token::Match(tok1, "%varid% =", var->varId())) return true; } @@ -1551,7 +1565,70 @@ bool CheckClass::hasAllocation(const Function *func, const Scope* scope) const return false; } -bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs) +static bool isTrueKeyword(const Token* tok) +{ + return tok->hasKnownIntValue() && tok->getKnownIntValue() == 1; +} + +static bool isFalseKeyword(const Token* tok) +{ + return tok->hasKnownIntValue() && tok->getKnownIntValue() == 0; +} + +/* + * Checks if self-assignment test is inverse + * For example 'if (this == &rhs)' + */ +CheckClass::Bool CheckClass::isInverted(const Token *tok, const Token *rhs) +{ + bool res = true; + for (const Token *itr = tok; itr && itr->str()!="("; itr=itr->astParent()) { + if (Token::simpleMatch(itr, "!=") && (isTrueKeyword(itr->astOperand1()) || isTrueKeyword(itr->astOperand2()))) { + res = !res; + } + else if (Token::simpleMatch(itr, "!=") && ( (Token::simpleMatch(itr->astOperand1(), "this") && Token::simpleMatch(itr->astOperand2(), "&") && Token::simpleMatch(itr->astOperand2()->next(), rhs->str().c_str(), rhs->str().size())) + || (Token::simpleMatch(itr->astOperand2(), "this") && Token::simpleMatch(itr->astOperand1(), "&") && Token::simpleMatch(itr->astOperand1()->next(), rhs->str().c_str(), rhs->str().size())))) { + res = !res; + } + else if (Token::simpleMatch(itr, "!=") && (isFalseKeyword(itr->astOperand1()) || isFalseKeyword(itr->astOperand2()))) { + //Do nothing + } + else if (Token::simpleMatch(itr, "!")) { + res = !res; + } + else if (Token::simpleMatch(itr, "==") && (isFalseKeyword(itr->astOperand1()) || isFalseKeyword(itr->astOperand2()))) { + res = !res; + } + else if (Token::simpleMatch(itr, "==") && (isTrueKeyword(itr->astOperand1()) || isTrueKeyword(itr->astOperand2()))) { + //Do nothing + } + else if (Token::simpleMatch(itr, "==") && ( (Token::simpleMatch(itr->astOperand1(), "this") && Token::simpleMatch(itr->astOperand2(), "&") && Token::simpleMatch(itr->astOperand2()->next(), rhs->str().c_str(), rhs->str().size())) + || (Token::simpleMatch(itr->astOperand2(), "this") && Token::simpleMatch(itr->astOperand1(), "&") && Token::simpleMatch(itr->astOperand1()->next(), rhs->str().c_str(), rhs->str().size())))) { + //Do nothing + } + else { + return Bool::BAILOUT; + } + } + if (res) + return Bool::TRUE; + return Bool::FALSE; +} + +const Token * CheckClass::getIfStmtBodyStart(const Token *tok, const Token *rhs) +{ + const Token *top = tok->astTop(); + if (Token::simpleMatch(top->link(), ") {")) { + switch (isInverted(tok->astParent(), rhs)) { + case Bool::BAILOUT: return nullptr; + case Bool::TRUE: return top->link()->next(); + case Bool::FALSE: return top->link()->next()->link(); + } + } + return nullptr; +} + +bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs, const Token **out_ifStatementScopeStart) { if (!rhs) return false; @@ -1573,6 +1650,9 @@ bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs) return ChildrenToVisit::op1_and_op2; if (tok2 && tok2->isUnaryOp("&") && tok2->astOperand1()->str() == rhs->str()) ret = true; + if (ret) { + *out_ifStatementScopeStart = getIfStmtBodyStart(tok2, rhs); + } return ret ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; }); if (ret) diff --git a/lib/checkclass.h b/lib/checkclass.h index cedd8f4fa16..0b1bb457c07 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -259,7 +259,12 @@ class CPPCHECKLIB CheckClass : public Check { // operatorEqToSelf helper functions bool hasAllocation(const Function *func, const Scope* scope) const; - static bool hasAssignSelf(const Function *func, const Token *rhs); + bool hasAllocation(const Function *func, const Scope* scope, const Token *start, const Token *end) const; + bool hasAllocationInIfScope(const Function *func, const Scope* scope, const Token *ifStatementScopeStart) const; + static bool hasAssignSelf(const Function *func, const Token *rhs, const Token **out_ifStatementScopeStart); + enum class Bool { TRUE, FALSE, BAILOUT }; + static Bool isInverted(const Token *tok, const Token *rhs); + static const Token * getIfStmtBodyStart(const Token *tok, const Token *rhs); // checkConst helper functions bool isMemberVar(const Scope *scope, const Token *tok) const; diff --git a/test/testclass.cpp b/test/testclass.cpp index 515525c5695..9f344c76d17 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -1534,6 +1534,145 @@ class TestClass : public TestFixture { "}"); ASSERT_EQUALS("", errout.str()); + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if (&a == this)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if ((&a == this) == true)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if ((&a == this) != false)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if (!((&a == this) == false))\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if ((&a != this) == false)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if (&a != this)\n" + " {\n" + " }\n" + " else\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if (&a != this)\n" + " free(s);\n" + " else\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test but doesn’t have it checkOpertorEqToSelf( "class A\n"