Skip to content

Commit a22e476

Browse files
alonlib12alonlib12
authored
Fix CheckClass::operatorEqToSelf (danmar#3088)
1 parent 913dbeb commit a22e476

3 files changed

Lines changed: 237 additions & 13 deletions

File tree

‎lib/checkclass.cpp‎

Lines changed: 92 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1518,27 +1518,41 @@ void CheckClass::operatorEqToSelf()
15181518
if (Token::Match(func.retDef, "%type% &") && func.retDef->str() == scope->className) {
15191519
// find the parameter name
15201520
const Token *rhs = func.argumentList.begin()->nameToken();
1521-
1522-
if (!hasAssignSelf(&func, rhs)) {
1521+
const Token* out_ifStatementScopeStart = nullptr;
1522+
if (!hasAssignSelf(&func, rhs, &out_ifStatementScopeStart)) {
15231523
if (hasAllocation(&func, scope))
15241524
operatorEqToSelfError(func.token);
15251525
}
1526+
else if (out_ifStatementScopeStart != nullptr) {
1527+
if(hasAllocationInIfScope(&func, scope, out_ifStatementScopeStart))
1528+
operatorEqToSelfError(func.token);
1529+
}
15261530
}
15271531
}
15281532
}
15291533
}
15301534
}
15311535

1536+
bool CheckClass::hasAllocationInIfScope(const Function *func, const Scope* scope, const Token *ifStatementScopeStart) const
1537+
{
1538+
const Token *end;
1539+
if (ifStatementScopeStart->str() == "{")
1540+
end = ifStatementScopeStart->link();
1541+
else
1542+
end = func->functionScope->bodyEnd;
1543+
return hasAllocation(func, scope, ifStatementScopeStart, end);
1544+
}
1545+
15321546
bool CheckClass::hasAllocation(const Function *func, const Scope* scope) const
15331547
{
1534-
// This function is called when no simple check was found for assignment
1535-
// to self. We are currently looking for:
1536-
// - deallocate member ; ... member =
1537-
// - alloc member
1538-
// That is not ideal because it can cause false negatives but its currently
1539-
// necessary to prevent false positives.
1540-
const Token *last = func->functionScope->bodyEnd;
1541-
for (const Token *tok = func->functionScope->bodyStart; tok && (tok != last); tok = tok->next()) {
1548+
return hasAllocation(func, scope, func->functionScope->bodyStart, func->functionScope->bodyEnd);
1549+
}
1550+
1551+
bool CheckClass::hasAllocation(const Function *func, const Scope* scope, const Token *start, const Token *end) const
1552+
{
1553+
if (!end)
1554+
end = func->functionScope->bodyEnd;
1555+
for (const Token *tok = start; tok && (tok != end); tok = tok->next()) {
15421556
if (Token::Match(tok, "%var% = malloc|realloc|calloc|new") && isMemberVar(scope, tok))
15431557
return true;
15441558

@@ -1554,7 +1568,7 @@ bool CheckClass::hasAllocation(const Function *func, const Scope* scope) const
15541568
continue;
15551569
// Check for assignment to the deleted pointer (only if its a member of the class)
15561570
if (isMemberVar(scope, var)) {
1557-
for (const Token *tok1 = var->next(); tok1 && (tok1 != last); tok1 = tok1->next()) {
1571+
for (const Token *tok1 = var->next(); tok1 && (tok1 != end); tok1 = tok1->next()) {
15581572
if (Token::Match(tok1, "%varid% =", var->varId()))
15591573
return true;
15601574
}
@@ -1564,7 +1578,70 @@ bool CheckClass::hasAllocation(const Function *func, const Scope* scope) const
15641578
return false;
15651579
}
15661580

1567-
bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs)
1581+
static bool isTrueKeyword(const Token* tok)
1582+
{
1583+
return tok->hasKnownIntValue() && tok->getKnownIntValue() == 1;
1584+
}
1585+
1586+
static bool isFalseKeyword(const Token* tok)
1587+
{
1588+
return tok->hasKnownIntValue() && tok->getKnownIntValue() == 0;
1589+
}
1590+
1591+
/*
1592+
* Checks if self-assignment test is inverse
1593+
* For example 'if (this == &rhs)'
1594+
*/
1595+
CheckClass::Bool CheckClass::isInverted(const Token *tok, const Token *rhs)
1596+
{
1597+
bool res = true;
1598+
for (const Token *itr = tok; itr && itr->str()!="("; itr=itr->astParent()) {
1599+
if (Token::simpleMatch(itr, "!=") && (isTrueKeyword(itr->astOperand1()) || isTrueKeyword(itr->astOperand2()))) {
1600+
res = !res;
1601+
}
1602+
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()))
1603+
|| (Token::simpleMatch(itr->astOperand2(), "this") && Token::simpleMatch(itr->astOperand1(), "&") && Token::simpleMatch(itr->astOperand1()->next(), rhs->str().c_str(), rhs->str().size())))) {
1604+
res = !res;
1605+
}
1606+
else if (Token::simpleMatch(itr, "!=") && (isFalseKeyword(itr->astOperand1()) || isFalseKeyword(itr->astOperand2()))) {
1607+
//Do nothing
1608+
}
1609+
else if (Token::simpleMatch(itr, "!")) {
1610+
res = !res;
1611+
}
1612+
else if (Token::simpleMatch(itr, "==") && (isFalseKeyword(itr->astOperand1()) || isFalseKeyword(itr->astOperand2()))) {
1613+
res = !res;
1614+
}
1615+
else if (Token::simpleMatch(itr, "==") && (isTrueKeyword(itr->astOperand1()) || isTrueKeyword(itr->astOperand2()))) {
1616+
//Do nothing
1617+
}
1618+
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()))
1619+
|| (Token::simpleMatch(itr->astOperand2(), "this") && Token::simpleMatch(itr->astOperand1(), "&") && Token::simpleMatch(itr->astOperand1()->next(), rhs->str().c_str(), rhs->str().size())))) {
1620+
//Do nothing
1621+
}
1622+
else {
1623+
return Bool::BAILOUT;
1624+
}
1625+
}
1626+
if (res)
1627+
return Bool::TRUE;
1628+
return Bool::FALSE;
1629+
}
1630+
1631+
const Token * CheckClass::getIfStmtBodyStart(const Token *tok, const Token *rhs)
1632+
{
1633+
const Token *top = tok->astTop();
1634+
if (Token::simpleMatch(top->link(), ") {")) {
1635+
switch (isInverted(tok->astParent(), rhs)) {
1636+
case Bool::BAILOUT: return nullptr;
1637+
case Bool::TRUE: return top->link()->next();
1638+
case Bool::FALSE: return top->link()->next()->link();
1639+
}
1640+
}
1641+
return nullptr;
1642+
}
1643+
1644+
bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs, const Token **out_ifStatementScopeStart)
15681645
{
15691646
if (!rhs)
15701647
return false;
@@ -1586,6 +1663,9 @@ bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs)
15861663
return ChildrenToVisit::op1_and_op2;
15871664
if (tok2 && tok2->isUnaryOp("&") && tok2->astOperand1()->str() == rhs->str())
15881665
ret = true;
1666+
if (ret) {
1667+
*out_ifStatementScopeStart = getIfStmtBodyStart(tok2, rhs);
1668+
}
15891669
return ret ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2;
15901670
});
15911671
if (ret)

