Backport "[analyzer] Restore recognition of mutex methods"

[steakhal]

Before commit 705788c the checker alpha.unix.BlockInCriticalSection "recognized" the methods std::mutex::lock and std::mutex::unlock with an extremely trivial check that accepted any function (or method) named lock/unlock.

To avoid matching unrelated user-defined function, this was refined to a check that also requires the presence of "std" and "mutex" as distinct parts of the qualified name.

However, as #99628 reported, there are standard library implementations where some methods of std::mutex are inherited from an implementation detail base class and the new code wasn't able to recognize these methods, which led to emitting false positive reports.

As a workaround, this commit partially restores the old behavior by omitting the check for the class name.

In the future, it would be good to replace this hack with a solution which ensures that CallDescription understands inherited methods.

(cherry picked from commit 99ae2ed)

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balazs Benics (steakhal)

Changes

Before commit 705788c the checker alpha.unix.BlockInCriticalSection "recognized" the methods std::mutex::lock and std::mutex::unlock with an extremely trivial check that accepted any function (or method) named lock/unlock.

To avoid matching unrelated user-defined function, this was refined to a check that also requires the presence of "std" and "mutex" as distinct parts of the qualified name.

However, as #99628 reported, there are standard library implementations where some methods of std::mutex are inherited from an implementation detail base class and the new code wasn't able to recognize these methods, which led to emitting false positive reports.

As a workaround, this commit partially restores the old behavior by omitting the check for the class name.

In the future, it would be good to replace this hack with a solution which ensures that CallDescription understands inherited methods.

(cherry picked from commit 99ae2ed)

---

Full diff: https://github.com/llvm/llvm-project/pull/101651.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp (+12-4)
• (added) clang/test/Analysis/block-in-critical-section-inheritance.cpp (+31)

diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 40f7e9cede1f1..4cd2f2802f30c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -147,10 +147,18 @@ using MutexDescriptor =
 class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
 private:
   const std::array<MutexDescriptor, 8> MutexDescriptors{
-      MemberMutexDescriptor({/*MatchAs=*/CDM::CXXMethod,
-                             /*QualifiedName=*/{"std", "mutex", "lock"},
-                             /*RequiredArgs=*/0},
-                            {CDM::CXXMethod, {"std", "mutex", "unlock"}, 0}),
+      // NOTE: There are standard library implementations where some methods
+      // of `std::mutex` are inherited from an implementation detail base
+      // class, and those aren't matched by the name specification {"std",
+      // "mutex", "lock"}.
+      // As a workaround here we omit the class name and only require the
+      // presence of the name parts "std" and "lock"/"unlock".
+      // TODO: Ensure that CallDescription understands inherited methods.
+      MemberMutexDescriptor(
+          {/*MatchAs=*/CDM::CXXMethod,
+           /*QualifiedName=*/{"std", /*"mutex",*/ "lock"},
+           /*RequiredArgs=*/0},
+          {CDM::CXXMethod, {"std", /*"mutex",*/ "unlock"}, 0}),
       FirstArgMutexDescriptor({CDM::CLibrary, {"pthread_mutex_lock"}, 1},
                               {CDM::CLibrary, {"pthread_mutex_unlock"}, 1}),
       FirstArgMutexDescriptor({CDM::CLibrary, {"mtx_lock"}, 1},
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
new file mode 100644
index 0000000000000..db20df8c60a5c
--- /dev/null
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:   -analyzer-checker=unix.BlockInCriticalSection \
+// RUN:   -std=c++11 \
+// RUN:   -analyzer-output text \
+// RUN:   -verify %s
+
+unsigned int sleep(unsigned int seconds) {return 0;}
+namespace std {
+// There are some standard library implementations where some mutex methods
+// come from an implementation detail base class. We need to ensure that these
+// are matched correctly.
+class __mutex_base {
+public:
+  void lock();
+};
+class mutex : public __mutex_base{
+public:
+  void unlock();
+  bool try_lock();
+};
+} // namespace std
+
+void gh_99628() {
+  std::mutex m;
+  m.lock();
+  // expected-note@-1 {{Entering critical section here}}
+  sleep(10);
+  // expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
+  // expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
+  m.unlock();
+}

[Xazax-hun]

LGTM, makes sense to backport.

[github-actions]

@steakhal (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.