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

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:35
   bool isNullAfterMoveMethod(const CallEvent &Call) const;
+  BugType NullDereferenceBugType{this, "Null-smartPtr-deref",
+                                 "C++ smart pointer"};
----------------
xazax.hun wrote:
> Nit: We do not use hypens/dashes in diagnostic names.
Fixed the format


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:53
+  // Set of STL smart pointer class which we are trying to model.
+  const llvm::StringSet<> StdSmartPtrs = {
+      "shared_ptr",
----------------
xazax.hun wrote:
> It might be just a personal preference but I tend to put these at the top of the file for easier discoverability. Feel free to ignore this comment unless other reviewers have the same preference as me.
Thanks!
Moved to top.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
                                 CheckerContext &C) const {
-  if (!isNullAfterMoveMethod(Call))
+  if (isNullAfterMoveMethod(Call)) {
+    ProgramStateRef State = C.getState();
----------------
xazax.hun wrote:
> Don't we want this to be also protected by the `isStdSmartPointerClass` check?
added `isStdSmartPointerClass` check in the beginning.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:115
+  const auto *MethodDecl = dyn_cast<CXXMethodDecl>(Call.getDecl());
+  if (!MethodDecl || !isStdSmartPointerClass(MethodDecl->getParent()))
+    return;
----------------
xazax.hun wrote:
> I wonder if making `isStdSmartPointerClass` operate on `CallEvent` would simllify the call sites of this function.
Yeah. Thanks!
That makes it better.
Made changes to pass `CallEvent` to `isStdSmartPointerClass`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:124
+    return;
+  updateTrackedRegion(Call, C, ThisValRegion);
+}
----------------
NoQ wrote:
> NoQ wrote:
> > 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.
> Actually, let's add an assertion into `updateTrackedRegion` that the value that's put into the map is of pointer type. This would give us an automatic notification when we make such mistakes.
- Added check to filter out copy/move constructor
- Added  assert to check the value is of type pointer.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:137
+    if (IsRegDead) {
+      State = State->remove<TrackedRegionMap>(Region);
+    }
----------------
NoQ wrote:
> xazax.hun wrote:
> > In LLVM we often omit braces for small single statement branches. Also, Artem mentioned that we could interact with the malloc checker. Maybe it is worth considering to notify the malloc checker to mark the appropriate region as deallocated? This would help find double release errors, avoid spurious leak errors and so on. 
> > Maybe it is worth considering to notify the malloc checker to mark the appropriate region as deallocated?
> 
> This happens in destructor.
removed the braces


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:155
+
+  if (MethodDecl && MethodDecl->isOverloadedOperator()) {
+    OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
----------------
xazax.hun wrote:
> Early returns can decrease the indentation. 
Updated with early return. 
Thanks!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:175
+    return;
+  updateTrackedRegion(Call, C, ThisValRegion);
+}
----------------
NoQ wrote:
> 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.
- Added TODO for `reset()`
- Added `BindExpr()` for the value of the call-expression to the value of the raw pointer.
- Added TODO for `release()` to track raw pointer in future.


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

https://reviews.llvm.org/D81315