llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Benics (steakhal) <details> <summary>Changes</summary> BlockInCriticalSectionChecker registers the pre-call for the RAII ctors and Dtors - and also the raw 'lock' and 'unlock' handlers. However, pre-call does not prevent inlining. This means that (in the likely case of) that the body is present, the analyzer will model the effect of the lock twice. This happens on libc++ unique_lock. We really should have eval-called the ctor/dtor to avoid the inlining of those, but here we are. rdar://175814310 --- Full diff: https://github.com/llvm/llvm-project/pull/208729.diff 2 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp (+28-6) - (added) clang/test/Analysis/block-in-critical-section-raii.cpp (+53) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp index 03c576270797b..ad26324cdab76 100644 --- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp @@ -212,7 +212,8 @@ class SuppressNonBlockingStreams : public BugReporterVisitor { } }; -class BlockInCriticalSectionChecker : public Checker<check::PostCall> { +class BlockInCriticalSectionChecker + : public Checker<check::PostCall, eval::Call> { private: const std::array<MutexDescriptor, 9> MutexDescriptors{ // NOTE: There are standard library implementations where some methods @@ -276,6 +277,9 @@ class BlockInCriticalSectionChecker : public Checker<check::PostCall> { /// Process lock. /// Process blocking functions (sleep, getc, fgets, read, recv) void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + + // Process RAII lock guard object ctors/dtors. + bool evalCall(const CallEvent &Call, CheckerContext &C) const; }; } // end anonymous namespace @@ -373,15 +377,33 @@ void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { if (isBlockingInCritSection(Call, C)) { reportBlockInCritSection(Call, C); - } else if (std::optional<MutexDescriptor> LockDesc = - checkDescriptorMatch(Call, C, /*IsLock=*/true)) { - handleLock(*LockDesc, Call, C); - } else if (std::optional<MutexDescriptor> UnlockDesc = - checkDescriptorMatch(Call, C, /*IsLock=*/false)) { + return; + } + + if (std::optional<MutexDescriptor> LockDesc = + checkDescriptorMatch(Call, C, /*IsLock=*/true)) { + if (!std::holds_alternative<RAIIMutexDescriptor>(*LockDesc)) + handleLock(*LockDesc, Call, C); + return; + } + if (std::optional<MutexDescriptor> UnlockDesc = + checkDescriptorMatch(Call, C, /*IsLock=*/false)) { handleUnlock(*UnlockDesc, Call, C); } } +bool BlockInCriticalSectionChecker::evalCall(const CallEvent &Call, + CheckerContext &C) const { + if (std::optional<MutexDescriptor> LockDesc = + checkDescriptorMatch(Call, C, /*IsLock=*/true)) { + if (std::holds_alternative<RAIIMutexDescriptor>(*LockDesc)) { + handleLock(*LockDesc, Call, C); + return true; + } + } + return false; +} + void BlockInCriticalSectionChecker::reportBlockInCritSection( const CallEvent &Call, CheckerContext &C) const { ExplodedNode *ErrNode = C.generateNonFatalErrorNode(C.getState()); diff --git a/clang/test/Analysis/block-in-critical-section-raii.cpp b/clang/test/Analysis/block-in-critical-section-raii.cpp new file mode 100644 index 0000000000000..4bc4c94c74259 --- /dev/null +++ b/clang/test/Analysis/block-in-critical-section-raii.cpp @@ -0,0 +1,53 @@ +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core,unix.BlockInCriticalSection + +void sleep(int x); + +namespace std { +struct mutex { + void lock(); + void unlock(); +}; +// libc++-shaped unique_lock: ctor and dtor have non-empty bodies that call +// the underlying mutex's lock()/unlock() member functions. +template <class M> +struct unique_lock { + M *m_; + bool owns_; + explicit unique_lock(M &m) : m_(&m), owns_(true) { m_->lock(); } + ~unique_lock() { + if (owns_) + m_->unlock(); + } +}; +} // namespace std + +// Recursive use of the RAII guard. Without the fix, deep recursion would +// prevent the dtor body from inlining at the inlining-stack-depth limit; +// the inner mutex unlock would not fire, used to emit a leak FP. +struct C { + std::mutex m; + bool aborted; + bool getAborted() { + std::unique_lock<std::mutex> lk(m); + return aborted; + } + void recurse() { + if (getAborted()) + recurse(); + sleep(1); // no-warning + } +}; + +// Direct case: RAII guard scoped to a function body. Constructor inlining + +// destructor inlining must not double-count, even when both bodies inline. +void callee(std::mutex &m) { + std::unique_lock<std::mutex> lk(m); + // The lock IS held here so blocking is correctly diagnosed. + sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}} +} // dtor fires here, releases the lock; no further sleep should warn. + +void top(std::mutex &m) { + callee(m); + sleep(1); // no-warning: the lock from `callee` was released by ~unique_lock before this sleep. +} `````````` </details> https://github.com/llvm/llvm-project/pull/208729 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
