Skip to content

Commit e23038c

Browse files
Lena Herscheiddanmar
authored andcommitted
Fixed danmar#4775 (Check for assert() with side effects)
1 parent 67979f0 commit e23038c

13 files changed

+388
-106
lines changed

Makefile

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ MAN_SOURCE=man/cppcheck.1.xml
9393
###### Object Files
9494

9595
LIBOBJ = $(SRCDIR)/check64bit.o \
96+
$(SRCDIR)/checkassert.o \
9697
$(SRCDIR)/checkassignif.o \
9798
$(SRCDIR)/checkautovariables.o \
9899
$(SRCDIR)/checkbool.o \
@@ -138,6 +139,7 @@ CLIOBJ = cli/cmdlineparser.o \
138139

139140
TESTOBJ = test/options.o \
140141
test/test64bit.o \
142+
test/testassert.o \
141143
test/testassignif.o \
142144
test/testautovariables.o \
143145
test/testbool.o \
@@ -234,6 +236,9 @@ install: cppcheck
234236
$(SRCDIR)/check64bit.o: lib/check64bit.cpp lib/check64bit.h lib/config.h lib/check.h lib/token.h lib/tokenize.h lib/errorlogger.h lib/suppressions.h lib/tokenlist.h lib/settings.h lib/standards.h lib/symboldatabase.h lib/mathlib.h
235237
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o $(SRCDIR)/check64bit.o $(SRCDIR)/check64bit.cpp
236238

239+
$(SRCDIR)/checkassert.o: lib/checkassert.cpp lib/checkassert.h lib/config.h lib/check.h lib/token.h lib/tokenize.h lib/errorlogger.h lib/suppressions.h lib/tokenlist.h lib/settings.h lib/standards.h lib/symboldatabase.h lib/mathlib.h
240+
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o $(SRCDIR)/checkassert.o $(SRCDIR)/checkassert.cpp
241+
237242
$(SRCDIR)/checkassignif.o: lib/checkassignif.cpp lib/checkassignif.h lib/config.h lib/check.h lib/token.h lib/tokenize.h lib/errorlogger.h lib/suppressions.h lib/tokenlist.h lib/settings.h lib/standards.h lib/mathlib.h lib/symboldatabase.h
238243
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o $(SRCDIR)/checkassignif.o $(SRCDIR)/checkassignif.cpp
239244

@@ -363,6 +368,9 @@ test/options.o: test/options.cpp test/options.h
363368
test/test64bit.o: test/test64bit.cpp lib/tokenize.h lib/errorlogger.h lib/config.h lib/suppressions.h lib/tokenlist.h lib/check64bit.h lib/check.h lib/token.h lib/settings.h lib/standards.h test/testsuite.h test/redirect.h
364369
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/test64bit.o test/test64bit.cpp
365370

371+
test/testassert.o: test/testassert.cpp lib/tokenize.h lib/errorlogger.h lib/config.h lib/suppressions.h lib/tokenlist.h lib/checkassert.h lib/check.h lib/token.h lib/settings.h lib/standards.h test/testsuite.h test/redirect.h
372+
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testassert.o test/testassert.cpp
373+
366374
test/testassignif.o: test/testassignif.cpp lib/tokenize.h lib/errorlogger.h lib/config.h lib/suppressions.h lib/tokenlist.h lib/checkassignif.h lib/check.h lib/token.h lib/settings.h lib/standards.h lib/mathlib.h test/testsuite.h test/redirect.h
367375
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testassignif.o test/testassignif.cpp
368376

