walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Fixed #4775 (Check for assert() with side effects)

This commit is contained in:
Lena Herscheid 2013-05-07 21:35:16 +02:00 committed by Daniel Marjamäki
parent 67979f00be
commit e23038c4de
13 changed files with 388 additions and 106 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
Makefile
View File

@ -93,6 +93,7 @@ MAN_SOURCE=man/cppcheck.1.xml
###### Object Files
LIBOBJ = $(SRCDIR)/check64bit.o \
$(SRCDIR)/checkassert.o \
$(SRCDIR)/checkassignif.o \
$(SRCDIR)/checkautovariables.o \
$(SRCDIR)/checkbool.o \
@ -138,6 +139,7 @@ CLIOBJ = cli/cmdlineparser.o \
TESTOBJ = test/options.o \
test/test64bit.o \
test/testassert.o \
test/testassignif.o \
test/testautovariables.o \
test/testbool.o \
@ -234,6 +236,9 @@ install: cppcheck
$(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
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o $(SRCDIR)/check64bit.o $(SRCDIR)/check64bit.cpp
$(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
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o $(SRCDIR)/checkassert.o $(SRCDIR)/checkassert.cpp
$(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
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o $(SRCDIR)/checkassignif.o $(SRCDIR)/checkassignif.cpp
@ -363,6 +368,9 @@ test/options.o: test/options.cpp test/options.h
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
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/test64bit.o test/test64bit.cpp
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
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testassert.o test/testassert.cpp
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
$(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testassignif.o test/testassignif.cpp

147
lib/checkassert.cpp Normal file
View File

@ -0,0 +1,147 @@
/*
* Cppcheck - A tool for static C/C++ code analysis
* Copyright (C) 2007-2013 Daniel Marjamäki and Cppcheck team.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
//---------------------------------------------------------------------------
// You should not write statements with side effects in assert()
//---------------------------------------------------------------------------
#include "checkassert.h"
#include "symboldatabase.h"
//---------------------------------------------------------------------------
// Register this check class (by creating a static instance of it)
namespace {
CheckAssert instance;
}
void CheckAssert::assertWithSideEffects()
{
if (!_settings->isEnabled("warning"))
return;
const Token *tok = findAssertPattern(_tokenizer->tokens());
const Token *endTok = tok ? tok->next()->link() : NULL;
while (tok && endTok) {
for (const Token* tmp = tok->tokAt(1); tmp != endTok; tmp = tmp->next()) {
checkVariableAssignment(tmp, true);
if (tmp->isName() && tmp->type() == Token::eFunction) {
const Function* f = tmp->function();
if (f->argCount() == 0 && f->isConst) continue;
// functions with non-const references
else if (f->argCount() != 0) {
for (std::list<Variable>::const_iterator it = f->argumentList.begin(); it != f->argumentList.end(); ++it) {
if (it->isConst() || it->isLocal()) continue;
else if (it->isReference()) {
const Token* next = it->nameToken()->next();
bool isAssigned = checkVariableAssignment(next, false);
if (isAssigned)
sideEffectInAssertError(tmp, f->name());
continue;
}
}
}
// variables in function scope
const Scope* scope = f->functionScope;
if (!scope) continue;
for (const Token *tok2 = scope->classStart; tok2 != scope->classEnd; tok2 = tok2->next()) {
if (tok2 && tok2->type() != Token::eAssignmentOp && tok2->type() != Token::eIncDecOp) continue;
const Variable* var = tok2->previous()->variable();
if (!var) continue;
std::vector<const Token*> returnTokens; // find all return statements
for (const Token *rt = scope->classStart; rt != scope->classEnd; rt = rt->next()) {
if (!Token::Match(rt, "return %any%")) continue;
returnTokens.push_back(rt);
}
bool noReturnInScope = true;
for (std::vector<const Token*>::iterator rt = returnTokens.begin(); rt != returnTokens.end(); ++rt) {
noReturnInScope &= !inSameScope(*rt, tok2);
}
if (noReturnInScope) continue;
bool isAssigned = checkVariableAssignment(tok2, false);
if (isAssigned)
sideEffectInAssertError(tmp, f->name());
}
}
}
tok = findAssertPattern(endTok->next());
endTok = tok ? tok->next()->link() : NULL;
}
}
//---------------------------------------------------------------------------
void CheckAssert::sideEffectInAssertError(const Token *tok, const std::string& functionName)
{
reportError(tok, Severity::warning,
"assertWithSideEffect", "Assert statement calls a function which may have desired side effects: '" + functionName + "'.\n"
"Non-pure function: '" + functionName + "' is called inside assert statement. "
"Assert statements are removed from release builds so the code inside "
"assert statement is not executed. If the code is needed also in release "
"builds, this is a bug.");
}
void CheckAssert::assignmentInAssertError(const Token *tok, const std::string& varname)
{
reportError(tok, Severity::warning,
"assignmentInAssert", "Assert statement modifies '" + varname + "'.\n"
"Variable '" + varname + "' is modified insert assert statement. "
"Assert statements are removed from release builds so the code inside "
"assert statement is not executed. If the code is needed also in release "
"builds, this is a bug.");
}
// checks if side effects happen on the variable prior to tmp
bool CheckAssert::checkVariableAssignment(const Token* assignTok, bool reportError /*= true*/)
{
const Variable* v = assignTok->previous()->variable();
if (!v) return false;
// assignment
if (assignTok->isAssignmentOp() || assignTok->type() == Token::eIncDecOp) {
if (v->isConst()) return false;
if (reportError) // report as variable assignment error
assignmentInAssertError(assignTok, v->name());
return true;
}
return false;
// TODO: function calls on v
}
const Token* CheckAssert::findAssertPattern(const Token* start)
{
return Token::findmatch(start, "assert ( %any%");
}
bool CheckAssert::inSameScope(const Token* returnTok, const Token* assignTok)
{
// TODO: even if a return is in the same scope, the assignment might not affect it.
return returnTok->scope() == assignTok->scope();
}

76
lib/checkassert.h Normal file
View File

@ -0,0 +1,76 @@
/*
* Cppcheck - A tool for static C/C++ code analysis
* Copyright (C) 2007-2013 Daniel Marjamäki and Cppcheck team.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
//---------------------------------------------------------------------------
#ifndef CheckAssertH
#define CheckAssertH
//---------------------------------------------------------------------------
#include "config.h"
#include "check.h"
/// @addtogroup Checks
/// @{
/**
* @brief Checking for side effects in assert statements
*/
class CPPCHECKLIB CheckAssert : public Check {
public:
CheckAssert() : Check(myName())
{}
CheckAssert(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger)
: Check(myName(), tokenizer, settings, errorLogger)
{}
virtual void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) {
CheckAssert check(tokenizer, settings, errorLogger);
check.assertWithSideEffects();
}
void assertWithSideEffects();
protected:
bool checkVariableAssignment(const Token* tmp, bool reportError = true);
bool inSameScope(const Token* returnTok, const Token* assignTok);
static const Token* findAssertPattern(const Token *start);
private:
void sideEffectInAssertError(const Token *tok, const std::string& functionName);
void assignmentInAssertError(const Token *tok, const std::string &varname);
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
CheckAssert c(0, settings, errorLogger);
c.assertWithSideEffects();
}
static std::string myName() {
return "Side Effects in asserts";
}
std::string classInfo() const {
return "Warn if side effects in assert statements \n";
}
};
/// @}
//---------------------------------------------------------------------------
#endif

40
lib/checkother.cpp
View File

@ -1170,46 +1170,6 @@ void CheckOther::selfAssignmentError(const Token *tok, const std::string &varnam
"selfAssignment", "Redundant assignment of '" + varname + "' to itself.");
}
//---------------------------------------------------------------------------
// int a = 1;
// assert(a = 2); // <- assert should not have a side-effect
//---------------------------------------------------------------------------
static inline const Token *findAssertPattern(const Token *start)
{
return Token::findmatch(start, "assert ( %any%");
}
void CheckOther::checkAssignmentInAssert()
{
if (!_settings->isEnabled("warning"))
return;
const Token *tok = findAssertPattern(_tokenizer->tokens());
const Token *endTok = tok ? tok->next()->link() : NULL;
while (tok && endTok) {
for (tok = tok->tokAt(2); tok != endTok; tok = tok->next()) {
if (tok->isName() && (tok->next()->isAssignmentOp() || tok->next()->type() == Token::eIncDecOp))
assignmentInAssertError(tok, tok->str());
else if (Token::Match(tok, "--|++ %var%"))
assignmentInAssertError(tok, tok->strAt(1));
}
tok = findAssertPattern(endTok->next());
endTok = tok ? tok->next()->link() : NULL;
}
}
void CheckOther::assignmentInAssertError(const Token *tok, const std::string &varname)
{
reportError(tok, Severity::warning,
"assignmentInAssert", "Assert statement modifies '" + varname + "'.\n"
"Variable '" + varname + "' is modified insert assert statement. "
"Assert statements are removed from release builds so the code inside "
"assert statement is not executed. If the code is needed also in release "
"builds, this is a bug.");
}
//---------------------------------------------------------------------------
// if ((x != 1) || (x != 3)) // expression always true
// if ((x == 1) && (x == 3)) // expression always false

