Page MenuHomePhabricator

[analyzer] Add check for mutex acquisition during interrupt context in Magenta kernel
Needs ReviewPublic

Authored by khazem on Dec 16 2016, 9:39 AM.

Details

Summary

Acquiring a mutex during the Magenta kernel exception handler can cause deadlocks and races. This patch adds a checker that ensures that no mutexes are acquired during an interrupt context.

Diff Detail

Event Timeline

khazem updated this revision to Diff 81767.Dec 16 2016, 9:39 AM
khazem retitled this revision from to [analyzer] Add check for mutex acquisition during interrupt context in Magenta kernel.
khazem updated this object.
khazem added reviewers: dcoughlin, dergachev.a.
khazem added subscribers: cfe-commits, phosek, seanklein.
NoQ added a subscriber: NoQ.Dec 17 2016, 3:37 AM

This checker looks good to me! I don't see any obvious problems, and i think we can land it into non-alpha (enabled by default) once reviewers' comments are addressed.

include/clang/StaticAnalyzer/Checkers/Checkers.td
78

optin is for checkers for which we cannot make a good guess if the user wants those or not.

Can we make a guess by looking at the target triple in this case? If we can, then it should not go into optin, but rather auto-enabled on specific platform, similarly to how it works for eg. osx.

lib/StaticAnalyzer/Checkers/MagentaInterruptContextChecker.cpp
32

Indent slightly off.

54

To see if we're in an interrupt, take current LocationContext and see if its stack frame or a parent stack frame is for x86_exception_handler. I think you don't need a program state trait for that check - it's already in your location context - and checkBeginFunction()/checkEndFunction() can be removed. Also, as far as i remember (i may be wrong), AnalysisDeclContext will bring you the current top-level function declaration under analysis directly, without ascending the stack frames, if that's in fact all you need.

For exit-functions you'd still need a flag in the program state that says that there was an exit.

93

Probably a bool was intended. Also, this may be moved inside the assert body if not used anywhere else.

109

I'd suggest to use operator== for such cases because it's easier to read.