[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> And about `checkDeadSymbols`,  I get your point, but I am interested why checker has different behavior on these two examples:
>
>   a1 int main(int argc, char **argv, char *envp[]) {
>   a2   putenv((char*) "NAME=VALUE"); // envp invalidated
>   a3   envp[0]; // gives error
>   a4 }
>
>
>
>   b1 int main(int argc, char **argv, char *envp[]) {
>   b2   char **e = envp;
>   b3   putenv((char*) "NAME=VALUE"); // envp invalidated
>   b4   e[0]; // does not give error
>   b5 } 
>
> If I manually force keeping `envp` region in state when I check `if (!SymReaper.isLiveRegion(E)) {` , then it reports as expected.

This is because we use the result of the live variable analysis to drive the cleanup mechanism (i.e. garbage collection) of the analyzer. This data-flow analysis is finished before the path sensitive symbolic execution is started. In the first example, `envp` becomes dead at line `a3` because that is the last point where we read it. In the second exmplae, `envp` becomes dead at line `b2` because that is the last location where we read it's value. And it seems like our live variable analysis does not handle aliasing.
In my humble opinion, the engine should be using a lexical based scoping analysis to drive the callbacks of `checkDeadSymbols`. Or, perhaps, we should have another callback that is called at the end of the scope based lifetime. (RFC @NoQ). Currently, the engine might report a symbol/region as dead even if that is still live in the lexical scoping sense.
On the other hand, having the cleanup mechanism eagerly removing dead symbols and all of their dependency helps the analyzer to keep it's State as small as absolutely needed, all the time. This way it can be as terse in the memory and as fast in runtime as possible.


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

https://reviews.llvm.org/D97699