Merge pull request #2682 from pfultz2/check-mutexes

Add new check for incorrect usage of mutexes and lock guards

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,62 @@ 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,
+                "globalLockGuard",
+                "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) {
+        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 (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);
+            }
+        }
+    }
+}
+

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() {
@@ -287,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";
     }
 };
 /// @}

lib/token.cpp

@@ -2086,6 +2086,8 @@ const ::Type *Token::typeOf(const Token *tok)
 
 std::pair<const Token*, const Token*> 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<const Token*, const Token*> 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<const Token*, const Token*> r = typeDecl(tok2->astOperand2());
+                if (r.first)
+                    return r;
+            }
+        }
         return {var->typeStartToken(), var->typeEndToken()->next()};
-    } else if (Token::Match(tok, "%name%")) {
+    } else if (Token::Match(tok->previous(), "%name% (")) {
-        const Function *function = tok->function();
+        const Function *function = tok->previous()->function();
         if (!function)
             return {};
         return {function->retDef, function->returnDefEnd()};

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<int> g();\n"

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,117 @@ private:
               true);
         ASSERT_EQUALS("", errout.str());
     }
+
+    void checkMutexes() {
+        check("void f() {\n"
+              "    static std::mutex m;\n"
+              "    static std::lock_guard<std::mutex> g(m);\n"
+              "}\n",
+              true);
+        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"
+              "    std::lock_guard<std::mutex> g(m);\n"
+              "}\n",
+              true);
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    std::mutex m;\n"
+              "    std::lock_guard<std::mutex> 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 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]: (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"
+              "        std::lock_guard<std::mutex> 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<std::mutex> g(m);\n"
+              "    }\n"
+              "};\n",
+              true);
+        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"
+              "    auto& m = h();\n"
+              "    std::lock_guard<std::mutex> 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<std::mutex> 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());
+    }
 };
 
 REGISTER_TEST(TestStl)