6
lib/checkother.h
View File

@ -60,7 +60,6 @@ public:
checkOther.checkRedundantAssignment();
checkOther.checkRedundantAssignmentInSwitch();
checkOther.checkSuspiciousCaseInSwitch();
checkOther.checkAssignmentInAssert();
checkOther.checkSelfAssignment();
checkOther.checkDuplicateIf();
checkOther.checkDuplicateBranch();
@ -186,9 +185,6 @@ public:
/** @brief %Check for assigning a variable to itself*/
void checkSelfAssignment();
/** @brief %Check for assignment to a variable in an assert test*/
void checkAssignmentInAssert();
/** @brief %Check for testing for mutual exclusion over ||*/
void checkIncorrectLogicOperator();
@ -293,7 +289,6 @@ private:
void suspiciousCaseInSwitchError(const Token* tok, const std::string& operatorString);
void suspiciousEqualityComparisonError(const Token* tok);
void selfAssignmentError(const Token *tok, const std::string &varname);
void assignmentInAssertError(const Token *tok, const std::string &varname);
void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always);
void redundantConditionError(const Token *tok, const std::string &text);
void misusedScopeObjectError(const Token *tok, const std::string &varname);
@ -357,7 +352,6 @@ private:
c.suspiciousCaseInSwitchError(0, "||");
c.suspiciousEqualityComparisonError(0);
c.selfAssignmentError(0, "varname");
c.assignmentInAssertError(0, "varname");
c.incorrectLogicOperatorError(0, "foo > 3 && foo < 4", true);
c.redundantConditionError(0, "If x > 10 the condition x > 11 is always true.");
c.memsetZeroBytesError(0, "varname");

