Fixed #2550 (Bad substr/strncmp comparison)

lib/checkother.cpp

@@ -2718,6 +2718,40 @@ void CheckOther::checkMisusedScopedObject()
     }
 }
 
+void CheckOther::checkIncorrectStringCompare()
+{
+    for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next())
+    {
+        if (Token::Match(tok, ". substr ( %any% , %num% ) ==|!= %str%"))
+        {
+            size_t clen = MathLib::toLongNumber(tok->tokAt(5)->str());
+            size_t slen = Token::getStrLength(tok->tokAt(8));
+            if (clen != slen)
+            {
+                incorrectStringCompareError(tok->next(), "substr", tok->tokAt(8)->str(), tok->tokAt(5)->str());
+            }
+        }
+        if (Token::Match(tok, "%str% ==|!= %var% . substr ( %any% , %num% )"))
+        {
+            size_t clen = MathLib::toLongNumber(tok->tokAt(8)->str());
+            size_t slen = Token::getStrLength(tok);
+            if (clen != slen)
+            {
+                incorrectStringCompareError(tok->next(), "substr", tok->str(), tok->tokAt(8)->str());
+            }
+        }
+        if (Token::Match(tok, "strncmp ( %any% , %str% , %num% )"))
+        {
+            size_t clen = MathLib::toLongNumber(tok->tokAt(6)->str());
+            size_t slen = Token::getStrLength(tok->tokAt(4));
+            if (clen != slen)
+            {
+                incorrectStringCompareError(tok, "strncmp", tok->tokAt(4)->str(), tok->tokAt(6)->str());
+            }
+        }
+    }
+}
+
 void CheckOther::cstyleCastError(const Token *tok)
 {
     reportError(tok, Severity::style, "cstyleCast", "C-style pointer casting");
@@ -2935,3 +2969,8 @@ void CheckOther::memsetZeroBytesError(const Token *tok, const std::string &varna
     const std::string verbose(summary + ". Second and third arguments might be inverted.");
     reportError(tok, Severity::warning, "memsetZeroBytes", summary + "\n" + verbose);
 }
+
+void CheckOther::incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string, const std::string &len)
+{
+    reportError(tok, Severity::warning, "incorrectStringCompare", "String literal " + string + " doesn't match length argument for " + func + "(" + len + ").");
+}

lib/checkother.h

@@ -87,6 +87,7 @@ public:
         checkOther.checkMisusedScopedObject();
         checkOther.checkCatchExceptionByValue();
         checkOther.checkMemsetZeroBytes();
+        checkOther.checkIncorrectStringCompare();
     }
 
     /** @brief Clarify calculation for ".. a * b ? .." */
@@ -180,6 +181,9 @@ public:
     /** @brief %Check for using sizeof with array given as function argument */
     void checkSizeofForArrayParameter();
 
+    /** @brief %Check for using bad usage of strncmp and substr */
+    void checkIncorrectStringCompare();
+
     // Error messages..
     void cstyleCastError(const Token *tok);
     void dangerousUsageStrtolError(const Token *tok);
@@ -204,6 +208,7 @@ public:
     void catchExceptionByValueError(const Token *tok);
     void memsetZeroBytesError(const Token *tok, const std::string &varname);
     void sizeofForArrayParameterError(const Token *tok);
+    void incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string, const std::string &len);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings)
     {
@@ -243,6 +248,7 @@ public:
         c.catchExceptionByValueError(0);
         c.memsetZeroBytesError(0, "varname");
         c.clarifyCalculationError(0);
+        c.incorrectStringCompareError(0, "substr", "\"Hello World\"", "12");
     }
 
     std::string myName() const
@@ -261,6 +267,7 @@ public:
                "* scoped object destroyed immediately after construction\n"
                "* assignment in an assert statement\n"
                "* sizeof for array given as function argument\n"
+               "* incorrect length arguments for 'substr' and 'strncmp'\n"
 
                // style
                "* C-style pointer cast in cpp file\n"

test/testother.cpp

@@ -102,6 +102,8 @@ private:
         TEST_CASE(sizeofForArrayParameter);
 
         TEST_CASE(clarifyCalculation);
+
+        TEST_CASE(incorrectStringCompare);
     }
 
     void check(const char code[], const char *filename = NULL)
@@ -138,6 +140,7 @@ private:
         checkOther.checkCatchExceptionByValue();
         checkOther.checkMemsetZeroBytes();
         checkOther.clarifyCalculation();
+        checkOther.checkIncorrectStringCompare();
     }
 
 
@@ -1856,6 +1859,39 @@ private:
         ASSERT_EQUALS("[test.cpp:2]: (information) Please clarify precedence: 'a*b?..'\n", 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(4).\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(4).\n", errout.str());
+
+        check("int f() {\n"
+              " return \"Hello\" == test.substr( 0 , 5 ) ? : 0 : 1 ;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("int f() {\n"
+              "    return strncmp(\"test\" , \"test\" , 2) ; \n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"test\" doesn't match length argument for strncmp(2).\n", errout.str());
+
+        check("int f() {\n"
+              "    return strncmp(\"test\" , \"test\" , 4) ; \n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+    }
+
 };
 
 REGISTER_TEST(TestOther)