From eba8c6f6c5c3461cdd2d9743566a50f0cb4a1aec Mon Sep 17 00:00:00 2001 From: PKEuS Date: Wed, 27 Aug 2014 09:42:09 +0200 Subject: [PATCH] Refactorization: - Added missing separating comments between checks in checkother.cpp - Moved checks related to strings into own file --- lib/checkother.cpp | 334 +++----------------- lib/checkother.h | 34 -- lib/checkstring.cpp | 314 +++++++++++++++++++ lib/checkstring.h | 119 +++++++ lib/cppcheck.vcxproj | 2 + lib/cppcheck.vcxproj.filters | 6 + test/testother.cpp | 477 +---------------------------- test/testrunner.vcxproj | 1 + test/testrunner.vcxproj.filters | 3 + test/teststring.cpp | 528 ++++++++++++++++++++++++++++++++ test/testutils.h | 18 ++ 11 files changed, 1035 insertions(+), 801 deletions(-) create mode 100644 lib/checkstring.cpp create mode 100644 lib/checkstring.h create mode 100644 test/teststring.cpp diff --git a/lib/checkother.cpp b/lib/checkother.cpp index c97148641..65207226a 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -273,6 +273,7 @@ void CheckOther::checkCastIntToCharAndBackError(const Token *tok, const std::str //--------------------------------------------------------------------------- +// Clarify calculation precedence for ternary operators. //--------------------------------------------------------------------------- void CheckOther::clarifyCalculation() { @@ -458,7 +459,9 @@ void CheckOther::clarifyStatementError(const Token *tok) "Thus, the dereference is meaningless. Did you intend to write '(*A)++;'?"); } - +//--------------------------------------------------------------------------- +// Check for suspicious occurences of 'if(); {}'. +//--------------------------------------------------------------------------- void CheckOther::checkSuspiciousSemicolon() { if (!_settings->inconclusive || !_settings->isEnabled("warning")) @@ -488,6 +491,7 @@ void CheckOther::SuspiciousSemicolonError(const Token* tok) //--------------------------------------------------------------------------- +// For C++ code, warn if C-style casts are used on pointer types //--------------------------------------------------------------------------- void CheckOther::warningOldStylePointerCast() { @@ -1001,6 +1005,7 @@ void CheckOther::redundantBitwiseOperationInSwitchError(const Token *tok, const //--------------------------------------------------------------------------- +// Detect fall through cases (experimental). //--------------------------------------------------------------------------- void CheckOther::checkSwitchCaseFallThrough() { @@ -1456,49 +1461,6 @@ void CheckOther::invalidFunctionUsage() } } } - - // sprintf|snprintf overlapping data - for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - // Get variable id of target buffer.. - unsigned int varid = 0; - - if (Token::Match(tok, "sprintf|snprintf|swprintf ( %var% ,")) - varid = tok->tokAt(2)->varId(); - - else if (Token::Match(tok, "sprintf|snprintf|swprintf ( %var% . %var% ,")) - varid = tok->tokAt(4)->varId(); - - if (varid == 0) - continue; - - // goto next argument - const Token *tok2 = tok->tokAt(2)->nextArgument(); - - if (tok->str() == "snprintf" || tok->str() == "swprintf") { // Jump over second parameter for snprintf and swprintf - tok2 = tok2->nextArgument(); - if (!tok2) - continue; - } - - // is any source buffer overlapping the target buffer? - do { - if (Token::Match(tok2, "%varid% [,)]", varid)) { - sprintfOverlappingDataError(tok2, tok2->str()); - break; - } - } while (nullptr != (tok2 = tok2->nextArgument())); - } -} - -void CheckOther::sprintfOverlappingDataError(const Token *tok, const std::string &varname) -{ - reportError(tok, Severity::error, "sprintfOverlappingData", - "Undefined behavior: Variable '" + varname + "' is used as parameter and destination in s[n]printf().\n" - "The variable '" + varname + "' is used both as a parameter and as destination in " - "s[n]printf(). The origin and destination buffers overlap. Quote from glibc (C-library) " - "documentation (http://www.gnu.org/software/libc/manual/html_mono/libc.html#Formatted-Output-Functions): " - "\"If copying takes place between objects that overlap as a result of a call " - "to sprintf() or snprintf(), the results are undefined.\""); } void CheckOther::invalidFunctionArgError(const Token *tok, const std::string &functionName, int argnr, const std::string &validstr) @@ -1821,8 +1783,10 @@ void CheckOther::checkVariableScope() elseif = true; else if (Token::simpleMatch(endif, "} else {") && Token::simpleMatch(endif->linkAt(2),"} }")) elseif = true; - if (elseif && Token::findmatch(tok->next(), "%varid%", tok->linkAt(1), var->declarationId())) + if (elseif && Token::findmatch(tok->next(), "%varid%", tok->linkAt(1), var->declarationId())) { reduce = false; + break; + } } else if (tok->varId() == var->declarationId() || tok->str() == "goto") { reduce = false; break; @@ -1940,6 +1904,9 @@ void CheckOther::variableScopeError(const Token *tok, const std::string &varname "When you see this message it is always safe to reduce the variable scope 1 level."); } +//--------------------------------------------------------------------------- +// Comma in return statement: return a+1, b++;. (experimental) +//--------------------------------------------------------------------------- void CheckOther::checkCommaSeparatedReturn() { // This is experimental for now. See #5076 @@ -2202,34 +2169,7 @@ void CheckOther::constStatementError(const Token *tok, const std::string &type) } //--------------------------------------------------------------------------- -// str plus char -//--------------------------------------------------------------------------- - -void CheckOther::strPlusChar() -{ - //_tokenizer->tokens()->printAst(true); - //_tokenizer->tokens()->printOut(); - 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() == "+") { - if (tok->astOperand1() && (tok->astOperand1()->type() == Token::eString)) { // string literal... - if (tok->astOperand2() && (tok->astOperand2()->type() == Token::eChar || isChar(tok->astOperand2()->variable()))) // added to char variable or char constant - strPlusCharError(tok); - } - } - } - } -} - -void CheckOther::strPlusCharError(const Token *tok) -{ - reportError(tok, Severity::error, "strPlusChar", "Unusual pointer arithmetic. A value of type 'char' is added to a string literal."); -} - -//--------------------------------------------------------------------------- +// Detect division by zero. //--------------------------------------------------------------------------- void CheckOther::checkZeroDivision() { @@ -2289,12 +2229,12 @@ void CheckOther::zerodivcondError(const Token *tokcond, const Token *tokdiv, boo const std::string linenr(MathLib::toString(tokdiv ? tokdiv->linenr() : 0)); reportError(callstack, Severity::warning, "zerodivcond", "Either the condition '"+condition+"' is useless or there is division by zero at line " + linenr + ".", inconclusive); } + //--------------------------------------------------------------------------- +// Check for NaN (not-a-number) in an arithmetic expression, e.g. +// double d = 1.0 / 0.0 + 100.0; //--------------------------------------------------------------------------- -/** @brief Check for NaN (not-a-number) in an arithmetic expression - * @note e.g. double d = 1.0 / 0.0 + 100.0; - */ void CheckOther::checkNanInArithmeticExpression() { for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { @@ -2315,6 +2255,7 @@ void CheckOther::nanInArithmeticExpressionError(const Token *tok) } //--------------------------------------------------------------------------- +// Detect passing wrong values to functions like atan(0, x); //--------------------------------------------------------------------------- void CheckOther::checkMathFunctions() { @@ -2383,7 +2324,9 @@ void CheckOther::mathfunctionCallError(const Token *tok, const unsigned int numP } else reportError(tok, Severity::error, "wrongmathcall", "Passing value '#' to #() leads to undefined result."); } + //--------------------------------------------------------------------------- +// Creating instance of clases which are destroyed immediately //--------------------------------------------------------------------------- void CheckOther::checkMisusedScopedObject() { @@ -2416,67 +2359,6 @@ void CheckOther::misusedScopeObjectError(const Token *tok, const std::string& va "unusedScopedObject", "Instance of '" + varname + "' object is destroyed immediately."); } -//--------------------------------------------------------------------------- -//--------------------------------------------------------------------------- -void CheckOther::checkIncorrectStringCompare() -{ - if (!_settings->isEnabled("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()) { - // skip "assert(str && ..)" and "assert(.. && str)" - if (Token::Match(tok, "%var% (") && - (Token::Match(tok->tokAt(2), "%str% &&") || Token::Match(tok->next()->link()->tokAt(-2), "&& %str% )")) && - (tok->str().find("assert")+6U==tok->str().size() || tok->str().find("ASSERT")+6U==tok->str().size())) - tok = tok->next()->link(); - - if (Token::simpleMatch(tok, ". substr (") && Token::Match(tok->tokAt(3)->nextArgument(), "%num% )")) { - MathLib::bigint clen = MathLib::toLongNumber(tok->linkAt(2)->strAt(-1)); - const Token* begin = tok->previous(); - for (;;) { // Find start of statement - while (begin->link() && Token::Match(begin, "]|)|>")) - begin = begin->link()->previous(); - if (Token::Match(begin->previous(), ".|::")) - begin = begin->tokAt(-2); - else - break; - } - begin = begin->previous(); - const Token* end = tok->linkAt(2)->next(); - if (Token::Match(begin->previous(), "%str% ==|!=") && begin->strAt(-2) != "+") { - std::size_t slen = Token::getStrLength(begin->previous()); - if (clen != (int)slen) { - incorrectStringCompareError(tok->next(), "substr", begin->strAt(-1)); - } - } else if (Token::Match(end, "==|!= %str% !!+")) { - std::size_t slen = Token::getStrLength(end->next()); - if (clen != (int)slen) { - incorrectStringCompareError(tok->next(), "substr", end->strAt(1)); - } - } - } else if (Token::Match(tok, "&&|%oror%|( %str% &&|%oror%|)") && !Token::Match(tok, "( %str% )")) { - incorrectStringBooleanError(tok->next(), tok->strAt(1)); - } else if (Token::Match(tok, "if|while ( %str% )")) { - incorrectStringBooleanError(tok->tokAt(2), tok->strAt(2)); - } - } - } -} - -void CheckOther::incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string) -{ - reportError(tok, Severity::warning, "incorrectStringCompare", "String literal " + string + " doesn't match length argument for " + func + "()."); -} - -void CheckOther::incorrectStringBooleanError(const Token *tok, const std::string& string) -{ - reportError(tok, Severity::warning, "incorrectStringBooleanError", "Conversion of string literal " + string + " to bool always evaluates to true."); -} - //----------------------------------------------------------------------------- // check for duplicate code in if and else branches // if (a) { b = true; } else { b = true; } @@ -2722,6 +2604,12 @@ void CheckOther::doubleCloseDirError(const Token *tok, const std::string &varnam reportError(tok, Severity::error, "doubleCloseDir", "Directory handle '" + varname +"' closed twice."); } +//--------------------------------------------------------------------------- +// check for the same expression on both sides of an operator +// (x == x), (x && x), (x || x) +// (x.y == x.y), (x.y && x.y), (x.y || x.y) +//--------------------------------------------------------------------------- + namespace { bool notconst(const Function* func) { @@ -2752,12 +2640,6 @@ namespace { } } -//--------------------------------------------------------------------------- -// check for the same expression on both sides of an operator -// (x == x), (x && x), (x || x) -// (x.y == x.y), (x.y && x.y), (x.y || x.y) -//--------------------------------------------------------------------------- - static bool isWithoutSideEffects(const Tokenizer *tokenizer, const Token* tok) { if (!tokenizer->isCPP()) @@ -2842,145 +2724,6 @@ void CheckOther::selfAssignmentError(const Token *tok, const std::string &varnam "selfAssignment", "Redundant assignment of '" + varname + "' to itself."); } -//--------------------------------------------------------------------------- -// Check for string comparison involving two static strings. -// if(strcmp("00FF00","00FF00")==0) // <- statement is always true -//--------------------------------------------------------------------------- -void CheckOther::checkAlwaysTrueOrFalseStringCompare() -{ - if (!_settings->isEnabled("warning")) - return; - - for (const Token* tok = _tokenizer->tokens(); tok; tok = tok->next()) { - if (Token::Match(tok, "strncmp|strcmp|stricmp|strcmpi|strcasecmp|wcscmp|wcsncmp (")) { - if (Token::Match(tok->tokAt(2), "%str% , %str%")) { - const std::string &str1 = tok->strAt(2); - const std::string &str2 = tok->strAt(4); - alwaysTrueFalseStringCompareError(tok, str1, str2); - tok = tok->tokAt(5); - } else if (Token::Match(tok->tokAt(2), "%var% , %var%")) { - const std::string &str1 = tok->strAt(2); - const std::string &str2 = tok->strAt(4); - if (str1 == str2) - alwaysTrueStringVariableCompareError(tok, str1, str2); - tok = tok->tokAt(5); - } else if (Token::Match(tok->tokAt(2), "%var% . c_str ( ) , %var% . c_str ( )")) { - const std::string &str1 = tok->strAt(2); - const std::string &str2 = tok->strAt(8); - if (str1 == str2) - alwaysTrueStringVariableCompareError(tok, str1, str2); - tok = tok->tokAt(13); - } - } else if (Token::Match(tok, "QString :: compare ( %str% , %str% )")) { - const std::string &str1 = tok->strAt(4); - const std::string &str2 = tok->strAt(6); - alwaysTrueFalseStringCompareError(tok, str1, str2); - tok = tok->tokAt(7); - } else if (Token::Match(tok, "!!+ %str% ==|!= %str% !!+")) { - const std::string &str1 = tok->strAt(1); - const std::string &str2 = tok->strAt(3); - alwaysTrueFalseStringCompareError(tok, str1, str2); - tok = tok->tokAt(5); - } - } -} - -void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2) -{ - const std::size_t stringLen = 10; - const std::string string1 = (str1.size() < stringLen) ? str1 : (str1.substr(0, stringLen-2) + ".."); - const std::string string2 = (str2.size() < stringLen) ? str2 : (str2.substr(0, stringLen-2) + ".."); - - reportError(tok, Severity::warning, "staticStringCompare", - "Unnecessary comparison of static strings.\n" - "The compared strings, '" + string1 + "' and '" + string2 + "', are always " + (str1==str2?"identical":"unequal") + ". " - "Therefore the comparison is unnecessary and looks suspicious."); -} - -void CheckOther::alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2) -{ - reportError(tok, Severity::warning, "stringCompare", - "Comparison of identical string variables.\n" - "The compared strings, '" + str1 + "' and '" + str2 + "', are identical. " - "This could be a logic bug."); -} - - -//----------------------------------------------------------------------------- -//----------------------------------------------------------------------------- -void CheckOther::checkSuspiciousStringCompare() -{ - if (!_settings->isEnabled("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->type() != Token::eComparisonOp) - continue; - - const Token* varTok = tok->astOperand1(); - const Token* litTok = tok->astOperand2(); - if (!varTok || !litTok) // <- failed to create AST for comparison - continue; - if (varTok->type() == Token::eString || varTok->type() == Token::eNumber) - std::swap(varTok, litTok); - else if (litTok->type() != Token::eString && litTok->type() != Token::eNumber) - continue; - - // Pointer addition? - if (varTok->str() == "+" && _tokenizer->isC()) { - const Token *tokens[2] = { varTok->astOperand1(), varTok->astOperand2() }; - for (int nr = 0; nr < 2; nr++) { - const Token *t = tokens[nr]; - while (t && (t->str() == "." || t->str() == "::")) - t = t->astOperand2(); - if (t && t->variable() && t->variable()->isPointer()) - varTok = t; - } - } - - if (varTok->str() == "*") { - if (!_tokenizer->isC() || varTok->astOperand2() != nullptr || litTok->type() != Token::eString) - continue; - varTok = varTok->astOperand1(); - } - - while (varTok && (varTok->str() == "." || varTok->str() == "::")) - varTok = varTok->astOperand2(); - if (!varTok || !varTok->isName()) - continue; - - const Variable *var = varTok->variable(); - - while (Token::Match(varTok->astParent(), "[.*]")) - varTok = varTok->astParent(); - const std::string varname = varTok->expressionString(); - - if (litTok->type() == Token::eString) { - if (_tokenizer->isC() || (var && var->isArrayOrPointer())) - suspiciousStringCompareError(tok, varname); - } else if (litTok->originalName() == "'\\0'" && var && var->isPointer()) { - suspiciousStringCompareError_char(tok, varname); - } - } - } -} - -void CheckOther::suspiciousStringCompareError(const Token* tok, const std::string& var) -{ - reportError(tok, Severity::warning, "literalWithCharPtrCompare", - "String literal compared with variable '" + var + "'. Did you intend to use strcmp() instead?"); -} - -void CheckOther::suspiciousStringCompareError_char(const Token* tok, const std::string& var) -{ - reportError(tok, Severity::warning, "charLiteralWithCharPtrCompare", - "Char literal compared with pointer '" + var + "'. Did you intend to dereference it?"); -} - //----------------------------------------------------------------------------- // Check is a comparison of two variables leads to condition, which is // always true or false. @@ -3010,10 +2753,10 @@ void CheckOther::checkComparisonFunctionIsAlwaysTrueOrFalse() const std::string& varNameLeft = tok->strAt(2); // get the left variable name if (functionName == "isgreater" || functionName == "isless" || functionName == "islessgreater") { // e.g.: isgreater(x,x) --> (x)>(x) --> false - checkComparisonFunctionIsAlwaysTrueOrFalseError(tok,functionName,varNameLeft,false); + checkComparisonFunctionIsAlwaysTrueOrFalseError(tok, functionName, varNameLeft, false); } else { // functionName == "isgreaterequal" || functionName == "islessequal" // e.g.: isgreaterequal(x,x) --> (x)>=(x) --> true - checkComparisonFunctionIsAlwaysTrueOrFalseError(tok,functionName,varNameLeft,true); + checkComparisonFunctionIsAlwaysTrueOrFalseError(tok, functionName, varNameLeft, true); } } } @@ -3024,12 +2767,13 @@ void CheckOther::checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* to { const std::string strResult = result ? "true" : "false"; reportError(tok, Severity::warning, "comparisonFunctionIsAlwaysTrueOrFalse", - "Comparison of two identical variables with "+functionName+"("+varName+","+varName+") evaluates always to "+strResult+".\n" - "The function "+functionName+" is designed to compare two variables. Calling this function with one variable ("+varName+") " - "for both parameters leads to a statement which is always "+strResult+"."); + "Comparison of two identical variables with " + functionName + "(" + varName + "," + varName + ") evaluates always to " + strResult + ".\n" + "The function " + functionName + " is designed to compare two variables. Calling this function with one variable (" + varName + ") " + "for both parameters leads to a statement which is always " + strResult + "."); } //----------------------------------------------------------------------------- +// Detect "(var % val1) > val2" where val2 is >= val1. //----------------------------------------------------------------------------- void CheckOther::checkModuloAlwaysTrueFalse() { @@ -3211,10 +2955,10 @@ static bool constructorTakesReference(const Scope * const classScope) return false; } -/* -This check rule works for checking the "const A a = getA()" usage when getA() returns "const A &" or "A &". -In most scenarios, "const A & a = getA()" will be more efficient. -*/ +//--------------------------------------------------------------------------- +// This check rule works for checking the "const A a = getA()" usage when getA() returns "const A &" or "A &". +// In most scenarios, "const A & a = getA()" will be more efficient. +//--------------------------------------------------------------------------- void CheckOther::checkRedundantCopy() { if (!_settings->isEnabled("performance") || _tokenizer->isC()) @@ -3354,6 +3098,10 @@ void CheckOther::incompleteArrayFillError(const Token* tok, const std::string& b } +//--------------------------------------------------------------------------- +// Detect oppositing inner and outer conditions +//--------------------------------------------------------------------------- + void CheckOther::oppositeInnerCondition() { if (!_settings->isEnabled("warning")) @@ -3450,6 +3198,10 @@ void CheckOther::oppositeInnerConditionError(const Token *tok1, const Token* tok } +//--------------------------------------------------------------------------- +// Detect NULL being passed to variadic function. +//--------------------------------------------------------------------------- + void CheckOther::checkVarFuncNullUB() { if (!_settings->isEnabled("portability")) diff --git a/lib/checkother.h b/lib/checkother.h index 34b554132..15f0af45c 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -58,7 +58,6 @@ public: checkOther.invalidPointerCast(); checkOther.checkUnsignedDivision(); checkOther.checkCharVariable(); - checkOther.strPlusChar(); checkOther.checkRedundantAssignment(); checkOther.checkRedundantAssignmentInSwitch(); checkOther.checkSuspiciousCaseInSwitch(); @@ -70,7 +69,6 @@ public: checkOther.clarifyCondition(); // not simplified because ifAssign checkOther.checkSignOfUnsignedVariable(); // don't ignore casts (#3574) checkOther.checkIncompleteArrayFill(); - checkOther.checkSuspiciousStringCompare(); checkOther.checkVarFuncNullUB(); checkOther.checkNanInArithmeticExpression(); checkOther.checkCommaSeparatedReturn(); @@ -97,9 +95,7 @@ public: checkOther.checkMisusedScopedObject(); checkOther.checkMemsetZeroBytes(); checkOther.checkMemsetInvalid2ndParam(); - checkOther.checkIncorrectStringCompare(); checkOther.checkSwitchCaseFallThrough(); - checkOther.checkAlwaysTrueOrFalseStringCompare(); checkOther.checkModuloAlwaysTrueFalse(); checkOther.checkPipeParameterSize(); @@ -158,9 +154,6 @@ public: /** @brief Incomplete statement. A statement that only contains a constant or variable */ void checkIncompleteStatement(); - /** @brief str plus char (unusual pointer arithmetic) */ - void strPlusChar(); - /** @brief %Check zero division*/ void checkZeroDivision(); @@ -203,12 +196,6 @@ public: /** @brief %Check for invalid 2nd parameter of memset() */ void checkMemsetInvalid2ndParam(); - /** @brief %Check for using bad usage of strncmp and substr */ - void checkIncorrectStringCompare(); - - /** @brief %Check for comparison of a string literal with a char* variable */ - void checkSuspiciousStringCompare(); - /** @brief %Check for suspicious code where multiple if have the same expression (e.g "if (a) { } else if (a) { }") */ void checkDuplicateIf(); @@ -218,9 +205,6 @@ public: /** @brief %Check for suspicious code with the same expression on both sides of operator (e.g "if (a && a)") */ void checkDuplicateExpression(); - /** @brief %Check for suspicious code that compares string literals for equality */ - void checkAlwaysTrueOrFalseStringCompare(); - /** @brief %Check for suspicious usage of modulo (e.g. "if(var % 4 == 4)") */ void checkModuloAlwaysTrueFalse(); @@ -277,7 +261,6 @@ private: void redundantGetAndSetUserIdError(const Token *tok); void cstyleCastError(const Token *tok); void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive); - void sprintfOverlappingDataError(const Token *tok, const std::string &varname); void invalidFunctionArgError(const Token *tok, const std::string &functionName, int argnr, const std::string &validstr); void invalidFunctionArgBoolError(const Token *tok, const std::string &functionName, int argnr); void udivError(const Token *tok, bool inconclusive); @@ -286,7 +269,6 @@ private: void charArrayIndexError(const Token *tok); void charBitOpError(const Token *tok); void variableScopeError(const Token *tok, const std::string &varname); - void strPlusCharError(const Token *tok); void zerodivError(const Token *tok, bool inconclusive); void zerodivcondError(const Token *tokcond, const Token *tokdiv, bool inconclusive); void nanInArithmeticExpressionError(const Token *tok); @@ -306,15 +288,11 @@ private: void memsetZeroBytesError(const Token *tok, const std::string &varname); void memsetFloatError(const Token *tok, const std::string &var_value); void memsetValueOutOfRangeError(const Token *tok, const std::string &value); - void incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string); - void incorrectStringBooleanError(const Token *tok, const std::string& string); void duplicateIfError(const Token *tok1, const Token *tok2); void duplicateBranchError(const Token *tok1, const Token *tok2); void duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string &op); void alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2); 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 duplicateBreakError(const Token *tok, bool inconclusive); void unreachableCodeError(const Token* tok, bool inconclusive); void unsignedLessThanZeroError(const Token *tok, const std::string &varname, bool inconclusive); @@ -334,7 +312,6 @@ private: CheckOther c(0, settings, errorLogger); // error - c.sprintfOverlappingDataError(0, "varname"); c.invalidFunctionArgError(0, "func_name", 1, "1-4"); c.invalidFunctionArgBoolError(0, "func_name", 1); c.udivError(0, false); @@ -362,7 +339,6 @@ private: c.charArrayIndexError(0); c.charBitOpError(0); c.variableScopeError(0, "varname"); - c.strPlusCharError(0); c.redundantAssignmentInSwitchError(0, 0, "var"); c.redundantCopyInSwitchError(0, 0, "var"); c.switchCaseFallThrough(0); @@ -377,14 +353,8 @@ private: c.clarifyCalculationError(0, "+"); c.clarifyConditionError(0, true, false); c.clarifyStatementError(0); - c.incorrectStringCompareError(0, "substr", "\"Hello World\""); - c.suspiciousStringCompareError(0, "foo"); - c.suspiciousStringCompareError_char(0, "foo"); - c.incorrectStringBooleanError(0, "\"Hello World\""); c.duplicateBranchError(0, 0); c.duplicateExpressionError(0, 0, "&&"); - c.alwaysTrueFalseStringCompareError(0, "str1", "str2"); - c.alwaysTrueStringVariableCompareError(0, "varname1", "varname2"); c.duplicateBreakError(0, false); c.unreachableCodeError(0, false); c.unsignedLessThanZeroError(0, "varname", false); @@ -411,7 +381,6 @@ private: "* division with zero\n" "* scoped object destroyed immediately after construction\n" "* assignment in an assert statement\n" - "* incorrect length arguments for 'substr' and 'strncmp'\n" "* free() or delete of an invalid memory location\n" "* double free() or double closedir()\n" "* bitwise operation with negative right operand\n" @@ -453,9 +422,6 @@ private: "* Comparison of values leading always to true or false\n" "* Clarify calculation with parentheses\n" "* suspicious condition (assignment+comparison)\n" - "* 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" "* duplicate break statement\n" "* unreachable code\n" diff --git a/lib/checkstring.cpp b/lib/checkstring.cpp new file mode 100644 index 000000000..9fdfa4f4a --- /dev/null +++ b/lib/checkstring.cpp @@ -0,0 +1,314 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2014 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 . + */ + + +//--------------------------------------------------------------------------- +#include "checkstring.h" +#include "symboldatabase.h" + +//--------------------------------------------------------------------------- + +// Register this check class (by creating a static instance of it) +namespace { + CheckString instance; +} + + +//--------------------------------------------------------------------------- +// Check for string comparison involving two static strings. +// if(strcmp("00FF00","00FF00")==0) // <- statement is always true +//--------------------------------------------------------------------------- +void CheckString::checkAlwaysTrueOrFalseStringCompare() +{ + if (!_settings->isEnabled("warning")) + return; + + for (const Token* tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (Token::Match(tok, "strncmp|strcmp|stricmp|strcmpi|strcasecmp|wcscmp|wcsncmp (")) { + if (Token::Match(tok->tokAt(2), "%str% , %str%")) { + const std::string &str1 = tok->strAt(2); + const std::string &str2 = tok->strAt(4); + alwaysTrueFalseStringCompareError(tok, str1, str2); + tok = tok->tokAt(5); + } else if (Token::Match(tok->tokAt(2), "%var% , %var%")) { + const std::string &str1 = tok->strAt(2); + const std::string &str2 = tok->strAt(4); + if (str1 == str2) + alwaysTrueStringVariableCompareError(tok, str1, str2); + tok = tok->tokAt(5); + } else if (Token::Match(tok->tokAt(2), "%var% . c_str ( ) , %var% . c_str ( )")) { + const std::string &str1 = tok->strAt(2); + const std::string &str2 = tok->strAt(8); + if (str1 == str2) + alwaysTrueStringVariableCompareError(tok, str1, str2); + tok = tok->tokAt(13); + } + } else if (Token::Match(tok, "QString :: compare ( %str% , %str% )")) { + const std::string &str1 = tok->strAt(4); + const std::string &str2 = tok->strAt(6); + alwaysTrueFalseStringCompareError(tok, str1, str2); + tok = tok->tokAt(7); + } else if (Token::Match(tok, "!!+ %str% ==|!= %str% !!+")) { + const std::string &str1 = tok->strAt(1); + const std::string &str2 = tok->strAt(3); + alwaysTrueFalseStringCompareError(tok, str1, str2); + tok = tok->tokAt(5); + } + } +} + +void CheckString::alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2) +{ + const std::size_t stringLen = 10; + const std::string string1 = (str1.size() < stringLen) ? str1 : (str1.substr(0, stringLen-2) + ".."); + const std::string string2 = (str2.size() < stringLen) ? str2 : (str2.substr(0, stringLen-2) + ".."); + + reportError(tok, Severity::warning, "staticStringCompare", + "Unnecessary comparison of static strings.\n" + "The compared strings, '" + string1 + "' and '" + string2 + "', are always " + (str1==str2?"identical":"unequal") + ". " + "Therefore the comparison is unnecessary and looks suspicious."); +} + +void CheckString::alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2) +{ + reportError(tok, Severity::warning, "stringCompare", + "Comparison of identical string variables.\n" + "The compared strings, '" + str1 + "' and '" + str2 + "', are identical. " + "This could be a logic bug."); +} + + +//----------------------------------------------------------------------------- +// Detect "str == '\0'" where "*str == '\0'" is correct. +// Comparing char* with each other instead of using strcmp() +//----------------------------------------------------------------------------- +void CheckString::checkSuspiciousStringCompare() +{ + if (!_settings->isEnabled("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->type() != Token::eComparisonOp) + continue; + + const Token* varTok = tok->astOperand1(); + const Token* litTok = tok->astOperand2(); + if (!varTok || !litTok) // <- failed to create AST for comparison + continue; + if (varTok->type() == Token::eString || varTok->type() == Token::eNumber) + std::swap(varTok, litTok); + else if (litTok->type() != Token::eString && litTok->type() != Token::eNumber) + continue; + + // Pointer addition? + if (varTok->str() == "+" && _tokenizer->isC()) { + const Token *tokens[2] = { varTok->astOperand1(), varTok->astOperand2() }; + for (int nr = 0; nr < 2; nr++) { + const Token *t = tokens[nr]; + while (t && (t->str() == "." || t->str() == "::")) + t = t->astOperand2(); + if (t && t->variable() && t->variable()->isPointer()) + varTok = t; + } + } + + if (varTok->str() == "*") { + if (!_tokenizer->isC() || varTok->astOperand2() != nullptr || litTok->type() != Token::eString) + continue; + varTok = varTok->astOperand1(); + } + + while (varTok && (varTok->str() == "." || varTok->str() == "::")) + varTok = varTok->astOperand2(); + if (!varTok || !varTok->isName()) + continue; + + const Variable *var = varTok->variable(); + + while (Token::Match(varTok->astParent(), "[.*]")) + varTok = varTok->astParent(); + const std::string varname = varTok->expressionString(); + + if (litTok->type() == Token::eString) { + if (_tokenizer->isC() || (var && var->isArrayOrPointer())) + suspiciousStringCompareError(tok, varname); + } else if (litTok->originalName() == "'\\0'" && var && var->isPointer()) { + suspiciousStringCompareError_char(tok, varname); + } + } + } +} + +void CheckString::suspiciousStringCompareError(const Token* tok, const std::string& var) +{ + reportError(tok, Severity::warning, "literalWithCharPtrCompare", + "String literal compared with variable '" + var + "'. Did you intend to use strcmp() instead?"); +} + +void CheckString::suspiciousStringCompareError_char(const Token* tok, const std::string& var) +{ + reportError(tok, Severity::warning, "charLiteralWithCharPtrCompare", + "Char literal compared with pointer '" + var + "'. Did you intend to dereference it?"); +} + + +//--------------------------------------------------------------------------- +// Adding C-string and char with operator+ +//--------------------------------------------------------------------------- + +static bool isChar(const Variable* var) +{ + return (var && !var->isPointer() && !var->isArray() && var->typeStartToken()->str() == "char"); +} + +void CheckString::strPlusChar() +{ + 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() == "+") { + if (tok->astOperand1() && (tok->astOperand1()->type() == Token::eString)) { // string literal... + if (tok->astOperand2() && (tok->astOperand2()->type() == Token::eChar || isChar(tok->astOperand2()->variable()))) // added to char variable or char constant + strPlusCharError(tok); + } + } + } + } +} + +void CheckString::strPlusCharError(const Token *tok) +{ + reportError(tok, Severity::error, "strPlusChar", "Unusual pointer arithmetic. A value of type 'char' is added to a string literal."); +} + +//--------------------------------------------------------------------------- +// Implicit casts of string literals to bool +// Comparing string literal with strlen() with wrong length +//--------------------------------------------------------------------------- +void CheckString::checkIncorrectStringCompare() +{ + if (!_settings->isEnabled("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()) { + // skip "assert(str && ..)" and "assert(.. && str)" + if (Token::Match(tok, "%var% (") && + (Token::Match(tok->tokAt(2), "%str% &&") || Token::Match(tok->next()->link()->tokAt(-2), "&& %str% )")) && + (tok->str().find("assert") + 6U == tok->str().size() || tok->str().find("ASSERT") + 6U == tok->str().size())) + tok = tok->next()->link(); + + if (Token::simpleMatch(tok, ". substr (") && Token::Match(tok->tokAt(3)->nextArgument(), "%num% )")) { + MathLib::bigint clen = MathLib::toLongNumber(tok->linkAt(2)->strAt(-1)); + const Token* begin = tok->previous(); + for (;;) { // Find start of statement + while (begin->link() && Token::Match(begin, "]|)|>")) + begin = begin->link()->previous(); + if (Token::Match(begin->previous(), ".|::")) + begin = begin->tokAt(-2); + else + break; + } + begin = begin->previous(); + const Token* end = tok->linkAt(2)->next(); + if (Token::Match(begin->previous(), "%str% ==|!=") && begin->strAt(-2) != "+") { + std::size_t slen = Token::getStrLength(begin->previous()); + if (clen != (int)slen) { + incorrectStringCompareError(tok->next(), "substr", begin->strAt(-1)); + } + } else if (Token::Match(end, "==|!= %str% !!+")) { + std::size_t slen = Token::getStrLength(end->next()); + if (clen != (int)slen) { + incorrectStringCompareError(tok->next(), "substr", end->strAt(1)); + } + } + } else if (Token::Match(tok, "&&|%oror%|( %str% &&|%oror%|)") && !Token::Match(tok, "( %str% )")) { + incorrectStringBooleanError(tok->next(), tok->strAt(1)); + } else if (Token::Match(tok, "if|while ( %str% )")) { + incorrectStringBooleanError(tok->tokAt(2), tok->strAt(2)); + } + } + } +} + +void CheckString::incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string) +{ + reportError(tok, Severity::warning, "incorrectStringCompare", "String literal " + string + " doesn't match length argument for " + func + "()."); +} + +void CheckString::incorrectStringBooleanError(const Token *tok, const std::string& string) +{ + reportError(tok, Severity::warning, "incorrectStringBooleanError", "Conversion of string literal " + string + " to bool always evaluates to true."); +} + +//--------------------------------------------------------------------------- +// Overlapping source and destination passed to sprintf(). +//--------------------------------------------------------------------------- +void CheckString::sprintfOverlappingData() +{ + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + // Get variable id of target buffer.. + unsigned int varid = 0; + + if (Token::Match(tok, "sprintf|snprintf|swprintf ( %var% ,")) + varid = tok->tokAt(2)->varId(); + + else if (Token::Match(tok, "sprintf|snprintf|swprintf ( %var% . %var% ,")) + varid = tok->tokAt(4)->varId(); + + if (varid == 0) + continue; + + // goto next argument + const Token *tok2 = tok->tokAt(2)->nextArgument(); + + if (tok->str() == "snprintf" || tok->str() == "swprintf") { // Jump over second parameter for snprintf and swprintf + tok2 = tok2->nextArgument(); + if (!tok2) + continue; + } + + // is any source buffer overlapping the target buffer? + do { + if (Token::Match(tok2, "%varid% [,)]", varid)) { + sprintfOverlappingDataError(tok2, tok2->str()); + break; + } + } while (nullptr != (tok2 = tok2->nextArgument())); + } +} + +void CheckString::sprintfOverlappingDataError(const Token *tok, const std::string &varname) +{ + reportError(tok, Severity::error, "sprintfOverlappingData", + "Undefined behavior: Variable '" + varname + "' is used as parameter and destination in s[n]printf().\n" + "The variable '" + varname + "' is used both as a parameter and as destination in " + "s[n]printf(). The origin and destination buffers overlap. Quote from glibc (C-library) " + "documentation (http://www.gnu.org/software/libc/manual/html_mono/libc.html#Formatted-Output-Functions): " + "\"If copying takes place between objects that overlap as a result of a call " + "to sprintf() or snprintf(), the results are undefined.\""); +} diff --git a/lib/checkstring.h b/lib/checkstring.h new file mode 100644 index 000000000..270ff3943 --- /dev/null +++ b/lib/checkstring.h @@ -0,0 +1,119 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2014 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 . + */ + + +//--------------------------------------------------------------------------- +#ifndef checkstringH +#define checkstringH +//--------------------------------------------------------------------------- + +#include "config.h" +#include "check.h" + +/// @addtogroup Checks +/// @{ + + +/** @brief Detect misusage of C-style strings and related standard functions */ + +class CPPCHECKLIB CheckString : public Check { +public: + /** @brief This constructor is used when registering the CheckClass */ + CheckString() : Check(myName()) { + } + + /** @brief This constructor is used when running checks. */ + CheckString(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) + : Check(myName(), tokenizer, settings, errorLogger) { + } + + /** @brief Run checks against the normal token list */ + void runChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) { + CheckString checkString(tokenizer, settings, errorLogger); + + // Checks + checkString.strPlusChar(); + checkString.checkSuspiciousStringCompare(); + } + + /** @brief Run checks against the simplified token list */ + void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) { + CheckString checkString(tokenizer, settings, errorLogger); + + // Checks + checkString.checkIncorrectStringCompare(); + checkString.checkAlwaysTrueOrFalseStringCompare(); + checkString.sprintfOverlappingData(); + } + + /** @brief str plus char (unusual pointer arithmetic) */ + void strPlusChar(); + + /** @brief %Check for using bad usage of strncmp and substr */ + void checkIncorrectStringCompare(); + + /** @brief %Check for comparison of a string literal with a char* variable */ + void checkSuspiciousStringCompare(); + + /** @brief %Check for suspicious code that compares string literals for equality */ + void checkAlwaysTrueOrFalseStringCompare(); + + /** @brief %Check for overlapping source and destination passed to sprintf() */ + void sprintfOverlappingData(); + +private: + void sprintfOverlappingDataError(const Token *tok, const std::string &varname); + void strPlusCharError(const Token *tok); + void incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string); + void incorrectStringBooleanError(const Token *tok, const std::string& string); + void alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2); + 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 getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { + CheckString c(0, settings, errorLogger); + + c.sprintfOverlappingDataError(0, "varname"); + c.strPlusCharError(0); + c.incorrectStringCompareError(0, "substr", "\"Hello World\""); + c.suspiciousStringCompareError(0, "foo"); + c.suspiciousStringCompareError_char(0, "foo"); + c.incorrectStringBooleanError(0, "\"Hello World\""); + c.alwaysTrueFalseStringCompareError(0, "str1", "str2"); + c.alwaysTrueStringVariableCompareError(0, "varname1", "varname2"); + } + + static std::string myName() { + return "String"; + } + + std::string classInfo() const { + return "Detect misusage of C-style strings:\n" + + "* overlapping buffers passed to sprintf as source and destination\n" + "* incorrect length arguments for 'substr' and 'strncmp'\n" + "* 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"; + } +}; +/// @} +//--------------------------------------------------------------------------- +#endif // checkstringH diff --git a/lib/cppcheck.vcxproj b/lib/cppcheck.vcxproj index 77bef2fbe..b6cd46491 100644 --- a/lib/cppcheck.vcxproj +++ b/lib/cppcheck.vcxproj @@ -45,6 +45,7 @@ + @@ -89,6 +90,7 @@ + diff --git a/lib/cppcheck.vcxproj.filters b/lib/cppcheck.vcxproj.filters index 634799577..9fa74291f 100644 --- a/lib/cppcheck.vcxproj.filters +++ b/lib/cppcheck.vcxproj.filters @@ -137,6 +137,9 @@ Source Files + + Source Files + @@ -271,6 +274,9 @@ Header Files + + Header Files + diff --git a/test/testother.cpp b/test/testother.cpp index 193cde2fe..34b9c330c 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -21,6 +21,7 @@ #include "symboldatabase.h" #include "checkother.h" #include "testsuite.h" +#include "testutils.h" #include #include @@ -53,16 +54,6 @@ private: TEST_CASE(invalidFunctionUsage1); - TEST_CASE(sprintf1); // Dangerous usage of sprintf - TEST_CASE(sprintf2); - TEST_CASE(sprintf3); - TEST_CASE(sprintf4); // struct member - - TEST_CASE(strPlusChar1); // "/usr" + '/' - TEST_CASE(strPlusChar2); // "/usr" + ch - TEST_CASE(strPlusChar3); // ok: path + "/sub" + '/' - TEST_CASE(strPlusChar4); // ast - TEST_CASE(varScope1); TEST_CASE(varScope2); TEST_CASE(varScope3); @@ -153,8 +144,6 @@ private: TEST_CASE(clarifyCondition5); // #3609 CWinTraits.. TEST_CASE(clarifyCondition6); // #3818 - TEST_CASE(incorrectStringCompare); - TEST_CASE(duplicateBranch); TEST_CASE(duplicateBranch1); // tests extracted by http://www.viva64.com/en/b/0149/ ( Comparison between PVS-Studio and cppcheck ): Errors detected in Quake 3: Arena by PVS-Studio: Fragement 2 TEST_CASE(duplicateBranch2); // empty macro @@ -165,9 +154,6 @@ private: TEST_CASE(duplicateExpression5); // ticket #3749 (macros with same values) TEST_CASE(duplicateExpression6); // ticket #4639 - TEST_CASE(alwaysTrueFalseStringCompare); - TEST_CASE(suspiciousStringCompare); - TEST_CASE(suspiciousStringCompare_char); TEST_CASE(checkSignOfUnsignedVariable); TEST_CASE(checkSignOfPointer); @@ -246,23 +232,6 @@ private: } } - class SimpleSuppressor: public ErrorLogger { - public: - SimpleSuppressor(Settings &settings, ErrorLogger *next) - : _settings(settings), _next(next) { - } - virtual void reportOut(const std::string &outmsg) { - _next->reportOut(outmsg); - } - virtual void reportErr(const ErrorLogger::ErrorMessage &msg) { - if (!msg._callStack.empty() && !_settings.nomsg.isSuppressed(msg._id, msg._callStack.begin()->getfile(), msg._callStack.begin()->line)) - _next->reportErr(msg); - } - private: - Settings &_settings; - ErrorLogger *_next; - }; - void check_preprocess_suppress(const char precode[], const char *filename = nullptr) { // Clear the error buffer.. errout.str(""); @@ -293,7 +262,6 @@ private: // Check.. CheckOther checkOther(&tokenizer, &settings, &logger); checkOther.checkSwitchCaseFallThrough(); - checkOther.checkAlwaysTrueOrFalseStringCompare(); logger.reportUnmatchedSuppressions(settings.nomsg.getUnmatchedLocalSuppressions(filename)); } @@ -804,113 +772,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void sprintf1() { - invalidFunctionUsage("void foo()\n" - "{\n" - " char buf[100];\n" - " sprintf(buf,\"%s\",buf);\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Undefined behavior: Variable 'buf' is used as parameter and destination in s[n]printf().\n", errout.str()); - } - - void sprintf2() { - invalidFunctionUsage("void foo()\n" - "{\n" - " char buf[100];\n" - " sprintf(buf,\"%i\",sizeof(buf));\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void sprintf3() { - invalidFunctionUsage("void foo()\n" - "{\n" - " char buf[100];\n" - " sprintf(buf,\"%i\",sizeof(buf));\n" - " if (buf[0]);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void sprintf4() { - invalidFunctionUsage("struct A\n" - "{\n" - " char filename[128];\n" - "};\n" - "\n" - "void foo()\n" - "{\n" - " const char* filename = \"hello\";\n" - " struct A a;\n" - " snprintf(a.filename, 128, \"%s\", filename);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - - - - - void strPlusChar(const char code[]) { - // Clear the error buffer.. - errout.str(""); - - Settings settings; - - // Tokenize.. - Tokenizer tokenizer(&settings, this); - std::istringstream istr(code); - tokenizer.tokenize(istr, "test.cpp"); - - // Check for redundant code.. - CheckOther checkOther(&tokenizer, &settings, this); - checkOther.strPlusChar(); - } - - void strPlusChar1() { - // Strange looking pointer arithmetic.. - strPlusChar("void foo()\n" - "{\n" - " const char *p = \"/usr\" + '/';\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Unusual pointer arithmetic. A value of type 'char' is added to a string literal.\n", errout.str()); - } - - void strPlusChar2() { - // Strange looking pointer arithmetic.. - strPlusChar("void foo()\n" - "{\n" - " char ch = 1;\n" - " const char *p = ch + \"/usr\";\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // Strange looking pointer arithmetic.. - strPlusChar("void foo()\n" - "{\n" - " int i = 1;\n" - " const char* psz = \"Bla\";\n" - " const std::string str = i + psz;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void strPlusChar3() { - // Strange looking pointer arithmetic.. - strPlusChar("void foo()\n" - "{\n" - " std::string temp = \"/tmp\";\n" - " std::string path = temp + '/' + \"sub\" + '/';\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void strPlusChar4() { - // don't crash - strPlusChar("int test() { int +; }"); - } - - void varScope(const char code[]) { // Clear the error buffer.. errout.str(""); @@ -4625,83 +4486,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void incorrectStringCompare() { - check("int f() {\n" - " return test.substr( 0 , 4 ) == \"Hello\" ? : 0 : 1 ;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str()); - - check("int f() {\n" - " return test.substr( 0 , 5 ) == \"Hello\" ? : 0 : 1 ;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("int f() {\n" - " return \"Hello\" == test.substr( 0 , 4 ) ? : 0 : 1 ;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str()); - - check("int f() {\n" - " return \"Hello\" == foo.bar().z[1].substr(i+j*4, 4) ? : 0 : 1 ;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str()); - - check("int f() {\n" - " return \"Hello\" == test.substr( 0 , 5 ) ? : 0 : 1 ;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("int f() {\n" - " if (\"Hello\") { }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); - - check("int f() {\n" - " if (\"Hello\" && test) { }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); - - check("int f() {\n" - " if (test && \"Hello\") { }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); - - check("int f() {\n" - " while (\"Hello\") { }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); - - check("int f() {\n" - " assert (test || \"Hello\");\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); - - check("int f() {\n" - " assert (test && \"Hello\");\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("int f() {\n" - " assert (\"Hello\" || test);\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); - - check("int f() {\n" - " assert (\"Hello\" && test);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("int f() {\n" - " BOOST_ASSERT (\"Hello\" && test);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("int f() {\n" - " return f2(\"Hello\");\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - void duplicateBranch() { check("void f(int a, int &b) {\n" " if (a)\n" @@ -5142,265 +4926,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void alwaysTrueFalseStringCompare() { - check_preprocess_suppress( - "#define MACRO \"00FF00\"\n" - "int main()\n" - "{\n" - " if (strcmp(MACRO,\"00FF00\") == 0)" - " {" - " std::cout << \"Equal\n\"" - " }" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Unnecessary comparison of static strings.\n", errout.str()); - - check_preprocess_suppress( - "int main()\n" - "{\n" - " if (stricmp(\"hotdog\",\"HOTdog\") == 0)" - " {" - " std::cout << \"Equal\n\"" - " }" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Unnecessary comparison of static strings.\n", errout.str()); - - check_preprocess_suppress( - "#define MACRO \"Hotdog\"\n" - "int main()\n" - "{\n" - " if (QString::compare(\"Hamburger\", MACRO) == 0)" - " {" - " std::cout << \"Equal\n\"" - " }" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Unnecessary comparison of static strings.\n", errout.str()); - - check_preprocess_suppress( - "int main()\n" - "{\n" - " if (QString::compare(argv[2], \"hotdog\") == 0)" - " {" - " std::cout << \"Equal\n\"" - " }" - "}"); - ASSERT_EQUALS("", errout.str()); - - check_preprocess_suppress( - "int main()\n" - "{\n" - " if (strncmp(\"hotdog\",\"hotdog\", 6) == 0)" - " {" - " std::cout << \"Equal\n\"" - " }" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Unnecessary comparison of static strings.\n", errout.str()); - - check( - "int foo(const char *buf)\n" - "{\n" - " if (strcmp(buf, buf) == 0)" - " {" - " std::cout << \"Equal\n\"" - " }" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of identical string variables.\n", errout.str()); - - check( - "int foo(const std::string& buf)\n" - "{\n" - " if (stricmp(buf.c_str(), buf.c_str()) == 0)" - " {" - " std::cout << \"Equal\n\"" - " }" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of identical string variables.\n", errout.str()); - - check_preprocess_suppress( - "int main() {\n" - " if (\"str\" == \"str\") {\n" - " std::cout << \"Equal\n\"\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Unnecessary comparison of static strings.\n", errout.str()); - - check_preprocess_suppress( - "int main() {\n" - " if (\"str\" != \"str\") {\n" - " std::cout << \"Equal\n\"\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Unnecessary comparison of static strings.\n", errout.str()); - - check_preprocess_suppress( - "int main() {\n" - " if (a+\"str\" != \"str\"+b) {\n" - " std::cout << \"Equal\n\"\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void suspiciousStringCompare() { - check("bool foo(char* c) {\n" - " return c == \"x\";\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", errout.str()); - - check("bool foo(const char* c) {\n" - " return \"x\" == c;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", errout.str()); - - check("bool foo(char* c) {\n" - " return foo+\"x\" == c;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("bool foo(char* c) {\n" - " return \"x\" == c+foo;\n" - "}", "test.cpp"); - ASSERT_EQUALS("", errout.str()); - - check("bool foo(char* c) {\n" - " return \"x\" == c+foo;\n" - "}", "test.c"); - ASSERT_EQUALS("[test.c:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", errout.str()); - - check("bool foo(Foo c) {\n" - " return \"x\" == c.foo;\n" - "}", "test.cpp"); - ASSERT_EQUALS("", errout.str()); - - check("bool foo(Foo c) {\n" - " return \"x\" == c.foo;\n" - "}", "test.c"); - ASSERT_EQUALS("[test.c:2]: (warning) String literal compared with variable 'c.foo'. Did you intend to use strcmp() instead?\n", errout.str()); - - check("bool foo(const std::string& c) {\n" - " return \"x\" == c;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("bool foo(const Foo* c) {\n" - " return \"x\" == c->bar();\n" // #4314 - "}"); - ASSERT_EQUALS("", errout.str()); - - // Ticket #4257 - check("bool foo() {\n" - "MyString *str=Getter();\n" - "return *str==\"bug\"; }\n", "test.c"); - ASSERT_EQUALS("[test.c:3]: (warning) String literal compared with variable '*str'. Did you intend to use strcmp() instead?\n", errout.str()); - - // Ticket #4257 - check("bool foo() {\n" - "MyString *str=Getter();\n" - "return *str==\"bug\"; }"); - ASSERT_EQUALS("", errout.str()); - - // Ticket #4257 - check("bool foo() {\n" - "MyString **str=OtherGetter();\n" - "return *str==\"bug\"; }"); - TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", - "", - errout.str()); - - // Ticket #4257 - check("bool foo() {\n" - "MyString str=OtherGetter2();\n" - "return &str==\"bug\"; }"); - TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", - "", - errout.str()); - - // Ticket #5734 - check("int foo(char c) {\n" - "return c == '42';}", "test.cpp"); - ASSERT_EQUALS("", errout.str()); - check("int foo(char c) {\n" - "return c == '42';}", "test.c"); - ASSERT_EQUALS("", errout.str()); - check("int foo(char c) {\n" - "return c == \"42\"[0];}", "test.cpp", false, true, false, false); - ASSERT_EQUALS("", errout.str()); - check("int foo(char c) {\n" - "return c == \"42\"[0];}", "test.c", false, true, false, false); - ASSERT_EQUALS("", errout.str()); - - // 5639 String literal compared with char buffer in a struct - check("struct Example {\n" - " char buffer[200];\n" - "};\n" - "void foo() {\n" - " struct Example example;\n" - " if (example.buffer == \"test\") ;\n" - "}\n", "test.cpp", false, true, false, false); - ASSERT_EQUALS("[test.cpp:6]: (warning) String literal compared with variable 'example.buffer'. Did you intend to use strcmp() instead?\n", errout.str()); - check("struct Example {\n" - " char buffer[200];\n" - "};\n" - "void foo() {\n" - " struct Example example;\n" - " if (example.buffer == \"test\") ;\n" - "}\n", "test.c", false, true, false, false); - ASSERT_EQUALS("[test.c:6]: (warning) String literal compared with variable 'example.buffer'. Did you intend to use strcmp() instead?\n", errout.str()); - } - - void suspiciousStringCompare_char() { - check("bool foo(char* c) {\n" - " return c == '\\0';\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Char literal compared with pointer 'c'. Did you intend to dereference it?\n", errout.str()); - - check("bool foo(char* c) {\n" - " return '\\0' != c;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Char literal compared with pointer 'c'. Did you intend to dereference it?\n", errout.str()); - - check("bool foo(char c) {\n" - " return c == '\\0';\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("bool foo(char c) {\n" - " return c == 0;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("bool foo(char* c) {\n" - " return *c == 0;\n" - "}", "test.c"); - ASSERT_EQUALS("", errout.str()); - - check("bool foo(char* c) {\n" - " return *c == 0;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("bool foo(Foo* c) {\n" - " return 0 == c->x;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void foo(char* c) {\n" - " if(c == '\\0') bar();\n" - "}"); - TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) Char literal compared with pointer 'c'. Did you intend to dereference it?\n", "", errout.str()); - - check("void f() {\n" - " struct { struct { char *str; } x; } a;\n" - " return a.x.str == '\\0';" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Char literal compared with pointer 'a.x.str'. Did you intend to dereference it?\n", errout.str()); - - check("void f() {\n" - " struct { struct { char *str; } x; } a;\n" - " return *a.x.str == '\\0';" - "}"); - ASSERT_EQUALS("", errout.str()); - } - void check_signOfUnsignedVariable(const char code[], bool inconclusive=false) { // Clear the error buffer.. errout.str(""); diff --git a/test/testrunner.vcxproj b/test/testrunner.vcxproj index eec9d63a4..aff333697 100644 --- a/test/testrunner.vcxproj +++ b/test/testrunner.vcxproj @@ -67,6 +67,7 @@ + Create Create diff --git a/test/testrunner.vcxproj.filters b/test/testrunner.vcxproj.filters index 36f53488d..e4efdeae1 100644 --- a/test/testrunner.vcxproj.filters +++ b/test/testrunner.vcxproj.filters @@ -187,6 +187,9 @@ Source Files + + Source Files + diff --git a/test/teststring.cpp b/test/teststring.cpp new file mode 100644 index 000000000..25ab53aa9 --- /dev/null +++ b/test/teststring.cpp @@ -0,0 +1,528 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2014 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 . + */ + + +#include "tokenize.h" +#include "checkstring.h" +#include "testsuite.h" +#include "preprocessor.h" +#include "testutils.h" +#include + +extern std::ostringstream errout; + +class TestString : public TestFixture { +public: + TestString() : TestFixture("TestString") { + } + +private: + + void run() { + TEST_CASE(alwaysTrueFalseStringCompare); + TEST_CASE(suspiciousStringCompare); + TEST_CASE(suspiciousStringCompare_char); + + TEST_CASE(strPlusChar1); // "/usr" + '/' + TEST_CASE(strPlusChar2); // "/usr" + ch + TEST_CASE(strPlusChar3); // ok: path + "/sub" + '/' + TEST_CASE(strPlusChar4); // ast + + TEST_CASE(sprintf1); // Dangerous usage of sprintf + TEST_CASE(sprintf2); + TEST_CASE(sprintf3); + TEST_CASE(sprintf4); // struct member + + TEST_CASE(incorrectStringCompare); + } + + void check(const char code[], const char filename[] = "test.cpp") { + // Clear the error buffer.. + errout.str(""); + + Settings settings; + settings.addEnabled("warning"); + settings.addEnabled("style"); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, filename); + + // Check char variable usage.. + CheckString checkString(&tokenizer, &settings, this); + checkString.runChecks(&tokenizer, &settings, this); + + tokenizer.simplifyTokenList2(); + checkString.runSimplifiedChecks(&tokenizer, &settings, this); + } + + void check_preprocess_suppress(const char precode[]) { + // Clear the error buffer.. + errout.str(""); + + Settings settings; + settings.addEnabled("warning"); + + // Preprocess file.. + SimpleSuppressor logger(settings, this); + Preprocessor preprocessor(&settings, &logger); + std::list configurations; + std::string filedata; + std::istringstream fin(precode); + preprocessor.preprocess(fin, filedata, configurations, "test.cpp", settings._includePaths); + const std::string code = preprocessor.getcode(filedata, "", "test.cpp"); + + // Tokenize.. + Tokenizer tokenizer(&settings, &logger); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + // Check.. + CheckString checkString(&tokenizer, &settings, &logger); + checkString.checkAlwaysTrueOrFalseStringCompare(); + + logger.reportUnmatchedSuppressions(settings.nomsg.getUnmatchedLocalSuppressions("test.cpp")); + } + + void alwaysTrueFalseStringCompare() { + check_preprocess_suppress( + "#define MACRO \"00FF00\"\n" + "int main()\n" + "{\n" + " if (strcmp(MACRO,\"00FF00\") == 0)" + " {" + " std::cout << \"Equal\n\"" + " }" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (warning) Unnecessary comparison of static strings.\n", errout.str()); + + check_preprocess_suppress( + "int main()\n" + "{\n" + " if (stricmp(\"hotdog\",\"HOTdog\") == 0)" + " {" + " std::cout << \"Equal\n\"" + " }" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Unnecessary comparison of static strings.\n", errout.str()); + + check_preprocess_suppress( + "#define MACRO \"Hotdog\"\n" + "int main()\n" + "{\n" + " if (QString::compare(\"Hamburger\", MACRO) == 0)" + " {" + " std::cout << \"Equal\n\"" + " }" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (warning) Unnecessary comparison of static strings.\n", errout.str()); + + check_preprocess_suppress( + "int main()\n" + "{\n" + " if (QString::compare(argv[2], \"hotdog\") == 0)" + " {" + " std::cout << \"Equal\n\"" + " }" + "}"); + ASSERT_EQUALS("", errout.str()); + + check_preprocess_suppress( + "int main()\n" + "{\n" + " if (strncmp(\"hotdog\",\"hotdog\", 6) == 0)" + " {" + " std::cout << \"Equal\n\"" + " }" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Unnecessary comparison of static strings.\n", errout.str()); + + check( + "int foo(const char *buf)\n" + "{\n" + " if (strcmp(buf, buf) == 0)" + " {" + " std::cout << \"Equal\n\"" + " }" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of identical string variables.\n", errout.str()); + + check( + "int foo(const std::string& buf)\n" + "{\n" + " if (stricmp(buf.c_str(), buf.c_str()) == 0)" + " {" + " std::cout << \"Equal\n\"" + " }" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of identical string variables.\n", errout.str()); + + check_preprocess_suppress( + "int main() {\n" + " if (\"str\" == \"str\") {\n" + " std::cout << \"Equal\n\"\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Unnecessary comparison of static strings.\n", errout.str()); + + check_preprocess_suppress( + "int main() {\n" + " if (\"str\" != \"str\") {\n" + " std::cout << \"Equal\n\"\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Unnecessary comparison of static strings.\n", errout.str()); + + check_preprocess_suppress( + "int main() {\n" + " if (a+\"str\" != \"str\"+b) {\n" + " std::cout << \"Equal\n\"\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void suspiciousStringCompare() { + check("bool foo(char* c) {\n" + " return c == \"x\";\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", errout.str()); + + check("bool foo(const char* c) {\n" + " return \"x\" == c;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", errout.str()); + + check("bool foo(char* c) {\n" + " return foo+\"x\" == c;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("bool foo(char* c) {\n" + " return \"x\" == c+foo;\n" + "}", "test.cpp"); + ASSERT_EQUALS("", errout.str()); + + check("bool foo(char* c) {\n" + " return \"x\" == c+foo;\n" + "}", "test.c"); + ASSERT_EQUALS("[test.c:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", errout.str()); + + check("bool foo(Foo c) {\n" + " return \"x\" == c.foo;\n" + "}", "test.cpp"); + ASSERT_EQUALS("", errout.str()); + + check("bool foo(Foo c) {\n" + " return \"x\" == c.foo;\n" + "}", "test.c"); + ASSERT_EQUALS("[test.c:2]: (warning) String literal compared with variable 'c.foo'. Did you intend to use strcmp() instead?\n", errout.str()); + + check("bool foo(const std::string& c) {\n" + " return \"x\" == c;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("bool foo(const Foo* c) {\n" + " return \"x\" == c->bar();\n" // #4314 + "}"); + ASSERT_EQUALS("", errout.str()); + + // Ticket #4257 + check("bool foo() {\n" + "MyString *str=Getter();\n" + "return *str==\"bug\"; }\n", "test.c"); + ASSERT_EQUALS("[test.c:3]: (warning) String literal compared with variable '*str'. Did you intend to use strcmp() instead?\n", errout.str()); + + // Ticket #4257 + check("bool foo() {\n" + "MyString *str=Getter();\n" + "return *str==\"bug\"; }"); + ASSERT_EQUALS("", errout.str()); + + // Ticket #4257 + check("bool foo() {\n" + "MyString **str=OtherGetter();\n" + "return *str==\"bug\"; }"); + TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", + "", + errout.str()); + + // Ticket #4257 + check("bool foo() {\n" + "MyString str=OtherGetter2();\n" + "return &str==\"bug\"; }"); + TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", + "", + errout.str()); + + // Ticket #5734 + check("int foo(char c) {\n" + "return c == '42';}", "test.cpp"); + ASSERT_EQUALS("", errout.str()); + check("int foo(char c) {\n" + "return c == '42';}", "test.c"); + ASSERT_EQUALS("", errout.str()); + check("int foo(char c) {\n" + "return c == \"42\"[0];}", "test.cpp"); + ASSERT_EQUALS("", errout.str()); + check("int foo(char c) {\n" + "return c == \"42\"[0];}", "test.c"); + ASSERT_EQUALS("", errout.str()); + + // 5639 String literal compared with char buffer in a struct + check("struct Example {\n" + " char buffer[200];\n" + "};\n" + "void foo() {\n" + " struct Example example;\n" + " if (example.buffer == \"test\") ;\n" + "}\n", "test.cpp"); + ASSERT_EQUALS("[test.cpp:6]: (warning) String literal compared with variable 'example.buffer'. Did you intend to use strcmp() instead?\n", errout.str()); + check("struct Example {\n" + " char buffer[200];\n" + "};\n" + "void foo() {\n" + " struct Example example;\n" + " if (example.buffer == \"test\") ;\n" + "}\n", "test.c"); + ASSERT_EQUALS("[test.c:6]: (warning) String literal compared with variable 'example.buffer'. Did you intend to use strcmp() instead?\n", errout.str()); + } + + void suspiciousStringCompare_char() { + check("bool foo(char* c) {\n" + " return c == '\\0';\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Char literal compared with pointer 'c'. Did you intend to dereference it?\n", errout.str()); + + check("bool foo(char* c) {\n" + " return '\\0' != c;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Char literal compared with pointer 'c'. Did you intend to dereference it?\n", errout.str()); + + check("bool foo(char c) {\n" + " return c == '\\0';\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("bool foo(char c) {\n" + " return c == 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("bool foo(char* c) {\n" + " return *c == 0;\n" + "}", "test.c"); + ASSERT_EQUALS("", errout.str()); + + check("bool foo(char* c) {\n" + " return *c == 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("bool foo(Foo* c) {\n" + " return 0 == c->x;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(char* c) {\n" + " if(c == '\\0') bar();\n" + "}"); + TODO_ASSERT_EQUALS("[test.cpp:2]: (warning) Char literal compared with pointer 'c'. Did you intend to dereference it?\n", "", errout.str()); + + check("void f() {\n" + " struct { struct { char *str; } x; } a;\n" + " return a.x.str == '\\0';" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Char literal compared with pointer 'a.x.str'. Did you intend to dereference it?\n", errout.str()); + + check("void f() {\n" + " struct { struct { char *str; } x; } a;\n" + " return *a.x.str == '\\0';" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + + void sprintf1() { + check("void foo()\n" + "{\n" + " char buf[100];\n" + " sprintf(buf,\"%s\",buf);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Undefined behavior: Variable 'buf' is used as parameter and destination in s[n]printf().\n", errout.str()); + } + + void sprintf2() { + check("void foo()\n" + "{\n" + " char buf[100];\n" + " sprintf(buf,\"%i\",sizeof(buf));\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void sprintf3() { + check("void foo()\n" + "{\n" + " char buf[100];\n" + " sprintf(buf,\"%i\",sizeof(buf));\n" + " if (buf[0]);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void sprintf4() { + check("struct A\n" + "{\n" + " char filename[128];\n" + "};\n" + "\n" + "void foo()\n" + "{\n" + " const char* filename = \"hello\";\n" + " struct A a;\n" + " snprintf(a.filename, 128, \"%s\", filename);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void strPlusChar1() { + // Strange looking pointer arithmetic.. + check("void foo()\n" + "{\n" + " const char *p = \"/usr\" + '/';\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Unusual pointer arithmetic. A value of type 'char' is added to a string literal.\n", errout.str()); + } + + void strPlusChar2() { + // Strange looking pointer arithmetic.. + check("void foo()\n" + "{\n" + " char ch = 1;\n" + " const char *p = ch + \"/usr\";\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // Strange looking pointer arithmetic.. + check("void foo()\n" + "{\n" + " int i = 1;\n" + " const char* psz = \"Bla\";\n" + " const std::string str = i + psz;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void strPlusChar3() { + // Strange looking pointer arithmetic.. + check("void foo()\n" + "{\n" + " std::string temp = \"/tmp\";\n" + " std::string path = temp + '/' + \"sub\" + '/';\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void strPlusChar4() { + // don't crash + check("int test() { int +; }"); + } + + + void incorrectStringCompare() { + check("int f() {\n" + " return test.substr( 0 , 4 ) == \"Hello\" ? : 0 : 1 ;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str()); + + check("int f() {\n" + " return test.substr( 0 , 5 ) == \"Hello\" ? : 0 : 1 ;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int f() {\n" + " return \"Hello\" == test.substr( 0 , 4 ) ? : 0 : 1 ;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str()); + + check("int f() {\n" + " return \"Hello\" == foo.bar().z[1].substr(i+j*4, 4) ? : 0 : 1 ;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str()); + + check("int f() {\n" + " return \"Hello\" == test.substr( 0 , 5 ) ? : 0 : 1 ;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int f() {\n" + " if (\"Hello\") { }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); + + check("int f() {\n" + " if (\"Hello\" && test) { }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); + + check("int f() {\n" + " if (test && \"Hello\") { }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); + + check("int f() {\n" + " while (\"Hello\") { }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); + + check("int f() {\n" + " assert (test || \"Hello\");\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); + + check("int f() {\n" + " assert (test && \"Hello\");\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int f() {\n" + " assert (\"Hello\" || test);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); + + check("int f() {\n" + " assert (\"Hello\" && test);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int f() {\n" + " BOOST_ASSERT (\"Hello\" && test);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int f() {\n" + " return f2(\"Hello\");\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } +}; + +REGISTER_TEST(TestString) diff --git a/test/testutils.h b/test/testutils.h index 07f7031a0..352c1746b 100644 --- a/test/testutils.h +++ b/test/testutils.h @@ -44,4 +44,22 @@ public: } }; + +class SimpleSuppressor : public ErrorLogger { +public: + SimpleSuppressor(Settings &settings, ErrorLogger *next) + : _settings(settings), _next(next) { + } + virtual void reportOut(const std::string &outmsg) { + _next->reportOut(outmsg); + } + virtual void reportErr(const ErrorLogger::ErrorMessage &msg) { + if (!msg._callStack.empty() && !_settings.nomsg.isSuppressed(msg._id, msg._callStack.begin()->getfile(), msg._callStack.begin()->line)) + _next->reportErr(msg); + } +private: + Settings &_settings; + ErrorLogger *_next; +}; + #endif // TestUtilsH