From caabe56f14ad1daf2fa8eadeb61629d3a1d1c939 Mon Sep 17 00:00:00 2001 From: Paul Date: Sun, 12 Jul 2020 21:31:53 -0500 Subject: [PATCH 1/2] Handle FPs: mutexes being locked at different scopes --- lib/checkstl.cpp | 9 +++++++++ test/teststl.cpp | 30 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index ebeb7f44f..15bf0ac70 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2430,19 +2430,28 @@ void CheckStl::localMutexError(const Token* tok) void CheckStl::checkMutexes() { for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) { + std::set checkedVars; for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) { + if (!Token::Match(tok, "%var%")) + continue; const Variable* var = tok->variable(); if (!var) continue; if (Token::Match(tok, "%var% . lock ( )")) { if (!isMutex(var)) continue; + if (!checkedVars.insert(var->declarationId()).second) + continue; if (isLocalMutex(var, tok->scope())) localMutexError(tok); } else if (Token::Match(tok, "%var% (|{ %var% )|}|,")) { if (!isLockGuard(var)) continue; const Variable* mvar = tok->tokAt(2)->variable(); + if (!mvar) + continue; + if (!checkedVars.insert(mvar->declarationId()).second) + continue; if (var->isStatic() || var->isGlobal()) globalLockGuardError(tok); else if (isLocalMutex(mvar, tok->scope())) diff --git a/test/teststl.cpp b/test/teststl.cpp index 240e20709..931fdc73f 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -4554,6 +4554,36 @@ private: "}\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()); + + check("void foo();\n" + "void bar();\n" + "void f() {\n" + " std::mutex m;\n" + " std::thread t([&m](){\n" + " m.lock();\n" + " foo();\n" + " m.unlock();\n" + " });\n" + " m.lock();\n" + " bar();\n" + " m.unlock();\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void foo();\n" + "void bar();\n" + "void f() {\n" + " std::mutex m;\n" + " std::thread t([&m](){\n" + " std::unique_lock g{m};\n" + " foo();\n" + " });\n" + " std::unique_lock g{m};\n" + " bar();\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); } }; From 4373404238a785bcb5b73d96a82afd0111086cfa Mon Sep 17 00:00:00 2001 From: Paul Date: Tue, 14 Jul 2020 13:04:59 -0500 Subject: [PATCH 2/2] Revert "Fixed #9795 (False positive: Local lock is not ineffective, mutex is locked in thread also.)" This reverts commit 27841d6b811a9a7537efe943af23f99f5afb68ce. --- lib/checkstl.cpp | 29 ++++++++++++++++++++++++++- lib/checkstl.h | 2 ++ test/teststl.cpp | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 58706e2ae..ebeb7f44f 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2394,12 +2394,25 @@ void CheckStl::useStlAlgorithm() } } +static bool isMutex(const Variable* var) +{ + 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 = 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, @@ -2407,6 +2420,13 @@ void CheckStl::globalLockGuardError(const Token* tok) "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) +{ + reportError(tok, Severity::warning, + "localMutex", + "The lock is ineffective because the mutex is locked at the same scope as the mutex itself.", CWE667, false); +} + void CheckStl::checkMutexes() { for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) { @@ -2414,12 +2434,19 @@ void CheckStl::checkMutexes() const Variable* var = tok->variable(); if (!var) continue; - if (Token::Match(tok, "%var% (|{ %var% )|}|,")) { + if (Token::Match(tok, "%var% . lock ( )")) { + if (!isMutex(var)) + continue; + if (isLocalMutex(var, 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 (isLocalMutex(mvar, tok->scope())) + localMutexError(tok); } } } diff --git a/lib/checkstl.h b/lib/checkstl.h index a0ae7d1e5..9dd5e07d5 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -233,6 +233,7 @@ 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; @@ -271,6 +272,7 @@ private: 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 e9e929a92..240e20709 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -4445,6 +4445,28 @@ private: 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]: (warning) The lock is ineffective because the mutex is locked at the same scope as the mutex itself.\n", errout.str()); + + check("void f() {\n" + " std::mutex m;\n" + " std::unique_lock g(m);\n" + "}\n", + true); + 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 f() {\n" + " std::mutex m;\n" + " std::unique_lock g(m, std::defer_lock);\n" + " std::lock(g);\n" + "}\n", + true); + 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" " static std::mutex m;\n" @@ -4455,6 +4477,16 @@ private: 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]: (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" " void f() {\n" @@ -4503,6 +4535,25 @@ private: "}\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()); } };