From 43800a9419e205c57f58fe40656b697ffbd815e1 Mon Sep 17 00:00:00 2001 From: Dmitry-Me Date: Wed, 5 Aug 2015 12:20:28 +0300 Subject: [PATCH] Detect non-interlocked check after InterlockedDecrement() --- lib/checkother.cpp | 27 ++++++ lib/checkother.h | 8 ++ test/testother.cpp | 208 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 243 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index c907e9cb0..35fd109f0 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2770,3 +2770,30 @@ void CheckOther::checkLibraryMatchFunctions() } } } + +void CheckOther::checkInterlockedDecrement() +{ + if (!_settings->isWindowsPlatform()) { + return; + } + + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (Token::Match(tok, "InterlockedDecrement ( & %name% ) ; if ( %name%|!|0")) { + const Token* interlockedVarTok = tok->tokAt(3); + const Token* checkStartTok = interlockedVarTok->tokAt(5); + if ((Token::Match(checkStartTok, "0 %comp% %name% )") && checkStartTok->strAt(2) == interlockedVarTok->str()) || + (Token::Match(checkStartTok, "! %name% )") && checkStartTok->strAt(1) == interlockedVarTok->str()) || + (Token::Match(checkStartTok, "%name% )") && checkStartTok->str() == interlockedVarTok->str()) || + (Token::Match(checkStartTok, "%name% %comp% 0 )") && checkStartTok->str() == interlockedVarTok->str())) { + raceAfterInterlockedDecrementError(checkStartTok); + } + } + } +} + +void CheckOther::raceAfterInterlockedDecrementError(const Token* tok) +{ + reportError(tok, Severity::error, "raceAfterInterlockedDecrement", + "Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead."); +} + diff --git a/lib/checkother.h b/lib/checkother.h index 488103d19..15627c4bc 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -83,6 +83,7 @@ public: // --check-library : functions with nonmatching configuration checkOther.checkLibraryMatchFunctions(); + checkOther.checkInterlockedDecrement(); } /** @brief Run checks against the simplified token list */ @@ -238,6 +239,10 @@ public: /** @brief --check-library: warn for unconfigured function calls */ void checkLibraryMatchFunctions(); + void checkInterlockedDecrement(); + + void checkNewAfterDelete(); + private: // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result); @@ -293,6 +298,7 @@ private: void commaSeparatedReturnError(const Token *tok); void ignoredReturnValueError(const Token* tok, const std::string& function); void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive); + void raceAfterInterlockedDecrementError(const Token* tok); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckOther c(0, settings, errorLogger); @@ -306,6 +312,7 @@ private: c.invalidPointerCastError(0, "float", "double", false); c.negativeBitwiseShiftError(0); c.checkPipeParameterSizeError(0, "varname", "dimension"); + c.raceAfterInterlockedDecrementError(0); //performance c.redundantCopyError(0, "varname"); @@ -368,6 +375,7 @@ private: "- provide wrong dimensioned array to pipe() system command (--std=posix)\n" "- cast the return values of getc(),fgetc() and getchar() to character and compare it to EOF\n" "- invalid input values for functions\n" + "- race condition with non-interlocked access after InterlockedDecrement() call\n" // warning "- either division by zero or useless condition\n" diff --git a/test/testother.cpp b/test/testother.cpp index dc50a3379..4a899a2fc 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -166,6 +166,7 @@ private: TEST_CASE(redundantPointerOp); TEST_CASE(test_isSameExpression); + TEST_CASE(raceAfterInterlockedDecrement); } void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) { @@ -211,6 +212,13 @@ private: &settings); } + void checkInterlockedDecrement(const char code[]) { + Settings settings; + settings.platformType = Settings::Win32A; + + check(code, nullptr, false, false, true, &settings); + } + void check_preprocess_suppress(const char precode[], const char *filename = nullptr) { // Clear the error buffer.. errout.str(""); @@ -6228,6 +6236,206 @@ private: "};", "test.cpp", false, false); TODO_ASSERT_EQUALS("", "[test.cpp:1] -> [test.cpp:1]: (style) Same expression on both sides of '||'.\n", errout.str()); } + + void raceAfterInterlockedDecrement() { + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " whatever();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (counter)\n" + " return;\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (!counter)\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (counter > 0)\n" + " return;\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (0 < counter)\n" + " return;\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (counter == 0)\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (0 == counter)\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (0 != counter)\n" + " return;\n" + " destroy()\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (counter != 0)\n" + " return;\n" + " destroy()\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (counter <= 0)\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " InterlockedDecrement(&counter);\n" + " if (0 >= counter)\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Race condition: non-interlocked access after InterlockedDecrement(). Use InterlockedDecrement() return value instead.\n", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (newCount)\n" + " return;\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (!newCount)\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (newCount > 0)\n" + " return;\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (0 < newCount)\n" + " return;\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (newCount == 0)\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (0 == newCount)\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (0 != newCount)\n" + " return;\n" + " destroy()\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (newCount != 0)\n" + " return;\n" + " destroy()\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (newCount <= 0)\n" + " destroy();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + checkInterlockedDecrement( + "void f() {\n" + " int counter = 0;\n" + " int newCount = InterlockedDecrement(&counter);\n" + " if (0 >= newCount)\n" + " destroy;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)