Skip to content

Fix CheckClass::operatorEqToSelf #3088

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danmar merged 5 commits into from
Feb 2, 2021
Merged

Fix CheckClass::operatorEqToSelf #3088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d369f5
Fix CheckClass::operatorEqToSelf
alonlib12 Jan 26, 2021
ef20a74
Fix 'hasAllocation' function declaration
alonlib12 Jan 27, 2021
f62afd8
Fix tests asserts
alonlib12 Jan 27, 2021
cd78a0a
Fix code review rejections
alonlib12 Jan 30, 2021
7e42a71
Fix code review rejections.
alonlib12 Jan 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 92 additions & 12 deletions lib/checkclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
}
Expand All @@ -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;
Expand All @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion lib/checkclass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
139 changes: 139 additions & 0 deletions test/testclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down