[PATCH] D83717: [clang-tidy] Add check fo SEI CERT item ENV32-C

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 abandoned this revision.
gamesh411 added a comment.

In D83717#2370154 <https://reviews.llvm.org/D83717#2370154>, @NoQ wrote:

> I don't think you actually need active support for invoking exit handlers path-sensitively at the end of `main()` in order to implement your checker. You can still find out in a path-insensitive manner whether any given function acts as an exit handler, like you already do. Then during path-sensitive analysis of that function you can warn at any invocation of exit().
>
> I do believe that you want to implement this check as a path-sensitive check as long as you want any sort of interprocedural analysis (i.e., warn when an exit handler calls a function that calls a function ... that calls a function that calls `exit()` - and you do already have such tests). This is because of the following potential situation:
>
>   void foo(bool already_exiting) {
>     if (!already_exiting)
>       exit();
>   }
>   
>   void bar() {
>     foo(true);
>   }
>   
>   void baz() {
>     foo(false);
>   }
>   
>   int main() {
>     atexit(bar);
>     return 0;
>   }
>
> In this case `bar()` is an exit handler that calls `foo()` that calls `exit()`. However the code is correct and no warning should be emitted, because `foo()` would never call `exit()` //when called from// `bar()` //specifically//. Precise reasoning about such problems requires path-sensitive analysis. Of course it's up to you to decide what to do with these false positives - whether you'll be ok with having them, or choose to suppress with an imprecise heuristic - but that's one of the possible reasons to consider reimplementing the checker via path sensitive analysis that the static analyzer provides.
>
> We've had a similar problem with the checker that warns on calling pure virtual functions in constructors/destructors (which is undefined behavior). Such checker had to be path sensitive in order to be interprocedural for the exact same unobvious reason. We've decided to re-implement it with path-sensitive analysis and now we're pretty happy about that decision.

Thanks! I have checked, and this is definitely doable (however the solution is a bit more involved in case of CTU). This check is definitely better to implement in a path-sensitive way.

For anyone looking to implement this (thanks to @NoQ for the original idea outline above) :

   This is conceptually a 2 phase solution.
  - Phase 1: detect the functions used as exit_handlers. As an approximation, this could be done based on syntax (with an ASTMatcher), not unlike the one implemented in this revision. This can be done on the whole TU in any check callback, so the availability of this information is not an issue. In case of CTU the AST is built during the analysis itself, so theoretically we would want to recheck the whole TU every time we use this information (but this seems overkill, and computationally intensive). So doing this only once is also an approximation.
  - Phase2: during symbolic execution, we check for call expressions of exit functions (`_Exit`, `exit`, `quick_exit`), and examine the call-stack to identify a parent function declaration, which is in the set of detected handler-functions (built during Phase 1). We report an error in this case.

I guess I will abandon this and will create a patch for ClangSA when I will have some time for it (can't really promise anything as of now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717