New check: detect suspicious comparison of string literal with char* variable

Bugfix: Update Token type when varId is set

lib/checkother.cpp

@@ -2834,6 +2834,44 @@ void CheckOther::alwaysTrueStringVariableCompareError(const Token *tok, const st
                 "This could be a logic bug.");
 }
 
+
+//-----------------------------------------------------------------------------
+//-----------------------------------------------------------------------------
+void CheckOther::checkSuspiciousStringCompare()
+{
+    if (!_settings->isEnabled("style"))
+        return;
+
+    const SymbolDatabase* symbolDatabase = _tokenizer->getSymbolDatabase();
+
+    for (const Token* tok = _tokenizer->list.front(); tok && tok->tokAt(3); tok = tok->next()) {
+        if (tok->next()->type() != Token::eComparisonOp)
+            continue;
+
+        const Token* varTok = tok;
+        const Token* litTok = tok->tokAt(2);
+
+        if (varTok->strAt(-1) == "+" || litTok->strAt(1) == "+")
+            continue;
+
+        if ((varTok->type() == Token::eString || varTok->type() == Token::eVariable) && (litTok->type() == Token::eString || litTok->type() == Token::eVariable) && litTok->type() != varTok->type()) {
+            if (varTok->type() == Token::eString)
+                std::swap(varTok, litTok);
+
+            const Variable* var = symbolDatabase->getVariableFromVarId(varTok->varId());
+            if (var && var->isPointer())
+                suspiciousStringCompareError(tok, var->name());
+        }
+    }
+}
+
+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::checkModuloAlwaysTrueFalse()

lib/checkother.h

@@ -75,6 +75,7 @@ public:
         checkOther.checkComparisonOfBoolExpressionWithInt();
         checkOther.checkSignOfUnsignedVariable();  // don't ignore casts (#3574)
         checkOther.checkIncompleteArrayFill();
+        checkOther.checkSuspiciousStringCompare();
     }
 
     /** @brief Run checks against the simplified token list */
@@ -200,6 +201,9 @@ public:
     /** @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 using postfix increment on bool */
     void checkIncrementBoolean();
 
@@ -297,6 +301,7 @@ private:
     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 duplicateBreakError(const Token *tok, bool inconclusive);
     void unreachableCodeError(const Token* tok, bool inconclusive);
     void assignBoolToPointerError(const Token *tok);
@@ -356,6 +361,7 @@ private:
         c.clarifyConditionError(0, true, false);
         c.clarifyStatementError(0);
         c.incorrectStringCompareError(0, "substr", "\"Hello World\"", "12");
+        c.suspiciousStringCompareError(0, "foo");
         c.incorrectStringBooleanError(0, "\"Hello World\"");
         c.incrementBooleanError(0);
         c.comparisonOfBoolWithIntError(0, "varname", true);
@@ -427,6 +433,7 @@ private:
                "* 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"
                "* duplicate break statement\n"
                "* unreachable code\n"
                "* testing if unsigned variable is negative\n"

lib/token.h

@@ -301,6 +301,8 @@ public:
     }
     void varId(unsigned int id) {
         _varId = id;
+        if (id != 0)
+            _type = eVariable;
     }
 
     /**

test/testother.cpp

@@ -153,6 +153,7 @@ private:
         TEST_CASE(duplicateExpression5); // ticket #3749 (macros with same values)
 
         TEST_CASE(alwaysTrueFalseStringCompare);
+        TEST_CASE(suspiciousStringCompare);
         TEST_CASE(checkPointerSizeof);
         TEST_CASE(checkSignOfUnsignedVariable);
         TEST_CASE(checkSignOfPointer);
@@ -4824,6 +4825,33 @@ private:
         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"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool foo(const std::string& c) {\n"
+              "    return \"x\" == c;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void checkPointerSizeof() {
         check(
             "char *x = malloc(10);\n"