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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: NoQ.
aaron.ballman added a subscriber: NoQ.
aaron.ballman added a comment.
Herald added a subscriber: Charusso.

In D83717#2366598 <https://reviews.llvm.org/D83717#2366598>, @gamesh411 wrote:

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

If the assert was directly within the handler code, then sure. However, consider a situation like this:

  struct Something {
    Something(int *ip) { assert(ip); ... }
    ...
  };

where the use of the assertion is far removed from the fact that it's being used within a handler.

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

Yes, this is exactly the situation I'm worried about.

> 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

Agreed.

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

I'm hoping someone more well-versed in the details of the static analyzer can speak to this point. @NoQ @Szelethus others?

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

Thank you for looking into this! If it turns out that there's some reason why the static analyzer cannot be at least as good of a home for the functionality as clang-tidy, that would be really interesting to learn. Either there are improvements we could consider making to the static analyzer, or we could leave the check in clang-tidy despite the limitations, but there's still a path forward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717