[analyzer] Block in critical section extension

[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). In this patch C11 and Pthread locking/unlocking support has been added and also two new parameters have been implemented to be able to switch these on/off.

[dcoughlin]

I saw on Twitter today that this checker already has at least one happy user! See https://twitter.com/steipete/status/830099493890703360

A question: under what scenarios do you envision the C11Enabled and PthreadEnabled analyzer options being used? Can we get away with not having the the options and just always check for them?

Also: Thanks for adding documentation to the .html!

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

141–145

I think it is worth fixing the diagnostic now to correctly mention the called function rather than have a stray '%s' in the message.

See, for example, CompareReturnTypes() in CheckObjCInstMethSignature.cpp for how to create a diagnostic message dynamically.

[xazax.hun]

In D29567#673660, @dcoughlin wrote:

A question: under what scenarios do you envision the C11Enabled and PthreadEnabled analyzer options being used? Can we get away with not having the the options and just always check for them?

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

[zaks.anna]

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

Good point! Have you observed performance issues due to this or is this based on an observation that the implementation of CallEvent::isCalled is not optimal?

Do you propose to keep a cache of all identifiers that were were previously looked up by all callers of CallEvent? Or should there be a per checker cache?

The main adopters of the new CallEvent API are ValistChecker.cpp, SimpleStreamChecker.cpp, and BlockInCriticalSectionChecker.cpp. The API optimization should be a separate patch. Are there volunteers to tackle it?

[xazax.hun]

In D29567#674338, @zaks.anna wrote:

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

Good point! Have you observed performance issues due to this or is this based on an observation that the implementation of CallEvent::isCalled is not optimal?

I did not measure the performance, only observed the implementation. Do you want me to do a measurement first? I fear that it might be noisy due to the fact that the execution time might still be dominated by other parts of the analyzer.

Do you propose to keep a cache of all identifiers that were were previously looked up by all callers of CallEvent? Or should there be a per checker cache?

I would put the cache inside the CallDescription objects, so checker authors do not need to wory about this.

The main adopters of the new CallEvent API are ValistChecker.cpp, SimpleStreamChecker.cpp, and BlockInCriticalSectionChecker.cpp. The API optimization should be a separate patch. Are there volunteers to tackle it?

I will submit a separate patch.

[xazax.hun]

In D29567#674339, @xazax.hun wrote:

In D29567#674338, @zaks.anna wrote:

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

Good point! Have you observed performance issues due to this or is this based on an observation that the implementation of CallEvent::isCalled is not optimal?

I did not measure the performance, only observed the implementation. Do you want me to do a measurement first? I fear that it might be noisy due to the fact that the execution time might still be dominated by other parts of the analyzer.

Do you propose to keep a cache of all identifiers that were were previously looked up by all callers of CallEvent? Or should there be a per checker cache?

I would put the cache inside the CallDescription objects, so checker authors do not need to wory about this.

The main adopters of the new CallEvent API are ValistChecker.cpp, SimpleStreamChecker.cpp, and BlockInCriticalSectionChecker.cpp. The API optimization should be a separate patch. Are there volunteers to tackle it?

I will submit a separate patch.

https://reviews.llvm.org/D29884 is up for review. With those changes accepted +1 for removing the options.

[xazax.hun]

In D29567#674832, @xazax.hun wrote:

In D29567#674339, @xazax.hun wrote:

In D29567#674338, @zaks.anna wrote:

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

Good point! Have you observed performance issues due to this or is this based on an observation that the implementation of CallEvent::isCalled is not optimal?

I did not measure the performance, only observed the implementation. Do you want me to do a measurement first? I fear that it might be noisy due to the fact that the execution time might still be dominated by other parts of the analyzer.

Do you propose to keep a cache of all identifiers that were were previously looked up by all callers of CallEvent? Or should there be a per checker cache?

I would put the cache inside the CallDescription objects, so checker authors do not need to wory about this.

The main adopters of the new CallEvent API are ValistChecker.cpp, SimpleStreamChecker.cpp, and BlockInCriticalSectionChecker.cpp. The API optimization should be a separate patch. Are there volunteers to tackle it?

I will submit a separate patch.

https://reviews.llvm.org/D29884 is up for review. With those changes accepted +1 for removing the options.

Commited in r295186.

[zaks.anna]

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

141–145

I think Devin suggests using llvm::raw_svector_ostream here. Another example is in MallocChecker.cpp.

