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

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

gamesh411 added a comment.

> Just to make sure we're on the same page -- the current approach is not flow-sensitive, and so my concern is that it won't report any true positives (not that it will be prone to false positives).

Sorry about that. You are absolutely right; what I was trying to say is CallGraph-based.

Just some thoughts on this example:

In D83717#2279263 <https://reviews.llvm.org/D83717#2279263>, @aaron.ballman wrote:

> One of the concerns I have with this not being a flow-sensitive check is that most of the bad situations are not going to be caught by the clang-tidy version of the check. The CERT rules show contrived code examples, but the more frequent issue looks like:
>
>   void cleanup(struct whatever *ptr) {
>     assert(ptr); // This potentially calls abort()
>     free(ptr->buffer);
>     free(ptr);
>   }
>   ...

What I have in support of this approach is this reasoning:
If a handler is used where either branch can abort then that branch is expected to be taken. Otherwise it is dead code. I would argue then, that this abortion should be refactored out of the handler function to ensure well-defined behaviour in every possible case.

As a counter-argument; suppose that there is a function that is used as both an exit-handler and as a simple invocation. In this case, I can understand if one would not want to factor the abortion logic out, or possibly pass flags around.

Then to this remark:

> The fact that we're not looking through the call sites (even without cross-TU support) means the check isn't going to catch the most problematic cases. You could modify the called function collector to gather this a bit better, but you'd issue false positives in flow-sensitive situations like:
>
>   void some_cleanup_func(void) {
>     for (size_t idx = 0; idx < GlobalElementCount; ++idx) {
>       struct whatever *ptr = GlobalElement[idx];
>       if (ptr) {
>         // Now we know abort() won't be called
>         cleanup(ptr);
>       }
>     }
>   }

The current approach definitely does not take 'adjacent' call-sites into account (not to mention CTU ones).
In this regard I also tend to see the benefit of this being a ClangSA checker as that would solve 3 problems at once:

1. Being path-sensitive, so we can explain how we got to the erroneous program-point
2. It utilizes CTU mode to take callsites from other TU-s into account
3. Runtime-stack building is implicitly done by ExprEngine as a side effect of symbolic execution

Counter-argument:
But using ClangSA also introduces a big challenge.
ClangSA analyzes all top-level functions during analysis. However I don't know if it understands the concept of exit-handlers, and I don't know a way of 'triggering' an analysis 'on-exit' so to speak.
So AFAIK this model of analyzing only top-level functions is a limitation when it comes to modelling the program behaviour 'on-exit'.
sidenote:
To validate this claim I have dumped the exploded graph of the following file:

  #include <cstdlib>
  #include <iostream>
  
  void f() {
    std::cout << "handler f";
  };
  
  int main() {
    std::atexit(f);
  }

And it has no mention of std::cout being used, so I concluded, that ClangSA does not model the 'on-exit' behaviour.

I wanted to clear these issues before I made the documentation.
Thanks for the effort and the tips on evaluating the solution, I will do some more exploration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717