Page MenuHomePhabricator

[analyzer] Block in critical section
ClosedPublic

Authored by zdtorok on Jun 19 2016, 10:54 AM.

Details

Summary

This checker should find the calls to blocking functions (for example: sleep, getc, fgets,read,recv etc.) inside a critical section. When sleep(x) is called while a mutex is held, other threads cannot lock the same mutex. This might take some time, leading to bad performance or even deadlock.

Example:

mutex_t m;

void f() {
  sleep(1000); // Error: sleep() while m is locked! [f() is called from foobar() while m is locked]
  // do some work
}

void foobar() {
  lock(m);
  f();
  unlock(m);
}

Diff Detail

Repository
rL LLVM

Event Timeline

zdtorok updated this revision to Diff 61206.Jun 19 2016, 10:54 AM
zdtorok retitled this revision from to [analyzer] Block in critical section.
zdtorok updated this object.
zdtorok added reviewers: zaks.anna, dcoughlin, xazax.hun.
zdtorok set the repository for this revision to rL LLVM.
zdtorok added a subscriber: cfe-commits.
NoQ added a subscriber: NoQ.Jun 20 2016, 3:23 AM
NoQ added a comment.Jun 20 2016, 3:45 AM

Useful stuff!

When I see your approach with mutexCount, the following test case comes to mind:

std::mutex m;
void foo() {
  // Suppose this function is always called
  // with the mutex 'm' locked.
  m.unlock();
  // Here probably some useful work is done.
  m.lock();
  // What's the current mutex count?
  sleep(1); // expected-warning{{}}
}
lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
11 ↗(On Diff #61206)

A bit of trailing whitespace here.

test/Analysis/block-in-critical-section.cpp
1 ↗(On Diff #61206)

The run line is lacking -verify, and therefore these tests always pass. In your case, it'd be -Xclang -verify. On the other hand, i'm not sure if it's a good idea to run the test under the driver, instead of the standard -cc1 approach - after all, you aren't including any system headers in the test.

zaks.anna edited edge metadata.Jun 20 2016, 5:19 PM

Thanks for the patch!

Have you run this on a large codebase?

include/clang/StaticAnalyzer/Checkers/Checkers.td
406 ↗(On Diff #61206)

"blocks" sounds a lot like blocks ObjC feature. I'd just be more explicit and say "Check for calls to blocking functions inside a critical section".

Please, change the name of the class as well.

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
54 ↗(On Diff #61206)

Counter -> MutexCounter or MutexState

61 ↗(On Diff #61206)

Same here: "Block" sounds like an ObjC language feature. Also, the category name ("Unix Stream API Error") should be different.

85 ↗(On Diff #61206)

Why is handling of unlock in preCall?

96 ↗(On Diff #61206)

It would be great if you could implement a BugReporterVisitor that walks the path and explains where the lock has occurred. (This is not blocking for this commit.)

103 ↗(On Diff #61206)

"Block in a critical section" -> "A blocking function %s is called inside a critical section."

test/Analysis/block-in-critical-section.cpp
12 ↗(On Diff #61206)

Please, add inter-procedural examples like in the description of the checker.

zaks.anna requested changes to this revision.Jul 22 2016, 10:20 AM
zaks.anna edited edge metadata.
This revision now requires changes to proceed.Jul 22 2016, 10:20 AM
zdtorok marked 8 inline comments as done.EditedSep 5 2016, 12:48 PM

Thank you for both of your comments and feedbacks! Also please excuse me for my (really) late response. Please have a look at my fixes and the new attached diff. Thanks!

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
85 ↗(On Diff #61206)

For some reason first I thought it's better for performance reasons - but now I've changed it to postCall.

zdtorok updated this revision to Diff 70345.Sep 5 2016, 12:50 PM
zdtorok edited edge metadata.
zdtorok removed rL LLVM as the repository for this revision.
zdtorok marked an inline comment as done.

Fixed based on the provided comments and feedbacks.

  • added new inter-procedural tests
  • minor styling fix (trailing whitespaces)
  • fixed tests (missing verify option)
  • fixed post, preCall checks
zdtorok edited edge metadata.Sep 5 2016, 12:51 PM
zdtorok set the repository for this revision to rL LLVM.
NoQ added a comment.EditedSep 12 2016, 11:48 AM

I think there's still this problem i've outlined in the comment above: you can step into an integer-underflow if your analysis begins with unlock(). You could just ignore all unlocks that move you below 0, which would be ok.

Could you add this test?

std::mutex m;
void foo() {
  m.unlock(); // MutexCount = 4294967295, should be 0, just ignore this unlock.
  sleep(1); // no-warning
  m.lock(); // MutexCount = 0, should be 1, woohoo we're sure we're in the section now.
  sleep(1); // expected-warning{{}}
}
zdtorok updated this revision to Diff 71375.Sep 14 2016, 9:48 AM

Fixed possible integer-underflow in case of unexpected unlocking function. Also added test for this.

You are right NoQ, thank you for the remark, I've upload a new diff containing a fix for the integer underflow case.

zaks.anna accepted this revision.Sep 14 2016, 12:01 PM
zaks.anna edited edge metadata.

LGTM! Thanks!

Future steps:
How do we plan to bring this checker out of alpha? Have you evaluated it on large codebases?

This revision is now accepted and ready to land.Sep 14 2016, 12:01 PM

Do you have commit access or should we commit?

As it's my first contribution I think I have no commit access.
So far I could not test it on larger codebase. I've tried to do so on clang but my computer was too slow for that. I will try it again.
What you mean by alpha-phase? What should be the next steps now?

This checker is now in alpha.unix, because it is new and is in active development. However, alpha checkers are not supported and are not turned on by default, so we should move it into unix package once we think it is ready to be used.

Evaluation on a large real codebase (or several) would give us a higher confidence in the checker, so it will be valuable to perform that before we move the package out of alpha. However, clang is probably not a good codebase to test this on because it's not heavily multithreaded.

This revision was automatically updated to reflect the committed changes.