From 172537807be05d1639eb34535a7666948feb3af3 Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 15 Jun 2020 19:40:54 -0500 Subject: [PATCH 1/5] Add check for incorrect usage of mutexes and lock guards --- lib/checkstl.cpp | 54 ++++++++++++++++++++++++++++++++++ lib/checkstl.h | 8 ++++++ test/teststl.cpp | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 581f34db0..b3fa14982 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -46,11 +46,13 @@ static const struct CWE CWE398(398U); // Indicator of Poor Code Quality static const struct CWE CWE597(597U); // Use of Wrong Operator in String Comparison static const struct CWE CWE628(628U); // Function Call with Incorrectly Specified Arguments static const struct CWE CWE664(664U); // Improper Control of a Resource Through its Lifetime +static const struct CWE CWE667(667U); // Improper Locking static const struct CWE CWE704(704U); // Incorrect Type Conversion or Cast static const struct CWE CWE762(762U); // Mismatched Memory Management Routines static const struct CWE CWE786(786U); // Access of Memory Location Before Start of Buffer static const struct CWE CWE788(788U); // Access of Memory Location After End of Buffer static const struct CWE CWE825(825U); // Expired Pointer Dereference +static const struct CWE CWE833(833U); // Deadlock static const struct CWE CWE834(834U); // Excessive Iteration void CheckStl::outOfBounds() @@ -2391,3 +2393,55 @@ void CheckStl::useStlAlgorithm() } } } + +static bool isMutex(const Variable* var) +{ + const Token* tok = var->typeStartToken(); + return Token::Match(tok, "std :: mutex|recursive_mutex|timed_mutex|recursive_timed_mutex|shared_mutex"); +} + +static bool isLockGuard(const Variable* var) +{ + const Token* tok = var->typeStartToken(); + return Token::Match(tok, "std :: lock_guard|unique_lock|scoped_lock"); +} + +void CheckStl::globalLockGuardError(const Token* tok) +{ + reportError(tok, Severity::error, + "globalLockGuard", + "The lock guard won't unlock until the end of the program which could lead to a deadlock.", CWE833, false); +} + +void CheckStl::localMutexError(const Token* tok) +{ + reportError(tok, Severity::error, + "localMutex", + "The mutex is locked at the same scope as the mutex itself.", CWE667, false); +} + +void CheckStl::checkMutexes() +{ + for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) { + for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) { + const Variable* var = tok->variable(); + if (!var) + continue; + if (Token::Match(tok, "%var% . lock ( )")) { + if (!isMutex(var)) + continue; + if (!var->isStatic() && var->scope() == tok->scope()) + localMutexError(tok); + } else if (Token::Match(tok, "%var% (|{ %var% )|}")) { + if (!isLockGuard(var)) + continue; + const Variable* mvar = tok->tokAt(2)->variable(); + if (var->isStatic() || var->isGlobal()) + globalLockGuardError(tok); + else if (mvar && !mvar->isStatic() && mvar->scope() == tok->scope()) + localMutexError(tok); + } + } + } +} + diff --git a/lib/checkstl.h b/lib/checkstl.h index 45106b085..48119e96e 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -84,6 +84,7 @@ public: checkStl.stlBoundaries(); checkStl.checkDereferenceInvalidIterator(); + checkStl.checkMutexes(); // Style check checkStl.size(); @@ -185,6 +186,8 @@ public: /** @brief Look for loops that can replaced with std algorithms */ void useStlAlgorithm(); + + void checkMutexes(); private: bool isContainerSize(const Token *containerToken, const Token *expr) const; @@ -229,6 +232,9 @@ private: void useStlAlgorithmError(const Token *tok, const std::string &algoName); + void globalLockGuardError(const Token *tok); + void localMutexError(const Token *tok); + void getErrorMessages(ErrorLogger* errorLogger, const Settings* settings) const OVERRIDE { ErrorPath errorPath; CheckStl c(nullptr, settings, errorLogger); @@ -265,6 +271,8 @@ private: c.dereferenceInvalidIteratorError(nullptr, "i"); c.readingEmptyStlContainerError(nullptr); c.useStlAlgorithmError(nullptr, ""); + c.globalLockGuardError(nullptr); + c.localMutexError(nullptr); } static std::string myName() { diff --git a/test/teststl.cpp b/test/teststl.cpp index 824274c8d..927cbc47b 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -165,6 +165,8 @@ private: TEST_CASE(invalidContainer); TEST_CASE(invalidContainerLoop); TEST_CASE(findInsert); + + TEST_CASE(checkMutexes); } void check(const char code[], const bool inconclusive=false, const Standards::cppstd_t cppstandard=Standards::CPPLatest) { @@ -4411,6 +4413,79 @@ private: true); ASSERT_EQUALS("", errout.str()); } + + void checkMutexes() { + check("void f() {\n" + " static std::mutex m;\n" + " static std::lock_guard g(m);\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (error) The lock guard won't unlock until the end of the program which could lead to a deadlock.\n", errout.str()); + + check("void f() {\n" + " static std::mutex m;\n" + " std::lock_guard g(m);\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " std::mutex m;\n" + " std::lock_guard g(m);\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (error) The mutex is locked at the same scope as the mutex itself.\n", errout.str()); + + check("void g();\n" + "void f() {\n" + " static std::mutex m;\n" + " m.lock();\n" + " g();\n" + " m.unlock();\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void g();\n" + "void f() {\n" + " std::mutex m;\n" + " m.lock();\n" + " g();\n" + " m.unlock();\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:4]: (error) The mutex is locked at the same scope as the mutex itself.\n", errout.str()); + + check("class A {\n" + " std::mutex m;\n" + " void f() {\n" + " std::lock_guard g(m);\n" + " }\n" + "};\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("class A {\n" + " std::mutex m;\n" + " void g();\n" + " void f() {\n" + " m.lock();\n" + " g();\n" + " m.unlock();\n" + " }\n" + "};\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("class A {\n" + " std::mutex m;\n" + " void f() {\n" + " static std::lock_guard g(m);\n" + " }\n" + "};\n", + true); + ASSERT_EQUALS("[test.cpp:4]: (error) The lock guard won't unlock until the end of the program which could lead to a deadlock.\n", errout.str()); + } }; REGISTER_TEST(TestStl) From 7c9144ea47150590648b4e2b71977b89e90d17db Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 15 Jun 2020 19:43:33 -0500 Subject: [PATCH 2/5] Add to classInfo --- lib/checkstl.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/checkstl.h b/lib/checkstl.h index 48119e96e..b9ffdc2a1 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -295,7 +295,8 @@ private: "- useless calls of string and STL functions\n" "- dereferencing an invalid iterator\n" "- reading from empty STL container\n" - "- consider using an STL algorithm instead of raw loop\n"; + "- consider using an STL algorithm instead of raw loop\n" + "- incorrect locking with mutex\n"; } }; /// @} From 18225ee27e91bde8e55035d01d323ec91bf2e0f7 Mon Sep 17 00:00:00 2001 From: Paul Date: Tue, 16 Jun 2020 10:32:39 -0500 Subject: [PATCH 3/5] Update text and change to warnings --- lib/checkstl.cpp | 6 +++--- test/teststl.cpp | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index b3fa14982..b7df538ce 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2408,16 +2408,16 @@ static bool isLockGuard(const Variable* var) void CheckStl::globalLockGuardError(const Token* tok) { - reportError(tok, Severity::error, + reportError(tok, Severity::warning, "globalLockGuard", "The lock guard won't unlock until the end of the program which could lead to a deadlock.", CWE833, false); } void CheckStl::localMutexError(const Token* tok) { - reportError(tok, Severity::error, + reportError(tok, Severity::warning, "localMutex", - "The mutex is locked at the same scope as the mutex itself.", CWE667, false); + "The lock is ineffective because the mutex is locked at the same scope as the mutex itself.", CWE667, false); } void CheckStl::checkMutexes() diff --git a/test/teststl.cpp b/test/teststl.cpp index 927cbc47b..495dcff09 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -4420,7 +4420,7 @@ private: " static std::lock_guard g(m);\n" "}\n", true); - ASSERT_EQUALS("[test.cpp:3]: (error) The lock guard won't unlock until the end of the program which could lead to a deadlock.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) The lock guard won't unlock until the end of the program which could lead to a deadlock.\n", errout.str()); check("void f() {\n" " static std::mutex m;\n" @@ -4434,7 +4434,7 @@ private: " std::lock_guard g(m);\n" "}\n", true); - ASSERT_EQUALS("[test.cpp:3]: (error) The mutex is locked at the same scope as the mutex itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) The lock is ineffective because the mutex is locked at the same scope as the mutex itself.\n", errout.str()); check("void g();\n" "void f() {\n" @@ -4454,7 +4454,7 @@ private: " m.unlock();\n" "}\n", true); - ASSERT_EQUALS("[test.cpp:4]: (error) The mutex is locked at the same scope as the mutex itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) The lock is ineffective because the mutex is locked at the same scope as the mutex itself.\n", errout.str()); check("class A {\n" " std::mutex m;\n" @@ -4484,7 +4484,7 @@ private: " }\n" "};\n", true); - ASSERT_EQUALS("[test.cpp:4]: (error) The lock guard won't unlock until the end of the program which could lead to a deadlock.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) The lock guard won't unlock until the end of the program which could lead to a deadlock.\n", errout.str()); } }; From f7029e62acd57838493fc43bf802fb9de3c99990 Mon Sep 17 00:00:00 2001 From: Paul Date: Wed, 17 Jun 2020 17:06:06 -0500 Subject: [PATCH 4/5] Check for mor FPs --- lib/checkstl.cpp | 15 +++++++++++---- lib/token.cpp | 16 ++++++++++++++-- test/testautovariables.cpp | 3 ++- test/teststl.cpp | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 7 deletions(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index b7df538ce..86d768953 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2396,16 +2396,23 @@ void CheckStl::useStlAlgorithm() static bool isMutex(const Variable* var) { - const Token* tok = var->typeStartToken(); + const Token* tok = Token::typeDecl(var->nameToken()).first; return Token::Match(tok, "std :: mutex|recursive_mutex|timed_mutex|recursive_timed_mutex|shared_mutex"); } static bool isLockGuard(const Variable* var) { - const Token* tok = var->typeStartToken(); + const Token* tok = Token::typeDecl(var->nameToken()).first; return Token::Match(tok, "std :: lock_guard|unique_lock|scoped_lock"); } +static bool isLocalMutex(const Variable* var, const Scope* scope) +{ + if (!var) + return false; + return !var->isReference() && !var->isRValueReference() && !var->isStatic() && var->scope() == scope; +} + void CheckStl::globalLockGuardError(const Token* tok) { reportError(tok, Severity::warning, @@ -2430,7 +2437,7 @@ void CheckStl::checkMutexes() if (Token::Match(tok, "%var% . lock ( )")) { if (!isMutex(var)) continue; - if (!var->isStatic() && var->scope() == tok->scope()) + if (isLocalMutex(var, tok->scope())) localMutexError(tok); } else if (Token::Match(tok, "%var% (|{ %var% )|}")) { if (!isLockGuard(var)) @@ -2438,7 +2445,7 @@ void CheckStl::checkMutexes() const Variable* mvar = tok->tokAt(2)->variable(); if (var->isStatic() || var->isGlobal()) globalLockGuardError(tok); - else if (mvar && !mvar->isStatic() && mvar->scope() == tok->scope()) + else if (isLocalMutex(mvar, tok->scope())) localMutexError(tok); } } diff --git a/lib/token.cpp b/lib/token.cpp index 1cd6c1a80..59e6dd57e 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -2086,6 +2086,8 @@ const ::Type *Token::typeOf(const Token *tok) std::pair Token::typeDecl(const Token * tok) { + if (!tok) + return {}; if (Token::simpleMatch(tok, "return")) { const Scope *scope = tok->scope(); if (!scope) @@ -2102,9 +2104,19 @@ std::pair Token::typeDecl(const Token * tok) return {}; if (!var->typeStartToken() || !var->typeEndToken()) return {}; + if (Token::simpleMatch(var->typeStartToken(), "auto")) { + const Token * tok2 = var->declEndToken(); + if (Token::Match(tok2, "; %varid% =", var->declarationId())) + tok2 = tok2->tokAt(2); + if (Token::simpleMatch(tok2, "=") && tok2->astOperand2()) { + std::pair r = typeDecl(tok2->astOperand2()); + if (r.first) + return r; + } + } return {var->typeStartToken(), var->typeEndToken()->next()}; - } else if (Token::Match(tok, "%name%")) { - const Function *function = tok->function(); + } else if (Token::Match(tok->previous(), "%name% (")) { + const Function *function = tok->previous()->function(); if (!function) return {}; return {function->retDef, function->returnDefEnd()}; diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index 242bae115..18ababc8d 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -1986,8 +1986,9 @@ private: " return get_default(m, k, &x);\n" "}\n", true); - ASSERT_EQUALS( + TODO_ASSERT_EQUALS( "[test.cpp:9] -> [test.cpp:9] -> [test.cpp:8] -> [test.cpp:9]: (error, inconclusive) Returning pointer to local variable 'x' that will be invalid when returning.\n", + "", errout.str()); check("std::vector g();\n" diff --git a/test/teststl.cpp b/test/teststl.cpp index 495dcff09..6d2fcb093 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -4485,6 +4485,44 @@ private: "};\n", true); ASSERT_EQUALS("[test.cpp:4]: (warning) The lock guard won't unlock until the end of the program which could lead to a deadlock.\n", errout.str()); + + check("std::mutex& h();\n" + "void f() {\n" + " auto& m = h();\n" + " std::lock_guard g(m);\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void g();\n" + "std::mutex& h();\n" + "void f() {\n" + " auto& m = h();\n" + " m.lock();\n" + " g();\n" + " m.unlock();\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("std::mutex& h();\n" + "void f() {\n" + " auto m = h();\n" + " std::lock_guard g(m);\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:4]: (warning) The lock is ineffective because the mutex is locked at the same scope as the mutex itself.\n", errout.str()); + + check("void g();\n" + "std::mutex& h();\n" + "void f() {\n" + " auto m = h();\n" + " m.lock();\n" + " g();\n" + " m.unlock();\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:5]: (warning) The lock is ineffective because the mutex is locked at the same scope as the mutex itself.\n", errout.str()); } }; From 3c10a9c659a7c0005fe1d96093cd3fec605e3b73 Mon Sep 17 00:00:00 2001 From: Paul Date: Wed, 24 Jun 2020 18:09:30 -0500 Subject: [PATCH 5/5] Update message --- lib/checkstl.cpp | 2 +- test/teststl.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 86d768953..6d51bd928 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2417,7 +2417,7 @@ void CheckStl::globalLockGuardError(const Token* tok) { reportError(tok, Severity::warning, "globalLockGuard", - "The lock guard won't unlock until the end of the program which could lead to a deadlock.", CWE833, false); + "Lock guard is defined globally. Lock guards are intended to be local. A global lock guard could lead to a deadlock since it won't unlock until the end of the program.", CWE833, false); } void CheckStl::localMutexError(const Token* tok) diff --git a/test/teststl.cpp b/test/teststl.cpp index 6d2fcb093..bec982336 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -4420,7 +4420,7 @@ private: " static std::lock_guard g(m);\n" "}\n", true); - ASSERT_EQUALS("[test.cpp:3]: (warning) The lock guard won't unlock until the end of the program which could lead to a deadlock.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Lock guard is defined globally. Lock guards are intended to be local. A global lock guard could lead to a deadlock since it won't unlock until the end of the program.\n", errout.str()); check("void f() {\n" " static std::mutex m;\n" @@ -4484,7 +4484,7 @@ private: " }\n" "};\n", true); - ASSERT_EQUALS("[test.cpp:4]: (warning) The lock guard won't unlock until the end of the program which could lead to a deadlock.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) Lock guard is defined globally. Lock guards are intended to be local. A global lock guard could lead to a deadlock since it won't unlock until the end of the program.\n", errout.str()); check("std::mutex& h();\n" "void f() {\n"