Checkstring fixes (#1783)

* teststring.cpp: Fix ternary syntax in tests

* stringLiteralWrite: Add tests wide character and utf16 strings

* suspiciousStringCompare: Add test with wide character string

* strPlusChar: Handle wide characters

* incorrectStringCompare: Add test with wide string

* Suspicious string compare: suggest wcscmp for wide strings

* deadStrcmp: Extend to handle wide strings

* sprintfOverlappingData: Print name of strcmp function

* Conversion of char literal to boolean, add wide character tests

* Conversion of char literal to boolean, fix ternary

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<const Token *> args1 = getArguments(eq0->previous());
                     const std::vector<const Token *> 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"
+                "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 "
+                "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) "
+                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);

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\'");

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<int>().z[1].substr(i+j*4, 4) ? : 0 : 1 ;\n"
+              "    return \"Hello\" == foo.bar<int>().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"