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

Fixed #8251 (Condition: bug when there are more than 2 strcmp() on same buffer (tricky))

This commit is contained in:
Daniel Marjamäki 2017-11-27 12:55:20 +01:00
parent 61ca480b85
commit f28d5e91ac
3 changed files with 112 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
lib/checkstring.cpp
View File

@ -32,6 +32,7 @@
#include <cstddef>
#include <list>
#include <vector>
#include <stack>
#include <utility>
//---------------------------------------------------------------------------
@ -331,6 +332,91 @@ void CheckString::incorrectStringBooleanError(const Token *tok, const std::strin
reportError(tok, Severity::warning, "incorrectStringBooleanError", "Conversion of string literal " + string + " to bool always evaluates to true.", CWE571, false);
}
//---------------------------------------------------------------------------
// always true: strcmp(str,"a")==0 || strcmp(str,"b")
// TODO: Library configuration for string comparison functions
//---------------------------------------------------------------------------
void CheckString::overlappingStringComparisons()
{
if (!_settings->isEnabled(Settings::WARNING))
return;
const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
const std::size_t functions = symbolDatabase->functionScopes.size();
for (std::size_t i = 0; i < functions; ++i) {
const Scope * scope = symbolDatabase->functionScopes[i];
for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) {
if (tok->str() != "||")
continue;
std::list<const Token *> equals0;
std::list<const Token *> notEquals0;
std::stack<const Token *> tokens;
tokens.push(tok);
while (!tokens.empty()) {
const Token * const t = tokens.top();
tokens.pop();
if (!t)
continue;
if (t->str() == "||") {
tokens.push(t->astOperand1());
tokens.push(t->astOperand2());
continue;
}
if (t->str() == "==") {
if (Token::simpleMatch(t->astOperand1(), "(") && Token::simpleMatch(t->astOperand2(), "0"))
equals0.push_back(t->astOperand1());
else if (Token::simpleMatch(t->astOperand2(), "(") && Token::simpleMatch(t->astOperand1(), "0"))
equals0.push_back(t->astOperand2());
continue;
}
if (t->str() == "!=") {
if (Token::simpleMatch(t->astOperand1(), "(") && Token::simpleMatch(t->astOperand2(), "0"))
notEquals0.push_back(t->astOperand1());
else if (Token::simpleMatch(t->astOperand2(), "(") && Token::simpleMatch(t->astOperand1(), "0"))
notEquals0.push_back(t->astOperand2());
continue;
}
if (t->str() == "!" && Token::simpleMatch(t->astOperand1(), "("))
equals0.push_back(t->astOperand1());
else if (t->str() == "(")
notEquals0.push_back(t);
}
bool error = false;
for (std::list<const Token *>::const_iterator eq0 = equals0.begin(); !error && eq0 != equals0.end(); ++eq0) {
for (std::list<const Token *>::const_iterator ne0 = notEquals0.begin(); !error && ne0 != notEquals0.end(); ++ne0) {
const Token *tok1 = *eq0;
const Token *tok2 = *ne0;
if (!Token::simpleMatch(tok1->previous(), "strcmp ("))
continue;
if (!Token::simpleMatch(tok2->previous(), "strcmp ("))
continue;
const std::vector<const Token *> args1 = getArguments(tok1->previous());
const std::vector<const Token *> args2 = getArguments(tok2->previous());
if (args1.size() != 2 || args2.size() != 2)
continue;
if (args1[1]->isLiteral() &&
args2[1]->isLiteral() &&
args1[1]->str() != args2[1]->str() &&
isSameExpression(_tokenizer->isCPP(), true, args1[0], args2[0], _settings->library, true))
overlappingStringComparisonsError(tok1, tok2);
}
}
}
}
}
void CheckString::overlappingStringComparisonsError(const Token *eq0, const Token *ne0)
{
std::string eq0Expr(eq0 ? eq0->expressionString() : std::string("strcmp(x,\"abc\")"));
if (eq0 && eq0->astParent()->str() == "!")
eq0Expr = "!" + eq0Expr;
else
eq0Expr += " == 0";
const std::string ne0Expr = (ne0 ? ne0->expressionString() : std::string("strcmp(x,\"def\")")) + " != 0";
reportError(ne0, Severity::warning, "overlappingStringComparisons", "The comparison operator in '" + ne0Expr + "' should maybe be '==' instead, currently the expression '" + eq0Expr + "' is redundant.");
}
//---------------------------------------------------------------------------
// Overlapping source and destination passed to sprintf().
// TODO: Library configuration for overlapping arguments

9
lib/checkstring.h
View File

@ -57,6 +57,7 @@ public:
checkString.strPlusChar();
checkString.checkSuspiciousStringCompare();
checkString.stringLiteralWrite();
checkString.overlappingStringComparisons();
}
/** @brief Run checks against the simplified token list */
@ -84,6 +85,9 @@ public:
/** @brief %Check for suspicious code that compares string literals for equality */
void checkAlwaysTrueOrFalseStringCompare();
/** @brief %Check for overlapping string comparisons */
void overlappingStringComparisons();
/** @brief %Check for overlapping source and destination passed to sprintf() */
void sprintfOverlappingData();
@ -97,6 +101,7 @@ private:
void alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2);
void suspiciousStringCompareError(const Token* tok, const std::string& var);
void suspiciousStringCompareError_char(const Token* tok, const std::string& var);
void overlappingStringComparisonsError(const Token* tok1, const Token *tok2);
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
CheckString c(nullptr, settings, errorLogger);
@ -110,6 +115,7 @@ private:
c.incorrectStringBooleanError(nullptr, "\"Hello World\"");
c.alwaysTrueFalseStringCompareError(nullptr, "str1", "str2");
c.alwaysTrueStringVariableCompareError(nullptr, "varname1", "varname2");
c.overlappingStringComparisonsError(nullptr, nullptr);
}
static std::string myName() {
@ -123,7 +129,8 @@ private:
"- suspicious condition (runtime comparison of string literals)\n"
"- suspicious condition (string literals as boolean)\n"
"- suspicious comparison of a string literal with a char* variable\n"
"- suspicious comparison of '\\0' with a char* variable\n";
"- suspicious comparison of '\\0' with a char* variable\n"
"- overlapping string comparisons (strcmp(str,\"x\")==0 || strcmp(str,\"y\")!=0)";
}
};
/// @}

18
test/teststring.cpp
View File

@ -51,6 +51,8 @@ private:
TEST_CASE(sprintf4); // struct member
TEST_CASE(incorrectStringCompare);
TEST_CASE(overlappingStringComparisons);
}
void check(const char code[], const char filename[] = "test.cpp") {
@ -578,6 +580,22 @@ private:
"}");
ASSERT_EQUALS("", errout.str());
}
void overlappingStringComparisons() {
check("void f(const char *str) {\n"
" if (strcmp(str, \"abc\") == 0 || strcmp(str, \"def\")) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) The comparison operator in 'strcmp(str,\"def\") != 0' should maybe be '==' instead, currently the expression 'strcmp(str,\"abc\") == 0' is redundant.\n", errout.str());
check("struct X {\n"
" char *str;\n"
"};\n"
"\n"
"void f(const struct X *x) {\n"
" if (strcmp(x->str, \"abc\") == 0 || strcmp(x->str, \"def\")) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:6]: (warning) The comparison operator in 'strcmp(x->str,\"def\") != 0' should maybe be '==' instead, currently the expression 'strcmp(x->str,\"abc\") == 0' is redundant.\n", errout.str());
}
};
REGISTER_TEST(TestString)