lib/checkassert.cpp

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
/*
2+
* Cppcheck - A tool for static C/C++ code analysis
3+
* Copyright (C) 2007-2013 Daniel Marjamäki and Cppcheck team.
4+
*
5+
* This program is free software: you can redistribute it and/or modify
6+
* it under the terms of the GNU General Public License as published by
7+
* the Free Software Foundation, either version 3 of the License, or
8+
* (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13+
* GNU General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU General Public License
16+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
17+
*/
18+
19+
//---------------------------------------------------------------------------
20+
// You should not write statements with side effects in assert()
21+
//---------------------------------------------------------------------------
22+
23+
#include "checkassert.h"
24+
#include "symboldatabase.h"
25+
26+
//---------------------------------------------------------------------------
27+
28+
29+
// Register this check class (by creating a static instance of it)
30+
namespace {
31+
CheckAssert instance;
32+
}
33+
34+
void CheckAssert::assertWithSideEffects()
35+
{
36+
if (!_settings->isEnabled("warning"))
37+
return;
38+
39+
const Token *tok = findAssertPattern(_tokenizer->tokens());
40+
const Token *endTok = tok ? tok->next()->link() : NULL;
41+
42+
while (tok && endTok) {
43+
for (const Token* tmp = tok->tokAt(1); tmp != endTok; tmp = tmp->next()) {
44+
checkVariableAssignment(tmp, true);
45+
46+
if (tmp->isName() && tmp->type() == Token::eFunction) {
47+
const Function* f = tmp->function();
48+
if (f->argCount() == 0 && f->isConst) continue;
49+
50+
// functions with non-const references
51+
else if (f->argCount() != 0) {
52+
for (std::list<Variable>::const_iterator it = f->argumentList.begin(); it != f->argumentList.end(); ++it) {
53+
if (it->isConst() || it->isLocal()) continue;
54+
else if (it->isReference()) {
55+
const Token* next = it->nameToken()->next();
56+
bool isAssigned = checkVariableAssignment(next, false);
57+
if (isAssigned)
58+
sideEffectInAssertError(tmp, f->name());
59+
continue;
60+
}
61+
}
62+
}
63+
64+
// variables in function scope
65+
const Scope* scope = f->functionScope;
66+
if (!scope) continue;
67+
68+
for (const Token *tok2 = scope->classStart; tok2 != scope->classEnd; tok2 = tok2->next()) {
69+
if (tok2 && tok2->type() != Token::eAssignmentOp && tok2->type() != Token::eIncDecOp) continue;
70+
const Variable* var = tok2->previous()->variable();
71+
if (!var) continue;
72+
73+
std::vector<const Token*> returnTokens; // find all return statements
74+
for (const Token *rt = scope->classStart; rt != scope->classEnd; rt = rt->next()) {
75+
if (!Token::Match(rt, "return %any%")) continue;
76+
returnTokens.push_back(rt);
77+
}
78+
79+
bool noReturnInScope = true;
80+
for (std::vector<const Token*>::iterator rt = returnTokens.begin(); rt != returnTokens.end(); ++rt) {
81+
noReturnInScope &= !inSameScope(*rt, tok2);
82+
}
83+
if (noReturnInScope) continue;
84+
bool isAssigned = checkVariableAssignment(tok2, false);
85+
if (isAssigned)
86+
sideEffectInAssertError(tmp, f->name());
87+
}
88+
}
89+
}
90+
91+
tok = findAssertPattern(endTok->next());
92+
endTok = tok ? tok->next()->link() : NULL;
93+
}
94+
}
95+
//---------------------------------------------------------------------------
96+
97+
98+
void CheckAssert::sideEffectInAssertError(const Token *tok, const std::string& functionName)
99+
{
100+
reportError(tok, Severity::warning,
101+
"assertWithSideEffect", "Assert statement calls a function which may have desired side effects: '" + functionName + "'.\n"
102+
"Non-pure function: '" + functionName + "' is called inside assert statement. "
103+
"Assert statements are removed from release builds so the code inside "
104+
"assert statement is not executed. If the code is needed also in release "
105+
"builds, this is a bug.");
106+
}
107+
108+
void CheckAssert::assignmentInAssertError(const Token *tok, const std::string& varname)
109+
{
110+
reportError(tok, Severity::warning,
111+
"assignmentInAssert", "Assert statement modifies '" + varname + "'.\n"
112+
"Variable '" + varname + "' is modified insert assert statement. "
113+
"Assert statements are removed from release builds so the code inside "
114+
"assert statement is not executed. If the code is needed also in release "
115+
"builds, this is a bug.");
116+
}
117+
118+
// checks if side effects happen on the variable prior to tmp
119+
bool CheckAssert::checkVariableAssignment(const Token* assignTok, bool reportError /*= true*/)
120+
{
121+
const Variable* v = assignTok->previous()->variable();
122+
if (!v) return false;
123+
124+
// assignment
125+
if (assignTok->isAssignmentOp() || assignTok->type() == Token::eIncDecOp) {
126+
127+
if (v->isConst()) return false;
128+
129+
if (reportError) // report as variable assignment error
130+
assignmentInAssertError(assignTok, v->name());
131+
132+
return true;
133+
}
134+
return false;
135+
// TODO: function calls on v
136+
}
137+
138+
const Token* CheckAssert::findAssertPattern(const Token* start)
139+
{
140+
return Token::findmatch(start, "assert ( %any%");
141+
}
142+
143+
bool CheckAssert::inSameScope(const Token* returnTok, const Token* assignTok)
144+
{
145+
// TODO: even if a return is in the same scope, the assignment might not affect it.
146+
return returnTok->scope() == assignTok->scope();
147+
}

