[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:19-20
+// handle variable is passed to different function calls or syscalls, its state
+// changes. The state changes can be generally represented by following ASCII
+// Art:
+//
----------------
NoQ wrote:
> Btw, this state machine is fairly common. Both MallocChecker and SimpleStreamChecker already follow this same model. Do you have any thoughts on re-using it in an abstract manner?
> 
> Something like this maybe?:
> ```lang=c++
> template <typename TraitID>
> class SimpleStreamStateMachine {
> public:
>   SimpleStreamStateMachine(void(*pointerEscapePolicy)(CheckerContext C, Other Customizations));
> 
>   ProgramStateRef makeOpened(ProgramStateRef State, SymbolRef Key) {
>     return State->set<TraitID>(Key, TraitID::ValueTy::makeOpened());
>   }
>   // makeReleased and so on pre-defined for all users,
>   // allowing customization when necessary.
> };
> 
> class SimpleStreamStateMachineBookkeeping : Checker {
> public:
>   checkDeadSymbols() {
>     // Perform the state cleanup for all concrete machines
>     // ever instantiated, in the only possible way, probably
>     // invoke callbacks for leaks.
>   }
>   checkPointerEscape() {
>     // Invoke the passed-down policy for each concrete
>     // state machine.
>   }
>   // Other callbacks are implemented in the dependent checker.
> };
> ```
> And then:
> ```lang=c++
> REGISTER_MAP_WITH_PROGRAMSTATE(MyTrait, SymbolRef, MyState);
> 
> class MyChecker : Checker {
>   SimpleStreamStateMachine<MyStateTy> StM {
>       getStateMachine<SimpleStreamStateMachine<MyStateTy>>(
>           &MyChecker::checkPointerEscapeImpl, /*Other Customizations*/)};
>   void checkPointerEscapeImpl(...);
> 
> public:
>   checkPostCall(...) {
>     C.addTransition(StM.makeOpened(C.getState(), Call.getRetVal()));
>   }
> };
> ```
> 
> 'Cause i have this pipe dream that we make a lot of such abstract state machines and then we'll never need to write more of them and that'll make it cheaper to introduce non-trivial operations over the program state such as replacing values or advanced widening because we'll only have to implement them in the few state machines rather than in many checkers.
Hmm, indeed, lots of those checks following a very similar idea. I think a good litmus test would be to check if the customization points of such abstractions are strong enough to handle all sorts of error handling mechanisms that would potentially be handled by the checkers including: return values (like your example), output arguments, errno/other global error state. Also, checkers sometimes have slightly different state machines see the AllocatedOfSizeZero in MallocChecker, and sometimes they also have more companion data in the state like allocation family (this one should be easy to solve though). 

That being said I would prefer such experiments to happen in a separate patch :) 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70470/new/

https://reviews.llvm.org/D70470