[zdtorok]

Based on your feedback I've removed the PThreadEnabled and C11Enabled options and also in the warning %s has been replaced by the function's name itself.
Thanks for your feedback and helpful comments!

[dcoughlin]

Thanks for updating this!

It looks good to me with the diagnostic changes (single quotes, removed period) mentioned inline!

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

143

• You should add single quotes around the function name to be consistent existing clang diagnostics. (For example, " function 'sleep' is")
• Similarly, you should not end the diagnostic with a period. (We are not always consistent about this in the analyzer -- but we should do better!)

Also, here is a suggestion for a slightly re-worded diagnostic that seems more direct to me:

"Call to blocking function 'foo' inside of critical section"

What do you think?

148

You shouldn't do it in this commit, but one thing that would make this checker even more helpful would be to add a BugReporterVisitor adding a path note indicating where the critical section began.

[zdtorok]

Changed the warning message.

[zdtorok]

Thanks for the feedback again. I've changed the diagnostic message and uploaded the new diff.
Please note, that I'm not allowed to commit yet, so if it's accepted, may I ask some of you to commit my changes please? Thanks!

[xazax.hun]

In D29567#693157, @zdtorok wrote:

Changed the warning message.

Did you rerun the tests? As far as I can see, you did not update the test files according to the rewording of the message, so I think they won't pass.

[zdtorok]

Fixed tests after re-phrasing the diagnostic message.

[zdtorok]