lib/checkassert.h

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
/*
2+
* Cppcheck - A tool for static C/C++ code analysis
3+
* Copyright (C) 2007-2013 Daniel Marjamäki and Cppcheck team.
4+
*
5+
* This program is free software: you can redistribute it and/or modify
6+
* it under the terms of the GNU General Public License as published by
7+
* the Free Software Foundation, either version 3 of the License, or
8+
* (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13+
* GNU General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU General Public License
16+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
17+
*/
18+
19+
20+
//---------------------------------------------------------------------------
21+
#ifndef CheckAssertH
22+
#define CheckAssertH
23+
//---------------------------------------------------------------------------
24+
25+
#include "config.h"
26+
#include "check.h"
27+
28+
/// @addtogroup Checks
29+
/// @{
30+
31+
/**
32+
* @brief Checking for side effects in assert statements
33+
*/
34+
35+
class CPPCHECKLIB CheckAssert : public Check {
36+
public:
37+
CheckAssert() : Check(myName())
38+
{}
39+
40+
CheckAssert(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger)
41+
: Check(myName(), tokenizer, settings, errorLogger)
42+
{}
43+
44+
virtual void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) {
45+
CheckAssert check(tokenizer, settings, errorLogger);
46+
check.assertWithSideEffects();
47+
}
48+
49+
void assertWithSideEffects();
50+
51+
protected:
52+
bool checkVariableAssignment(const Token* tmp, bool reportError = true);
53+
bool inSameScope(const Token* returnTok, const Token* assignTok);
54+
55+
static const Token* findAssertPattern(const Token *start);
56+
57+
private:
58+
void sideEffectInAssertError(const Token *tok, const std::string& functionName);
59+
void assignmentInAssertError(const Token *tok, const std::string &varname);
60+
61+
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
62+
CheckAssert c(0, settings, errorLogger);
63+
c.assertWithSideEffects();
64+
}
65+
66+
static std::string myName() {
67+
return "Side Effects in asserts";
68+
}
69+
70+
std::string classInfo() const {
71+
return "Warn if side effects in assert statements \n";
72+
}
73+
};
74+
/// @}
75+
//---------------------------------------------------------------------------
76+
#endif

lib/checkother.cpp

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1170,46 +1170,6 @@ void CheckOther::selfAssignmentError(const Token *tok, const std::string &varnam
11701170
"selfAssignment", "Redundant assignment of '" + varname + "' to itself.");
11711171
}
11721172

1173-
//---------------------------------------------------------------------------
1174-
// int a = 1;
1175-
// assert(a = 2); // <- assert should not have a side-effect
1176-
//---------------------------------------------------------------------------
1177-
static inline const Token *findAssertPattern(const Token *start)
1178-
{
1179-
return Token::findmatch(start, "assert ( %any%");
1180-
}
1181-
1182-
void CheckOther::checkAssignmentInAssert()
1183-
{
1184-
if (!_settings->isEnabled("warning"))
1185-
return;
1186-
1187-
const Token *tok = findAssertPattern(_tokenizer->tokens());
1188-
const Token *endTok = tok ? tok->next()->link() : NULL;
1189-
1190-
while (tok && endTok) {
1191-
for (tok = tok->tokAt(2); tok != endTok; tok = tok->next()) {
1192-
if (tok->isName() && (tok->next()->isAssignmentOp() || tok->next()->type() == Token::eIncDecOp))
1193-
assignmentInAssertError(tok, tok->str());
1194-
else if (Token::Match(tok, "--|++ %var%"))
1195-
assignmentInAssertError(tok, tok->strAt(1));
1196-
}
1197-
1198-
tok = findAssertPattern(endTok->next());
1199-
endTok = tok ? tok->next()->link() : NULL;
1200-
}
1201-
}
1202-
1203-
void CheckOther::assignmentInAssertError(const Token *tok, const std::string &varname)
1204-
{
1205-
reportError(tok, Severity::warning,
1206-
"assignmentInAssert", "Assert statement modifies '" + varname + "'.\n"
1207-
"Variable '" + varname + "' is modified insert assert statement. "
1208-
"Assert statements are removed from release builds so the code inside "
1209-
"assert statement is not executed. If the code is needed also in release "
1210-
"builds, this is a bug.");
1211-
}
1212-
12131173
//---------------------------------------------------------------------------
12141174
// if ((x != 1) || (x != 3)) // expression always true
12151175
// if ((x == 1) && (x == 3)) // expression always false