2
lib/cppcheck.vcxproj
View File

@ -36,6 +36,7 @@
</ItemGroup>
<ItemGroup>
<ClCompile Include="check64bit.cpp" />
<ClCompile Include="checkassert.cpp" />
<ClCompile Include="checkassignif.cpp" />
<ClCompile Include="checkautovariables.cpp" />
<ClCompile Include="checkbool.cpp" />
@ -75,6 +76,7 @@
<ItemGroup>
<ClInclude Include="check.h" />
<ClInclude Include="check64bit.h" />
<ClInclude Include="checkassert.h" />
<ClInclude Include="checkassignif.h" />
<ClInclude Include="checkautovariables.h" />
<ClInclude Include="checkbool.h" />

6
lib/cppcheck.vcxproj.filters
View File

@ -119,6 +119,9 @@
<ClCompile Include="checksizeof.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="checkassert.cpp">
<Filter>Source Files</Filter>
</ClCompile>
</ItemGroup>
<ItemGroup>
<ClInclude Include="checkbufferoverrun.h">
@ -238,6 +241,9 @@
<ClInclude Include="checksizeof.h">
<Filter>Header Files</Filter>
</ClInclude>
<ClInclude Include="checkassert.h">
<Filter>Header Files</Filter>
</ClInclude>
</ItemGroup>
<ItemGroup>
<ResourceCompile Include="version.rc" />

2
lib/lib.pri
View File

@ -4,6 +4,7 @@ include($$PWD/pcrerules.pri)
INCLUDEPATH += ../externals/tinyxml
HEADERS += $${BASEPATH}check.h \
$${BASEPATH}check64bit.h \
$${BASEPATH}checkassert.h \
$${BASEPATH}checkassignif.h \
$${BASEPATH}checkautovariables.h \
$${BASEPATH}checkbool.h \
@ -42,6 +43,7 @@ HEADERS += $${BASEPATH}check.h \
SOURCES += $${BASEPATH}check64bit.cpp \
$${BASEPATH}checkassert.cpp \
$${BASEPATH}checkassignif.cpp \
$${BASEPATH}checkautovariables.cpp \
$${BASEPATH}checkbool.cpp \

141
test/testassert.cpp Normal file
View File

