[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hey, that's a lot of progress already! =)



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:124
+    return;
+  updateTrackedRegion(Call, C, ThisValRegion);
+}
----------------
Not all constructors behave this way. In particular, if it's a copy/move constructor this function would track the value of the original smart pointer to this smart pointer, but what we need is to track the value of the raw pointer instead.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:175
+    return;
+  updateTrackedRegion(Call, C, ThisValRegion);
+}
----------------
When you're doing `evalCall`, you're responsible for all aspects of the call - not just your private GDM map but also the Store and the Environment.

For `.reset()` the other important thing to do would be to invalidate the region in the Store so that the Store did not think that it contains the old value. We can't set the new value correctly but invalidating it would still be better than doing nothing - simply because "not being sure" is not as bad as "being confidently incorrect". That said, once you model *all* methods of the smart pointers, you would probably no longer need to invalidate the Store, because it will never be actually accessed during analysis. So my conclusion here is:
- For this first patch invalidating the Store is probably not worth it but let's add a FIXME.
- We should make sure that we eventually add the invalidation if we don't have time to model all methods.

Now, you're calling this same function for `.release()` as well. And for `.release()` there is another important thing that we're responsible for: //model the return value//. The `.release()` method returns the raw pointer - which is something this checker is accidentally an expert on! - so let's `BindExpr()` the value of the call-expression to the value of the raw pointer. If you want to delay this exercise until later patches, i recommend to temporarily modeling the return value as a conjured symbol instead, but we cannot completely drop this part.

Another useful thing that we should do with `.release()` in the future is to tell `MallocChecker` to start tracking the raw pointer. This will allow us to emit memory leak warnings against it. Let's add a TODO for that as well.


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

https://reviews.llvm.org/D81315