lib/checkother.h

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,6 @@ class CPPCHECKLIB CheckOther : public Check {
6060
checkOther.checkRedundantAssignment();
6161
checkOther.checkRedundantAssignmentInSwitch();
6262
checkOther.checkSuspiciousCaseInSwitch();
63-
checkOther.checkAssignmentInAssert();
6463
checkOther.checkSelfAssignment();
6564
checkOther.checkDuplicateIf();
6665
checkOther.checkDuplicateBranch();
@@ -186,9 +185,6 @@ class CPPCHECKLIB CheckOther : public Check {
186185
/** @brief %Check for assigning a variable to itself*/
187186
void checkSelfAssignment();
188187

189-
/** @brief %Check for assignment to a variable in an assert test*/
190-
void checkAssignmentInAssert();
191-
192188
/** @brief %Check for testing for mutual exclusion over ||*/
193189
void checkIncorrectLogicOperator();
194190

@@ -293,7 +289,6 @@ class CPPCHECKLIB CheckOther : public Check {
293289
void suspiciousCaseInSwitchError(const Token* tok, const std::string& operatorString);
294290
void suspiciousEqualityComparisonError(const Token* tok);
295291
void selfAssignmentError(const Token *tok, const std::string &varname);
296-
void assignmentInAssertError(const Token *tok, const std::string &varname);
297292
void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always);
298293
void redundantConditionError(const Token *tok, const std::string &text);
299294
void misusedScopeObjectError(const Token *tok, const std::string &varname);
@@ -357,7 +352,6 @@ class CPPCHECKLIB CheckOther : public Check {
357352
c.suspiciousCaseInSwitchError(0, "||");
358353
c.suspiciousEqualityComparisonError(0);
359354
c.selfAssignmentError(0, "varname");
360-
c.assignmentInAssertError(0, "varname");
361355
c.incorrectLogicOperatorError(0, "foo > 3 && foo < 4", true);
362356
c.redundantConditionError(0, "If x > 10 the condition x > 11 is always true.");
363357
c.memsetZeroBytesError(0, "varname");

lib/cppcheck.vcxproj

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
</ItemGroup>
3737
<ItemGroup>
3838
<ClCompile Include="check64bit.cpp" />
39+
<ClCompile Include="checkassert.cpp" />
3940
<ClCompile Include="checkassignif.cpp" />
4041
<ClCompile Include="checkautovariables.cpp" />
4142
<ClCompile Include="checkbool.cpp" />
@@ -75,6 +76,7 @@
7576
<ItemGroup>
7677
<ClInclude Include="check.h" />
7778
<ClInclude Include="check64bit.h" />
79+
<ClInclude Include="checkassert.h" />
7880
<ClInclude Include="checkassignif.h" />
7981
<ClInclude Include="checkautovariables.h" />
8082
<ClInclude Include="checkbool.h" />

lib/cppcheck.vcxproj.filters

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,9 @@
119119
<ClCompile Include="checksizeof.cpp">
120120
<Filter>Source Files</Filter>
121121
</ClCompile>
122+
<ClCompile Include="checkassert.cpp">
123+
<Filter>Source Files</Filter>
124+
</ClCompile>
122125
</ItemGroup>
123126
<ItemGroup>
124127
<ClInclude Include="checkbufferoverrun.h">
@@ -238,6 +241,9 @@
238241
<ClInclude Include="checksizeof.h">
239242
<Filter>Header Files</Filter>
240243
</ClInclude>
244+
<ClInclude Include="checkassert.h">
245+
<Filter>Header Files</Filter>
246+
</ClInclude>
241247
</ItemGroup>
242248
<ItemGroup>
243249
<ResourceCompile Include="version.rc" />

lib/lib.pri

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ include($$PWD/pcrerules.pri)
44
INCLUDEPATH += ../externals/tinyxml
55
HEADERS += $${BASEPATH}check.h \
66
$${BASEPATH}check64bit.h \
7+
$${BASEPATH}checkassert.h \
78
$${BASEPATH}checkassignif.h \
89
$${BASEPATH}checkautovariables.h \
910
$${BASEPATH}checkbool.h \
@@ -42,6 +43,7 @@ HEADERS += $${BASEPATH}check.h \
4243

4344

4445
SOURCES += $${BASEPATH}check64bit.cpp \
46+
$${BASEPATH}checkassert.cpp \
4547
$${BASEPATH}checkassignif.cpp \
4648
$${BASEPATH}checkautovariables.cpp \
4749
$${BASEPATH}checkbool.cpp \

0 commit comments

Comments
 (0)