@ -0,0 +1,141 @@
/*
* Cppcheck - A tool for static C/C++ code analysis
* Copyright (C) 2007-2013 Daniel Marjamäki and Cppcheck team.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
#include "tokenize.h"
#include "checkassert.h"
#include "testsuite.h"
#include <sstream>
extern std::ostringstream errout;
class TestAssert : public TestFixture {
public:
TestAssert() : TestFixture("TestAsserts") {}
private:
void check(
const char code[],
const char *filename = NULL) {
// Clear the error buffer..
errout.str("");
Settings settings;
settings.addEnabled("style");
settings.addEnabled("warning");
settings.addEnabled("portability");
settings.addEnabled("performance");
// Tokenize..
Tokenizer tokenizer(&settings, this);
std::istringstream istr(code);
tokenizer.tokenize(istr, filename ? filename : "test.cpp");
// Check..
CheckAssert checkAssert(&tokenizer, &settings, this);
checkAssert.runSimplifiedChecks(&tokenizer, &settings, this);
}
void run() {
TEST_CASE(assignmentInAssert);
TEST_CASE(functionCallInAssert);
TEST_CASE(safeFunctionCallInAssert);
}
void safeFunctionCallInAssert() {
check(
"int a;\n"
"bool b = false;\n"
"int foo() {\n"
" if (b) { a = 1+2 };\n"
" return a;\n"
"}\n"
"assert(foo() == 3); \n"
);
ASSERT_EQUALS("", errout.str());
}
void functionCallInAssert() {
check(
"int a;\n"
"int foo() {\n"
" a = 1+2;\n"
" return a;\n"
"}\n"
"assert(foo() == 3); \n"
);
ASSERT_EQUALS("[test.cpp:6]: (warning) Assert statement calls a function which may have desired side effects: 'foo'.\n", errout.str());
}
void assignmentInAssert() {
check("void f() {\n"
" int a; a = 0;\n"
" assert(a = 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
check("void f(int a) {\n"
" assert(a == 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("", errout.str());
check("void f(int a, int b) {\n"
" assert(a == 2 && b = 1);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:2]: (warning) Assert statement modifies 'b'.\n", errout.str());
check("void f() {\n"
" int a; a = 0;\n"
" assert(a += 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
check("void f() {\n"
" int a; a = 0;\n"
" assert(a *= 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
check("void f() {\n"
" int a; a = 0;\n"
" assert(a -= 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
check("void f() {\n"
" int a = 0;\n"
" assert(a--);\n"
" return a;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
}
};
REGISTER_TEST(TestAssert)

1
test/testfiles.pri
View File

@ -4,6 +4,7 @@ INCLUDEPATH += ../externals/tinyxml
SOURCES += $${BASEPATH}/test64bit.cpp \
$${BASEPATH}/testassert.cpp \
$${BASEPATH}/testassignif.cpp \
$${BASEPATH}/testautovariables.cpp \
$${BASEPATH}/testbool.cpp \

61
test/testother.cpp
View File

@ -106,9 +106,6 @@ private:
TEST_CASE(trac2071);
TEST_CASE(trac2084);
TEST_CASE(trac3693);
TEST_CASE(assignmentInAssert);
TEST_CASE(modulo);
TEST_CASE(incorrectLogicOperator1);
@ -3235,62 +3232,6 @@ private:
ASSERT_EQUALS("", errout.str());
}
void assignmentInAssert() {
check("void f() {\n"
" int a; a = 0;\n"
" assert(a = 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
check("void f(int a) {\n"
" assert(a == 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("", errout.str());
check("void f(int a, int b) {\n"
" assert(a == 2 && b = 1);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:2]: (warning) Assert statement modifies 'b'.\n", errout.str());
check("void f() {\n"
" int a; a = 0;\n"
" assert(a += 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
check("void f() {\n"
" int a; a = 0;\n"
" assert(a *= 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
check("void f() {\n"
" int a; a = 0;\n"
" assert(a -= 2);\n"
" return a;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
check("void f() {\n"
" int a = 0;\n"
" assert(a--);\n" // don't simplify and verify
" return a;\n"
"}\n", 0, false, false, false, false
);
ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str());
}
void modulo() {
check("bool f(bool& b1, bool& b2, bool& b3) {\n"
" b1 = a % 5 == 4;\n"
@ -6224,4 +6165,4 @@ private:
}
};
REGISTER_TEST(TestOther)
REGISTER_TEST(TestOther)

1
test/testrunner.vcxproj
View File

@ -31,6 +31,7 @@
<ClCompile Include="..\cli\threadexecutor.cpp" />
<ClCompile Include="options.cpp" />
<ClCompile Include="test64bit.cpp" />
<ClCompile Include="testassert.cpp" />
<ClCompile Include="testassignif.cpp" />
<ClCompile Include="testautovariables.cpp" />
<ClCompile Include="testbool.cpp" />

3
test/testrunner.vcxproj.filters
View File

@ -172,6 +172,9 @@
<ClCompile Include="testsizeof.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="testassert.cpp">
<Filter>Source Files</Filter>
</ClCompile>
</ItemGroup>
<ItemGroup>
<ClInclude Include="options.h">