Fixed the tests with the new diagnostic message. Now all tests pass.

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

 using namespace clang;
 using namespace ento;
 
 namespace {
 
 class BlockInCriticalSectionChecker : public Checker<check::PostCall,
                                                      check::PreCall> {
 
-  CallDescription LockFn, UnlockFn, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn;
+  CallDescription LockFn, UnlockFn, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn,
+                  PthreadLockFn, PthreadTryLockFn, PthreadUnlockFn,
+                  MtxLock, MtxTimedLock, MtxTryLock, MtxUnlock;
 
   std::unique_ptr<BugType> BlockInCritSectionBugType;
 
   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"),
-      FgetsFn("fgets"), ReadFn("read"), RecvFn("recv") {
+      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") {
   // Initialize the bug type.
   BlockInCritSectionBugType.reset(
       new BugType(this, "Call to blocking function in critical section",
                         "Blocking Error"));
 }
 
+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 (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 (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 {
-  if (!Call.isCalled(LockFn)
-      && !Call.isCalled(SleepFn)
-      && !Call.isCalled(GetcFn)
-      && !Call.isCalled(FgetsFn)
-      && !Call.isCalled(ReadFn)
-      && !Call.isCalled(RecvFn)
-      && !Call.isCalled(UnlockFn))
+  if (!isBlockingFunction(Call)
+      && !isLockFunction(Call)
+      && !isUnlockFunction(Call))
     return;
 
   ProgramStateRef State = C.getState();
   unsigned mutexCount = State->get<MutexCounter>();
-  if (Call.isCalled(UnlockFn) && mutexCount > 0) {
+  if (isUnlockFunction(Call) && mutexCount > 0) {
     State = State->set<MutexCounter>(--mutexCount);
     C.addTransition(State);
-  } else if (Call.isCalled(LockFn)) {
+  } else if (isLockFunction(Call)) {
     State = State->set<MutexCounter>(++mutexCount);
     C.addTransition(State);
   } else if (mutexCount > 0) {
     SymbolRef BlockDesc = Call.getReturnValue().getAsSymbol();
     reportBlockInCritSection(BlockDesc, Call, C);
   }
 }
 
 void BlockInCriticalSectionChecker::reportBlockInCritSection(
     SymbolRef BlockDescSym, const CallEvent &Call, CheckerContext &C) const {
   ExplodedNode *ErrNode = C.generateNonFatalErrorNode();
   if (!ErrNode)
     return;
 
-  auto R = llvm::make_unique<BugReport>(*BlockInCritSectionBugType,
-      "A blocking function %s is called inside a critical section.", ErrNode);
+  std::string msg;
+  llvm::raw_string_ostream os(msg);
+  os << "Call to blocking function '" << Call.getCalleeIdentifier()->getName()
+     << "' inside of critical section";
+  auto R = llvm::make_unique<BugReport>(*BlockInCritSectionBugType, os.str(), ErrNode);
   R->addRange(Call.getSourceRange());
   R->markInteresting(BlockDescSym);
   C.emitReport(std::move(R));
 }
 
 void ento::registerBlockInCriticalSectionChecker(CheckerManager &mgr) {
   mgr.registerChecker<BlockInCriticalSectionChecker>();
 }

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

 // RUN: %clang_cc1 -analyze -analyzer-checker=alpha.unix.BlockInCriticalSection -std=c++11 -verify %s
 
 void sleep(int x) {}
 
 namespace std {
 struct mutex {
   void lock() {}
   void unlock() {}
 };
 }
 
-void testBlockInCriticalSection() {
+void getc() {}
+void fgets() {}
+void read() {}
+void recv() {}
+
+void pthread_mutex_lock() {}
+void pthread_mutex_trylock() {}
+void pthread_mutex_unlock() {}
+
+void mtx_lock() {}
+void mtx_timedlock() {}
+void mtx_trylock() {}
+void mtx_unlock() {}
+
+void testBlockInCriticalSectionWithStdMutex() {
   std::mutex m;
   m.lock();
-  sleep(3); // expected-warning {{A blocking function %s is called inside a critical section}}
+  sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+  getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+  fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+  read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+  recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
   m.unlock();
 }
 
+void testBlockInCriticalSectionWithPthreadMutex() {
+  pthread_mutex_lock();
+  sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+  getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+  fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+  read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+  recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+  pthread_mutex_unlock();
+
+  pthread_mutex_trylock();
+  sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+  getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+  fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+  read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+  recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+  pthread_mutex_unlock();
+}
+
+void testBlockInCriticalSectionC11Locks() {
+  mtx_lock();
+  sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+  getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+  fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+  read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+  recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+  mtx_unlock();
+
+  mtx_timedlock();
+  sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+  getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+  fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+  read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+  recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+  mtx_unlock();
+
+  mtx_trylock();
+  sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+  getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+  fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+  read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+  recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+  mtx_unlock();
+}
+
 void testBlockInCriticalSectionWithNestedMutexes() {
   std::mutex m, n, k;
   m.lock();
   n.lock();
   k.lock();
-  sleep(3); // expected-warning {{A blocking function %s is called inside a critical section}}
+  sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
   k.unlock();
-  sleep(5); // expected-warning {{A blocking function %s is called inside a critical section}}
+  sleep(5); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
   n.unlock();
-  sleep(3); // expected-warning {{A blocking function %s is called inside a critical section}}
+  sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
   m.unlock();
   sleep(3); // no-warning
 }
 
 void f() {
-  sleep(1000); // expected-warning {{A blocking function %s is called inside a critical section}}
+  sleep(1000); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
 }
 
 void testBlockInCriticalSectionInterProcedural() {
   std::mutex m;
   m.lock();
   f();
   m.unlock();
 }
 
 void testBlockInCriticalSectionUnexpectedUnlock() {
   std::mutex m;
   m.unlock();
   sleep(1); // no-warning
   m.lock();
-  sleep(1); // expected-warning {{A blocking function %s is called inside a critical section}}
+  sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
 }

www/analyzer/alpha_checks.html

 <div class="example"><pre>
 void test(char *y) {
   char x[4];
   if (strlen(y) == 4)
     strncpy(x, y, 5); // warn
 }
 </pre></div></div></td></tr>
 
+
+<tr><td><div class="namedescr expandable"><span class="name">
+alpha.unix.cstring.BlockInCriticalSection</span><span class="lang">
+(C)</span><div class="descr">
+Check for calls to blocking functions inside a critical section; applies
+to:<div class=functions>
+lock, unlock<br>
+sleep<br>
+getc<br>
+fgets<br>
+read<br>
+recv<br>
+pthread_mutex_lock, pthread_mutex_trylock, pthread_mutex_unlock<br>
+mtx_lock, mtx_timedlock, mtx_trylock, mtx_unlock<br>
+</div></div></div></td>
+<td><div class="exampleContainer expandable">
+<div class="example"><pre>
+void testBlockInCriticalSection() {
+  std::mutex m;
+  m.lock();
+  sleep(3); // warn
+  m.unlock();
+}
+</pre></div></div></td></tr>
 </tbody></table>
 
 </div> <!-- page -->
 </div> <!-- content -->
 </body>
 </html>