From aeae5a867d2e9f5b0077f14a6cd9ca4a6773ecba Mon Sep 17 00:00:00 2001 From: Erik Lax Date: Tue, 8 Feb 2011 19:49:29 +0100 Subject: [PATCH] Fixed #2550 (Bad substr/strncmp comparison) --- lib/checkother.cpp | 39 +++++++++++++++++++++++++++++++++++++++ lib/checkother.h | 7 +++++++ test/testother.cpp | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index dff996088..891502d6e 100644 --- a/lib/checkother.cpp +++ b/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 + ")."); +} diff --git a/lib/checkother.h b/lib/checkother.h index 0ca251491..07df64867 100644 --- a/lib/checkother.h +++ b/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" diff --git a/test/testother.cpp b/test/testother.cpp index 864cf25a1..f5adf548d 100644 --- a/test/testother.cpp +++ b/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)