[PATCH] D60112: [analyzer] Treat write into a top-level parameter variable with destructor as escape.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, jdoerfert, dkrupp, donat.nagy, a.sidorin, szepet.
Herald added a project: clang.

Writing stuff into an argument variable is usually equivalent to writing stuff to a local variable: it will have no effect outside of the function. There's an important exception from this rule: if the argument variable has a non-trivial destructor, the destructor would be invoked on the parent stack frame, exposing contents of the otherwise dead argument variable to the caller.

We've had this problem in https://bugs.llvm.org/show_bug.cgi?id=37459#c3 where we weren't invalidating argument regions after the call. Such invalidation is completely unnecessary when the argument doesn't have a destructor, but it's vital when it does.

The newly added test case demonstrates the same problem but "inside out": when we're receiving an object with a non-trivial destructor as a top-level argument, we're exposing ourselves to the destructor call of this variable which we won't ever encounter during the current analysis because it'll only happen in the parent stack frame. Such destructor may do various stuff with values we put into the variable, such as deallocating memory owned by the object, but we won't see it and report spurious leaks.

Note that the parameter variable is dead after it's referenced for the last time within the function regardless of whether it has a destructor or not. The variable is dead because we can guarantee that we'll never be able to access it throughout the rest of the analysis. It indicates that all our knowledge about the variable is final. For example, if there's a pointer stored in this variable that's allocated, and it's not stored anywhere else, it won't be deallocated until the end of the analysis. This is why it is incorrect to simply make top-level parameter variables with destructors live forever: it contradicts the performance-related purpose of dead symbol collection, even if it does play nicely with the leak-finding purpose of dead symbols collection.

Therefore i believe that the right solution is to treat any writes into top-level parameters with destructors as //escapes//. The value is still stored there and something that's beyond our control (in this case, a destructor call) will happen to it and we cannot predict what exactly will happen. It's a typical escape scenario.

Well, in fact, we *can* predict what happens. After all, it happens immediately after the end of the analysis and we don't need to know anything about the caller stack frame in order to evaluate these destructors. So the right right solution is to just append destructor modeling to the end of the analysis. This, however, is going to be very hard to implement because you'll have to teach the analyzer how to behave correctly with a null location context - that's going to be a looooot of crashes to sort out.


Repository:
  rC Clang

https://reviews.llvm.org/D60112

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/malloc.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60112.193229.patch
Type: text/x-patch
Size: 4124 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190402/518062d5/attachment-0001.bin>