[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.

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);
>   }
>   
>   void some_cleanup_func(void) {
>     for (size_t idx = 0; idx < GlobalElementCount; ++idx) {
>       cleanup(GlobalElement[idx]);
>     }
>   }
>   
>   void some_exit_handler(void) {
>     ...
>     some_cleanup_func();
>     ...
>   }
>
> 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);
>       }
>     }
>   }
>
> Have you run this check over any large code bases to see if it currently catches any true positive diagnostics?

I have tried llvm, tmux, curl and tried codesearch.com to look for other sources containing `atexit`, but no results were found. So it is hard to see whether this flow-sensitive approach would result in many false positives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717