[analyzer] lock_guard and unique_lock extension for BlockInCriticalSection Static Analyzer checker

[zdtorok]

Summary

This is an extension to an already existing alpha-checker which finds the calls to blocking functions inside a critical section. (See that original diff: https://reviews.llvm.org/D21506, and an earlier extension: https://reviews.llvm.org/D29567). In this patch lock_guard and unique_lock support has been added.

[zaks.anna]

Looks good overall, but we should try and extend isCalled if possible.

Also, what is the plan for bringing this checker out of alpha? Turning checkers on by default should be top priority since most users are not exposed to them otherwise!

Thank you!
Anna

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

111 ↗

(On Diff #100885)

Why isCalled is not used here? Would it be possible coextend it to support CXXConstructorCall?

[NoQ]

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

111 ↗

(On Diff #100885)

CallDescription objects don't support C++ constructors or Objective-C methods. By the way, we've just noticed that this checker crashes on pretty much any Objective-C code:

$ cat test.m

#include <Foundation/Foundation.h>

void foo() {
  NSObject *obj = [[NSObject alloc] init];
  [obj release];
}

$ clang --analyze -Xanalyzer -analyzer-checker -Xanalyzer alpha.unix.BlockInCriticalSection test.m

Assertion failed: (getKind() != CE_ObjCMessage && "Obj-C methods are not supported"), function isCalled, file <censored>/llvm/tools/clang/lib/StaticAnalyzer/Core/CallEvent.cpp, line 214.

So when you use isCalled(), currently you must ensure that a plain C function is being called.

I totally agree that isCalled() should be improved to support various calls - its whole point is removing code duplication, i.e. stuff like initIdentifierInfo().

[zaks.anna]

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

111 ↗

(On Diff #100885)

I suggest extending isCalled with C++ constructors.
We should also make sure that it isCalled conservatively handles ObjC method calls when assertions are off. Not sure what we want to do for assert builds... there are benefits of having the assertion there and there are downsides.

Another option would be to remove isCalled and all dependencies on it if we do not think it should be used by everyone.

@xazax.hun, what do you think?

[xazax.hun]

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

111 ↗

(On Diff #100885)

I do agree, handling ObjC methods conservatively in a release build is a good idea. I am in favor of keeping the asserts though.

One of the reasons I do like the isCalled abstraction, we do not need to use the mutable keyword in the checkers. It would definitely be good to support C++ methods and constructors. It is not trivial to come up with a good design due to overloading.

The CallDescription could accept a list of strings that describes the name of the types of the arguments, but what about type aliases?
It should also have a way to describe constraints on namespaces.
It could have an enum to restrict results to ctors/dtors/overloaded operators/methods/objc-methods/c functions in the future.
Also in case of methods, a method might be const, volatile, it could be overloaded on lvalues/rvalues.

We might add features as we go, but it might become harder to extend it in the future if we do not have a good design upfront. I think such a "call description" will be useful in the future since similar mechanism would be needed to have API Notes support for C++.

[zaks.anna]

Ping... While extending isCalled is not a must, this should not crash on the sample ObjC code.

[xazax.hun]

In D33729#853432, @zaks.anna wrote:

Ping... While extending isCalled is not a must, this should not crash on the sample ObjC code.

Created a new patch for that: https://reviews.llvm.org/D37470

[zaks.anna]

Thanks!

[xazax.hun]

LGTM as well.

cfe/trunk/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
-class BlockInCriticalSectionChecker : public Checker<check::PostCall,
-                                                     check::PreCall> {
+class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
+
+  mutable IdentifierInfo *IILockGuard, *IIUniqueLock;
 
   CallDescription LockFn, UnlockFn, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn,
                   PthreadLockFn, PthreadTryLockFn, PthreadUnlockFn,
                   MtxLock, MtxTimedLock, MtxTryLock, MtxUnlock;
 
+  StringRef ClassLockGuard, ClassUniqueLock;
+
+  mutable bool IdentifierInfoInitialized;
+
   std::unique_ptr<BugType> BlockInCritSectionBugType;
 
+  void initIdentifierInfo(ASTContext &Ctx) const;
+
   void reportBlockInCritSection(SymbolRef FileDescSym,
                                 const CallEvent &call,
                                 CheckerContext &C) const;
 
 public:
   BlockInCriticalSectionChecker();
 
   bool isBlockingFunction(const CallEvent &Call) const;
   bool isLockFunction(const CallEvent &Call) const;
   bool isUnlockFunction(const CallEvent &Call) const;
 
-  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
-
   /// Process unlock.
   /// Process lock.
   /// Process blocking functions (sleep, getc, fgets, read, recv)
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
-
 };
 
 } // end anonymous namespace
 
 REGISTER_TRAIT_WITH_PROGRAMSTATE(MutexCounter, unsigned)
 
 BlockInCriticalSectionChecker::BlockInCriticalSectionChecker()
-    : LockFn("lock"), UnlockFn("unlock"), SleepFn("sleep"), GetcFn("getc"),
+    : IILockGuard(nullptr), IIUniqueLock(nullptr),
+      LockFn("lock"), UnlockFn("unlock"), SleepFn("sleep"), GetcFn("getc"),
       FgetsFn("fgets"), ReadFn("read"), RecvFn("recv"),
       PthreadLockFn("pthread_mutex_lock"),
       PthreadTryLockFn("pthread_mutex_trylock"),
       PthreadUnlockFn("pthread_mutex_unlock"),
       MtxLock("mtx_lock"),
       MtxTimedLock("mtx_timedlock"),
       MtxTryLock("mtx_trylock"),
-      MtxUnlock("mtx_unlock") {
+      MtxUnlock("mtx_unlock"),
+      ClassLockGuard("lock_guard"),
+      ClassUniqueLock("unique_lock"),
+      IdentifierInfoInitialized(false) {
   // Initialize the bug type.
   BlockInCritSectionBugType.reset(
       new BugType(this, "Call to blocking function in critical section",
                         "Blocking Error"));
 }
 
+void BlockInCriticalSectionChecker::initIdentifierInfo(ASTContext &Ctx) const {
+  if (!IdentifierInfoInitialized) {
+    /* In case of checking C code, or when the corresponding headers are not
+     * included, we might end up query the identifier table every time when this
+     * function is called instead of early returning it. To avoid this, a bool
+     * variable (IdentifierInfoInitialized) is used and the function will be run
+     * only once. */
+    IILockGuard  = &Ctx.Idents.get(ClassLockGuard);
+    IIUniqueLock = &Ctx.Idents.get(ClassUniqueLock);
+    IdentifierInfoInitialized = true;
+  }
+}
+
 bool BlockInCriticalSectionChecker::isBlockingFunction(const CallEvent &Call) const {
   if (Call.isCalled(SleepFn)
       || Call.isCalled(GetcFn)
       || Call.isCalled(FgetsFn)
       || Call.isCalled(ReadFn)
       || Call.isCalled(RecvFn)) {
     return true;
   }
   return false;
 }
 
 bool BlockInCriticalSectionChecker::isLockFunction(const CallEvent &Call) const {
+  if (const auto *Ctor = dyn_cast<CXXConstructorCall>(&Call)) {
+    auto IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier();
+    if (IdentifierInfo == IILockGuard || IdentifierInfo == IIUniqueLock)
+      return true;
+  }
+
   if (Call.isCalled(LockFn)
       || Call.isCalled(PthreadLockFn)
       || Call.isCalled(PthreadTryLockFn)
       || Call.isCalled(MtxLock)
       || Call.isCalled(MtxTimedLock)
       || Call.isCalled(MtxTryLock)) {
     return true;
   }
   return false;
 }
 
 bool BlockInCriticalSectionChecker::isUnlockFunction(const CallEvent &Call) const {
+  if (const auto *Dtor = dyn_cast<CXXDestructorCall>(&Call)) {
+    const auto *DRecordDecl = dyn_cast<CXXRecordDecl>(Dtor->getDecl()->getParent());
+    auto IdentifierInfo = DRecordDecl->getIdentifier();
+    if (IdentifierInfo == IILockGuard || IdentifierInfo == IIUniqueLock)
+      return true;
+  }
+
   if (Call.isCalled(UnlockFn)
        || Call.isCalled(PthreadUnlockFn)
        || Call.isCalled(MtxUnlock)) {
     return true;
   }
   return false;
 }
 
-void BlockInCriticalSectionChecker::checkPreCall(const CallEvent &Call,
-                                                 CheckerContext &C) const {
-}
-
 void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call,
                                                   CheckerContext &C) const {
+  initIdentifierInfo(C.getASTContext());
+
   if (!isBlockingFunction(Call)
       && !isLockFunction(Call)
       && !isUnlockFunction(Call))
     return;
 
   ProgramStateRef State = C.getState();
   unsigned mutexCount = State->get<MutexCounter>();
   if (isUnlockFunction(Call) && mutexCount > 0) {

cfe/trunk/test/Analysis/block-in-critical-section.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.BlockInCriticalSection -std=c++11 -verify %s
 
 void sleep(int x) {}
 
 namespace std {
 struct mutex {
   void lock() {}
   void unlock() {}
 };
+template<typename T>
+struct lock_guard {
+  lock_guard<T>(std::mutex) {}
+  ~lock_guard<T>() {}
+};
+template<typename T>
+struct unique_lock {
+  unique_lock<T>(std::mutex) {}
+  ~unique_lock<T>() {}
+};
+template<typename T>
+struct not_real_lock {
+  not_real_lock<T>(std::mutex) {}
+};
 }
 
 void getc() {}
 void fgets() {}
 void read() {}
 void recv() {}
 
 void pthread_mutex_lock() {}

 
 void testBlockInCriticalSectionUnexpectedUnlock() {
   std::mutex m;
   m.unlock();
   sleep(1); // no-warning
   m.lock();
   sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
 }
+
+void testBlockInCriticalSectionLockGuard() {
+  std::mutex g_mutex;
+  std::not_real_lock<std::mutex> not_real_lock(g_mutex);
+  sleep(1); // no-warning
+
+  std::lock_guard<std::mutex> lock(g_mutex);
+  sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+}
+
+void testBlockInCriticalSectionLockGuardNested() {
+  testBlockInCriticalSectionLockGuard();
+  sleep(1); // no-warning
+}
+
+void testBlockInCriticalSectionUniqueLock() {
+  std::mutex g_mutex;
+  std::not_real_lock<std::mutex> not_real_lock(g_mutex);
+  sleep(1); // no-warning
+
+  std::unique_lock<std::mutex> lock(g_mutex);
+  sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+}
+
+void testBlockInCriticalSectionUniqueLockNested() {
+  testBlockInCriticalSectionUniqueLock();
+  sleep(1); // no-warning
+}