diff --git a/lib/checkstring.cpp b/lib/checkstring.cpp index 3e80d3048..78118ea30 100644 --- a/lib/checkstring.cpp +++ b/lib/checkstring.cpp @@ -213,7 +213,7 @@ void CheckString::checkSuspiciousStringCompare() const bool ischar(litTok->tokType() == Token::eChar); if (litTok->tokType() == Token::eString) { if (mTokenizer->isC() || (var && var->isArrayOrPointer())) - suspiciousStringCompareError(tok, varname); + suspiciousStringCompareError(tok, varname, litTok->isLong()); } else if (ischar && var && var->isPointer()) { suspiciousStringCompareError_char(tok, varname); } @@ -221,10 +221,11 @@ void CheckString::checkSuspiciousStringCompare() } } -void CheckString::suspiciousStringCompareError(const Token* tok, const std::string& var) +void CheckString::suspiciousStringCompareError(const Token* tok, const std::string& var, bool isLong) { + const std::string cmpFunc = isLong ? "wcscmp" : "strcmp"; reportError(tok, Severity::warning, "literalWithCharPtrCompare", - "$symbol:" + var + "\nString literal compared with variable '$symbol'. Did you intend to use strcmp() instead?", CWE595, false); + "$symbol:" + var + "\nString literal compared with variable '$symbol'. Did you intend to use " + cmpFunc + "() instead?", CWE595, false); } void CheckString::suspiciousStringCompareError_char(const Token* tok, const std::string& var) @@ -240,7 +241,7 @@ void CheckString::suspiciousStringCompareError_char(const Token* tok, const std: static bool isChar(const Variable* var) { - return (var && !var->isPointer() && !var->isArray() && var->typeStartToken()->str() == "char"); + return (var && !var->isPointer() && !var->isArray() && (var->typeStartToken()->str() == "char" || var->typeStartToken()->str() == "wchar_t")); } void CheckString::strPlusChar() @@ -260,7 +261,12 @@ void CheckString::strPlusChar() 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.", CWE665, false); + std::string charType = "char"; + if (tok && tok->astOperand2() && tok->astOperand2()->variable()) + charType = tok->astOperand2()->variable()->typeStartToken()->str(); + else if (tok && tok->astOperand2() && tok->astOperand2()->tokType() == Token::eChar && tok->astOperand2()->isLong()) + charType = "wchar_t"; + reportError(tok, Severity::error, "strPlusChar", "Unusual pointer arithmetic. A value of type '" + charType +"' is added to a string literal.", CWE665, false); } //--------------------------------------------------------------------------- @@ -309,7 +315,8 @@ void CheckString::checkIncorrectStringCompare() incorrectStringBooleanError(tok->next(), tok->strAt(1)); } else if (Token::Match(tok, "if|while ( %str%|%char% )") && !tok->tokAt(2)->getValue(0)) { incorrectStringBooleanError(tok->tokAt(2), tok->strAt(2)); - } + } else if (tok->str() == "?" && Token::Match(tok->astOperand1(), "%str%|%char%")) + incorrectStringBooleanError(tok->astOperand1(), tok->astOperand1()->str()); } } } @@ -380,9 +387,9 @@ void CheckString::overlappingStrcmp() for (const Token *eq0 : equals0) { for (const Token * ne0 : notEquals0) { - if (!Token::simpleMatch(eq0->previous(), "strcmp (")) + if (!Token::Match(eq0->previous(), "strcmp|wcscmp (")) continue; - if (!Token::simpleMatch(ne0->previous(), "strcmp (")) + if (!Token::Match(ne0->previous(), "strcmp|wcscmp (")) continue; const std::vector args1 = getArguments(eq0->previous()); const std::vector args2 = getArguments(ne0->previous()); @@ -445,20 +452,22 @@ void CheckString::sprintfOverlappingData() true, false); if (same) { - sprintfOverlappingDataError(args[argnr], args[argnr]->expressionString()); + sprintfOverlappingDataError(tok, args[argnr], args[argnr]->expressionString()); } } } } } -void CheckString::sprintfOverlappingDataError(const Token *tok, const std::string &varname) +void CheckString::sprintfOverlappingDataError(const Token *funcTok, const Token *tok, const std::string &varname) { + const std::string func = funcTok ? funcTok->str() : "s[n]printf"; + reportError(tok, Severity::error, "sprintfOverlappingData", "$symbol:" + varname + "\n" - "Undefined behavior: Variable '$symbol' is used as parameter and destination in s[n]printf().\n" - "The variable '$symbol' is used both as a parameter and as destination in " - "s[n]printf(). The origin and destination buffers overlap. Quote from glibc (C-library) " + "Undefined behavior: Variable '$symbol' is used as parameter and destination in " + func + "().\n" + + "The variable '$symbol' is used both as a parameter and as destination in " + + func + "(). 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.\"", CWE628, false); diff --git a/lib/checkstring.h b/lib/checkstring.h index d878b4783..86841e2e6 100644 --- a/lib/checkstring.h +++ b/lib/checkstring.h @@ -86,13 +86,13 @@ public: private: void stringLiteralWriteError(const Token *tok, const Token *strValue); - void sprintfOverlappingDataError(const Token *tok, const std::string &varname); + void sprintfOverlappingDataError(const Token *funcTok, 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(const Token* tok, const std::string& var, bool isLong); void suspiciousStringCompareError_char(const Token* tok, const std::string& var); void overlappingStrcmpError(const Token* eq0, const Token *ne0); @@ -100,10 +100,10 @@ private: CheckString c(nullptr, settings, errorLogger); c.stringLiteralWriteError(nullptr, nullptr); - c.sprintfOverlappingDataError(nullptr, "varname"); + c.sprintfOverlappingDataError(nullptr, nullptr, "varname"); c.strPlusCharError(nullptr); c.incorrectStringCompareError(nullptr, "substr", "\"Hello World\""); - c.suspiciousStringCompareError(nullptr, "foo"); + c.suspiciousStringCompareError(nullptr, "foo", false); c.suspiciousStringCompareError_char(nullptr, "foo"); c.incorrectStringBooleanError(nullptr, "\"Hello World\""); c.incorrectStringBooleanError(nullptr, "\'x\'"); diff --git a/test/teststring.cpp b/test/teststring.cpp index c27f7f54f..e68485140 100644 --- a/test/teststring.cpp +++ b/test/teststring.cpp @@ -44,11 +44,14 @@ private: TEST_CASE(strPlusChar1); // "/usr" + '/' TEST_CASE(strPlusChar2); // "/usr" + ch TEST_CASE(strPlusChar3); // ok: path + "/sub" + '/' + TEST_CASE(strPlusChar4); // L"/usr" + L'/' + TEST_CASE(snprintf1); // Dangerous usage of snprintf TEST_CASE(sprintf1); // Dangerous usage of sprintf TEST_CASE(sprintf2); TEST_CASE(sprintf3); TEST_CASE(sprintf4); // struct member + TEST_CASE(wsprintf1); // Dangerous usage of wsprintf TEST_CASE(incorrectStringCompare); @@ -105,6 +108,18 @@ private: " foo_FP1(s);\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " wchar_t *abc = L\"abc\";\n" + " abc[0] = u'a';\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (error) Modifying string literal \"abc\" directly or indirectly is undefined behaviour.\n", errout.str()); + + check("void f() {\n" + " char16_t *abc = u\"abc\";\n" + " abc[0] = 'a';\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (error) Modifying string literal \"abc\" directly or indirectly is undefined behaviour.\n", errout.str()); } void alwaysTrueFalseStringCompare() { @@ -263,6 +278,11 @@ private: "}"); 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(wchar_t* c) {\n" + " return c == L\"x\";\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use wcscmp() instead?\n", errout.str()); + check("bool foo(const char* c) {\n" " return \"x\" == c;\n" "}"); @@ -370,6 +390,11 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (warning) Char literal compared with pointer 'c'. Did you intend to dereference it?\n", errout.str()); + check("bool foo(wchar_t* c) {\n" + " return c == L'x';\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" "}"); @@ -419,13 +444,22 @@ private: } + void snprintf1() { + check("void foo()\n" + "{\n" + " char buf[100];\n" + " snprintf(buf,100,\"%s\",buf);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Undefined behavior: Variable 'buf' is used as parameter and destination in snprintf().\n", 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()); + ASSERT_EQUALS("[test.cpp:4]: (error) Undefined behavior: Variable 'buf' is used as parameter and destination in sprintf().\n", errout.str()); } void sprintf2() { @@ -462,6 +496,15 @@ private: ASSERT_EQUALS("", errout.str()); } + void wsprintf1() { + check("void foo()\n" + "{\n" + " wchar_t buf[100];\n" + " swprintf(buf,10, \"%s\",buf);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Undefined behavior: Variable 'buf' is used as parameter and destination in swprintf().\n", errout.str()); + } + void strPlusChar1() { // Strange looking pointer arithmetic.. check("void foo()\n" @@ -500,30 +543,49 @@ private: ASSERT_EQUALS("", errout.str()); } + void strPlusChar4() { + // Strange looking pointer arithmetic, wide char.. + check("void foo()\n" + "{\n" + " const wchar_t *p = L\"/usr\" + L'/';\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Unusual pointer arithmetic. A value of type 'wchar_t' is added to a string literal.\n", errout.str()); + + check("void foo(wchar_t c)\n" + "{\n" + " const wchar_t *p = L\"/usr\" + c;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Unusual pointer arithmetic. A value of type 'wchar_t' is added to a string literal.\n", errout.str()); + } void incorrectStringCompare() { check("int f() {\n" - " return test.substr( 0 , 4 ) == \"Hello\" ? : 0 : 1 ;\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" + " return test.substr( 0 , 4 ) == L\"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" + " 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" + " 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" + " return \"Hello\" == test.substr( 0 , 5 ) ? 0 : 1 ;\n" "}"); ASSERT_EQUALS("", errout.str()); @@ -547,6 +609,11 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str()); + check("int f() {\n" + " return \"Hello\" ? 1 : 2;\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" "}"); @@ -582,22 +649,26 @@ private: " if('a'){}\n" " if(L'b'){}\n" " if(1 && 'c'){}\n" - " int x = 'd' ? 1 : 2;\n" // <- TODO + " int x = 'd' ? 1 : 2;\n" "}"); ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of char literal 'a' to bool always evaluates to true.\n" "[test.cpp:3]: (warning) Conversion of char literal 'b' to bool always evaluates to true.\n" "[test.cpp:4]: (warning) Conversion of char literal 'c' to bool always evaluates to true.\n" + "[test.cpp:5]: (warning) Conversion of char literal 'd' to bool always evaluates to true.\n" , errout.str()); check("void f() {\n" " if('\\0'){}\n" + " if(L'\\0'){}\n" "}"); ASSERT_EQUALS("", errout.str()); check("void f() {\n" " if('\\0' || cond){}\n" + " if(L'\\0' || cond){}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of char literal '\\0' to bool always evaluates to false.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of char literal '\\0' to bool always evaluates to false.\n" + "[test.cpp:3]: (warning) Conversion of char literal '\\0' to bool always evaluates to false.\n", errout.str()); } void deadStrcmp() { @@ -606,6 +677,11 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (warning) The expression 'strcmp(str,\"def\") != 0' is suspicious. It overlaps 'strcmp(str,\"abc\") == 0'.\n", errout.str()); + check("void f(const wchar_t *str) {\n" + " if (wcscmp(str, L\"abc\") == 0 || wcscmp(str, L\"def\")) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) The expression 'wcscmp(str,L\"def\") != 0' is suspicious. It overlaps 'wcscmp(str,L\"abc\") == 0'.\n", errout.str()); + check("struct X {\n" " char *str;\n" "};\n"