[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:624
+        if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
+          if (!MR->hasStackStorage())
+            Escaped.push_back(State->getSVal(MR, Pointee));
----------------
xazax.hun wrote:
> NoQ wrote:
> > Ok, so this patch interacts with D71152 in a non-trivial manner. We should re-use the logic that decides whether an escape on bind occurs.
> Yeah, I just started to look into it and some skeletons fell out. 
> 
> Let's consider the following example:
> ```
> void leak() {
>   zx_handle_t sa, sb;
>   if (zx_channel_create(0, &sa, &sb))
>     return;
>   zx_handle_close(sb);
> } 
> ```
> 
> After using the same logic we can no longer detect the leak. The reason is, after `zx_channel_create` both `sa` and `sb` regions are escaped (and they become escaped during the conservative call) and then after the PostCall callback, when we attached the traits to our symbols, we will trigger pointer escape immediately, and lose the traits we just added.
> 
> I am not immediately sure what to do here. I feel like the main source of the problem is the fact that `zx_channel_create` should not escape anything and the checker has now way to communicate that to the analyzer core. Any ideas?
Suddenly, I see four ways forward, none of which is optimal. Hopefully, you can come up with something better.
1. Do not care about escaped local regions in this code path. So basically we would tune the invalidation down a bit. 
2. Make it possible for modeling checkers to prevent escaping. It would be great if there would be a way to do that without doing EvalCall.
3. Require the users to annotate out parameters. And we could assume that out parameters do not escape.
4. Do some heuristics like if a checker just attached some info in a PostCall, it is likely that we should not escape that symbol.


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

https://reviews.llvm.org/D71224