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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