‎lib/checkclass.h‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,12 @@ class CPPCHECKLIB CheckClass : public Check {
263263

264264
// operatorEqToSelf helper functions
265265
bool hasAllocation(const Function *func, const Scope* scope) const;
266-
static bool hasAssignSelf(const Function *func, const Token *rhs);
266+
bool hasAllocation(const Function *func, const Scope* scope, const Token *start, const Token *end) const;
267+
bool hasAllocationInIfScope(const Function *func, const Scope* scope, const Token *ifStatementScopeStart) const;
268+
static bool hasAssignSelf(const Function *func, const Token *rhs, const Token **out_ifStatementScopeStart);
269+
enum class Bool { TRUE, FALSE, BAILOUT };
270+
static Bool isInverted(const Token *tok, const Token *rhs);
271+
static const Token * getIfStmtBodyStart(const Token *tok, const Token *rhs);
267272

268273
// checkConst helper functions
269274
bool isMemberVar(const Scope *scope, const Token *tok) const;

‎test/testclass.cpp‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1548,6 +1548,145 @@ class TestClass : public TestFixture {
15481548
"}");
15491549
ASSERT_EQUALS("", errout.str());
15501550

1551+
// this test needs an assignment test and has the inverse test
1552+
checkOpertorEqToSelf(
1553+
"class A\n"
1554+
"{\n"
1555+
"public:\n"
1556+
" char *s;\n"
1557+
" A & operator=(const A &);\n"
1558+
"};\n"
1559+
"A & A::operator=(const A &a)\n"
1560+
"{\n"
1561+
" if (&a == this)\n"
1562+
" {\n"
1563+
" free(s);\n"
1564+
" s = strdup(a.s);\n"
1565+
" }\n"
1566+
" return *this;\n"
1567+
"}");
1568+
ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str());
1569+
1570+
// this test needs an assignment test and has the inverse test
1571+
checkOpertorEqToSelf(
1572+
"class A\n"
1573+
"{\n"
1574+
"public:\n"
1575+
" char *s;\n"
1576+
" A & operator=(const A &);\n"
1577+
"};\n"
1578+
"A & A::operator=(const A &a)\n"
1579+
"{\n"
1580+
" if ((&a == this) == true)\n"
1581+
" {\n"
1582+
" free(s);\n"
1583+
" s = strdup(a.s);\n"
1584+
" }\n"
1585+
" return *this;\n"
1586+
"}");
1587+
ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str());
1588+
1589+
// this test needs an assignment test and has the inverse test
1590+
checkOpertorEqToSelf(
1591+
"class A\n"
1592+
"{\n"
1593+
"public:\n"
1594+
" char *s;\n"
1595+
" A & operator=(const A &);\n"
1596+
"};\n"
1597+
"A & A::operator=(const A &a)\n"
1598+
"{\n"
1599+
" if ((&a == this) != false)\n"
1600+
" {\n"
1601+
" free(s);\n"
1602+
" s = strdup(a.s);\n"
1603+
" }\n"
1604+
" return *this;\n"
1605+
"}");
1606+
ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str());
1607+
1608+
// this test needs an assignment test and has the inverse test
1609+
checkOpertorEqToSelf(
1610+
"class A\n"
1611+
"{\n"
1612+
"public:\n"
1613+
" char *s;\n"
1614+
" A & operator=(const A &);\n"
1615+
"};\n"
1616+
"A & A::operator=(const A &a)\n"
1617+
"{\n"
1618+
" if (!((&a == this) == false))\n"
1619+
" {\n"
1620+
" free(s);\n"
1621+
" s = strdup(a.s);\n"
1622+
" }\n"
1623+
" return *this;\n"
1624+
"}");
1625+
ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str());
1626+
1627+
// this test needs an assignment test and has the inverse test
1628+
checkOpertorEqToSelf(
1629+
"class A\n"
1630+
"{\n"
1631+
"public:\n"
1632+
" char *s;\n"
1633+
" A & operator=(const A &);\n"
1634+
"};\n"
1635+
"A & A::operator=(const A &a)\n"
1636+
"{\n"
1637+
" if ((&a != this) == false)\n"
1638+
" {\n"
1639+
" free(s);\n"
1640+
" s = strdup(a.s);\n"
1641+
" }\n"
1642+
" return *this;\n"
1643+
"}");
1644+
ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str());
1645+
1646+
// this test needs an assignment test and has the inverse test
1647+
checkOpertorEqToSelf(
1648+
"class A\n"
1649+
"{\n"
1650+
"public:\n"
1651+
" char *s;\n"
1652+
" A & operator=(const A &);\n"
1653+
"};\n"
1654+
"A & A::operator=(const A &a)\n"
1655+
"{\n"
1656+
" if (&a != this)\n"
1657+
" {\n"
1658+
" }\n"
1659+
" else\n"
1660+
" {\n"
1661+
" free(s);\n"
1662+
" s = strdup(a.s);\n"
1663+
" }\n"
1664+
" return *this;\n"
1665+
"}");
1666+
ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str());
1667+
1668+
// this test needs an assignment test and has the inverse test
1669+
checkOpertorEqToSelf(
1670+
"class A\n"
1671+
"{\n"
1672+
"public:\n"
1673+
" char *s;\n"
1674+
" A & operator=(const A &);\n"
1675+
"};\n"
1676+
"A & A::operator=(const A &a)\n"
1677+
"{\n"
1678+
" if (&a != this)\n"
1679+
" free(s);\n"
1680+
" else\n"
1681+
" {\n"
1682+
" free(s);\n"
1683+
" s = strdup(a.s);\n"
1684+
" }\n"
1685+
" return *this;\n"
1686+
"}");
1687+
ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str());
1688+
1689+
15511690
// this test needs an assignment test but doesn’t have it
15521691
checkOpertorEqToSelf(
15531692
"class A\n"

0 commit comments

Comments
 (0)