[analyzer] Add modeling of Eq operator in smart ptr

[vrnithinkumar]

Summary

Support for std::unique_ptr>::operator=

[vrnithinkumar]

• Add assignment to nullptr case

[NoQ]

This looks outright correct to me. I have random suggestions on note text here and there.

clang/test/Analysis/smart-ptr-text-output.cpp

131

I suggest: Null pointer value move-assigned to 'P'.

138

I suggest: Smart pointer 'PToMove' is null; previous value moved to 'P'. Or maybe instead keep the note that the move-checker currently emits?

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

353

I'd prefer to call this AssignOp to avoid confusion with ==. While your naming is correct, I always found this nomenclature confusing.

386

I also find the names of the variables confusing.

Instead of ArgRegion what about OtherSmartPtrRegion?
Instead of ArgRegionVal what about OtherInnerPtr?

393

Adding return after every addTransition is a good practive that we should follow.

421

Isn't this the same as the beginning of the note tag above?

I wonder if there is a way to deduplicate this code. Not a huge priority though as I do not have an immediate idea for doing this in a clean way.

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

353

IMO it's not a question of preference with this one, EqOp is misleading.

401

The grammar seems off in this one. I think it should be smith like "after being moved to" or "after it was moved to".

clang/test/Analysis/smart-ptr-text-output.cpp

131

+1

[vrnithinkumar]

• Addressing review comments

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

353

Changed to AssignOp

386

Changed to OtherSmartPtrRegion and OtherInnerPtr

393

Added return

401

Changed to "after being moved to"

421

Yes.
The check for bug type is duplicated across all the note tags.

clang/test/Analysis/smart-ptr-text-output.cpp

131

Updated to Null pointer value move-assigned to 'P'

138

Going with first option "Smart pointer 'PToMove' is null; previous value moved to 'P'"

[NoQ]

Looks amazing now.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

 
 using namespace clang;
 using namespace ento;
 
 namespace {
 class SmartPtrModeling
     : public Checker<eval::Call, check::DeadSymbols, check::RegionChanges> {
 
-  bool isNullAfterMoveMethod(const CallEvent &Call) const;
+  bool isAssignOpMethod(const CallEvent &Call) const;
 
 public:
   // Whether the checker should model for null dereferences of smart pointers.
   DefaultBool ModelSmartPtrDereference;
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   ProgramStateRef
   checkRegionChanges(ProgramStateRef State,
                      const InvalidatedSymbols *Invalidated,
                      ArrayRef<const MemRegion *> ExplicitRegions,
                      ArrayRef<const MemRegion *> Regions,
                      const LocationContext *LCtx, const CallEvent *Call) const;
 
 private:
   void handleReset(const CallEvent &Call, CheckerContext &C) const;
   void handleRelease(const CallEvent &Call, CheckerContext &C) const;
   void handleSwap(const CallEvent &Call, CheckerContext &C) const;
   void handleGet(const CallEvent &Call, CheckerContext &C) const;
+  bool handleAssignOp(const CallEvent &Call, CheckerContext &C) const;
 
   using SmartPtrMethodHandlerFn =
       void (SmartPtrModeling::*)(const CallEvent &Call, CheckerContext &) const;
   CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
       {{"reset"}, &SmartPtrModeling::handleReset},
       {{"release"}, &SmartPtrModeling::handleRelease},
       {{"swap", 1}, &SmartPtrModeling::handleSwap},
       {{"get"}, &SmartPtrModeling::handleGet}};
static ProgramStateRef updateSwappedRegion(ProgramStateRef State,
   if (RegionInnerPointerVal) {
     State = State->set<TrackedRegionMap>(Region, *RegionInnerPointerVal);
   } else {
     State = State->remove<TrackedRegionMap>(Region);
   }
   return State;
 }
 
-bool SmartPtrModeling::isNullAfterMoveMethod(const CallEvent &Call) const {
+bool SmartPtrModeling::isAssignOpMethod(const CallEvent &Call) const {
   // TODO: Update CallDescription to support anonymous calls?
   // TODO: Handle other methods, such as .get() or .release().
   // But once we do, we'd need a visitor to explain null dereferences
   // that are found via such modeling.
   const auto *CD = dyn_cast_or_null<CXXConversionDecl>(Call.getDecl());
   return CD && CD->getConversionType()->isBooleanType();
 }
 
 bool SmartPtrModeling::evalCall(const CallEvent &Call,
                                 CheckerContext &C) const {
-
   ProgramStateRef State = C.getState();
   if (!smartptr::isStdSmartPtrCall(Call))
     return false;
 
-  if (isNullAfterMoveMethod(Call)) {
+  if (isAssignOpMethod(Call)) {
     const MemRegion *ThisR =
         cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
 
     if (!move::isMovedFrom(State, ThisR)) {
       // TODO: Model this case as well. At least, avoid invalidation of
       // globals.
       return false;
     }
if (Call.getNumArgs() == 0) {
           OS << " is constructed using a null value";
         else
           OS << " is constructed";
       }));
     }
     return true;
   }
 
+  if (handleAssignOp(Call, C))
+    return true;
+
   const SmartPtrMethodHandlerFn *Handler = SmartPtrMethodHandlers.lookup(Call);
   if (!Handler)
     return false;
   (this->**Handler)(Call, C);
 
   return C.isDifferent();
 }
 
C.addTransition(
         BR.markInteresting(ArgRegion);
         OS << "Swapped null smart pointer ";
         ArgRegion->printPretty(OS);
         OS << " with smart pointer ";
         ThisRegion->printPretty(OS);
       }));
 }
 
 void SmartPtrModeling::handleGet(const CallEvent &Call,
                                  CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   const auto *IC = dyn_cast<CXXInstanceCall>(&Call);
   if (!IC)
     return;
 
   const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
   if (!ThisRegion)
void SmartPtrModeling::handleGet(const CallEvent &Call,
   }
 
   State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
                           InnerPointerVal);
   // TODO: Add NoteTag, for how the raw pointer got using 'get' method.
   C.addTransition(State);
 }
 
+bool SmartPtrModeling::handleAssignOp(const CallEvent &Call,
+                                      CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  const auto *OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+  if (!OC)
+    return false;
+  OverloadedOperatorKind OOK = OC->getOverloadedOperator();
+  if (OOK != OO_Equal)
+    return false;
+  const MemRegion *ThisRegion = OC->getCXXThisVal().getAsRegion();
+  if (!ThisRegion)
+    return false;
+
+  const MemRegion *OtherSmartPtrRegion = OC->getArgSVal(0).getAsRegion();
+  // In case of 'nullptr' or '0' assigned
+  if (!OtherSmartPtrRegion) {
+    bool AssignedNull = Call.getArgSVal(0).isZeroConstant();
+    if (!AssignedNull)
+      return false;
+    auto NullVal = C.getSValBuilder().makeNull();
+    State = State->set<TrackedRegionMap>(ThisRegion, NullVal);
+    C.addTransition(State, C.getNoteTag([ThisRegion](PathSensitiveBugReport &BR,
+                                                     llvm::raw_ostream &OS) {
+      if (&BR.getBugType() != smartptr::getNullDereferenceBugType() ||
+          !BR.isInteresting(ThisRegion))
+        return;
+      OS << "Smart pointer ";
+      ThisRegion->printPretty(OS);
+      OS << " is assigned to null";
+    }));
+    return true;
+  }
+
+  const auto *OtherInnerPtr = State->get<TrackedRegionMap>(OtherSmartPtrRegion);
+  if (OtherInnerPtr) {
+    State = State->set<TrackedRegionMap>(ThisRegion, *OtherInnerPtr);
+    auto NullVal = C.getSValBuilder().makeNull();
+    State = State->set<TrackedRegionMap>(OtherSmartPtrRegion, NullVal);
+    bool IsArgValNull = OtherInnerPtr->isZeroConstant();
+
+    C.addTransition(
+        State,
+        C.getNoteTag([ThisRegion, OtherSmartPtrRegion, IsArgValNull](
+                         PathSensitiveBugReport &BR, llvm::raw_ostream &OS) {
+          if (&BR.getBugType() != smartptr::getNullDereferenceBugType())
+            return;
+          if (BR.isInteresting(OtherSmartPtrRegion)) {
+            OS << "Smart pointer ";
+            OtherSmartPtrRegion->printPretty(OS);
+            OS << " is null after being moved to ";
+            ThisRegion->printPretty(OS);
+          }
+          if (BR.isInteresting(ThisRegion) && IsArgValNull) {
+            OS << "Null pointer value move-assigned to ";
+            ThisRegion->printPretty(OS);
+            BR.markInteresting(OtherSmartPtrRegion);
+          }
+        }));
+    return true;
+  } else {
+    // In case we dont know anything about value we are moving from
+    // remove the entry from map for which smart pointer got moved to.
+    auto NullVal = C.getSValBuilder().makeNull();
+    State = State->remove<TrackedRegionMap>(ThisRegion);
+    State = State->set<TrackedRegionMap>(OtherSmartPtrRegion, NullVal);
+    C.addTransition(State, C.getNoteTag([OtherSmartPtrRegion,
+                                         ThisRegion](PathSensitiveBugReport &BR,
+                                                     llvm::raw_ostream &OS) {
+      if (&BR.getBugType() != smartptr::getNullDereferenceBugType() ||
+          !BR.isInteresting(OtherSmartPtrRegion))
+        return;
+      OS << "Smart pointer ";
+      OtherSmartPtrRegion->printPretty(OS);
+      OS << " is null after; previous value moved to ";
+      ThisRegion->printPretty(OS);
+    }));
+    return true;
+  }
+  return false;
+}
+
 void ento::registerSmartPtrModeling(CheckerManager &Mgr) {
   auto *Checker = Mgr.registerChecker<SmartPtrModeling>();
   Checker->ModelSmartPtrDereference =
       Mgr.getAnalyzerOptions().getCheckerBooleanOption(
           Checker, "ModelSmartPtrDereference");
 }
 
 bool ento::shouldRegisterSmartPtrModeling(const CheckerManager &mgr) {
   const LangOptions &LO = mgr.getLangOpts();
   return LO.CPlusPlus;
 }

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

public:
   T *release() noexcept;
   void reset(T *p = nullptr) noexcept;
   void swap(unique_ptr<T> &p) noexcept;
 
   typename std::add_lvalue_reference<T>::type operator*() const;
   T *operator->() const noexcept;
   operator bool() const noexcept;
   unique_ptr<T> &operator=(unique_ptr<T> &&p) noexcept;
+  unique_ptr<T> &operator=(nullptr_t) noexcept;
 };
 
 // TODO :: Once the deleter parameter is added update with additional template parameter.
 template <typename T>
 void swap(unique_ptr<T> &x, unique_ptr<T> &y) noexcept {
   x.swap(y);
 }
 } // namespace std

clang/test/Analysis/smart-ptr-text-output.cpp

void derefOnSwappedNullPtr() {
   (*P).foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
 // FIXME: Fix this test when "std::swap" is modeled seperately.
 void derefOnStdSwappedNullPtr() {
   std::unique_ptr<A> P; // expected-note {{Default constructed smart pointer 'P' is null}}
   std::unique_ptr<A> PNull; // expected-note {{Default constructed smart pointer 'PNull' is null}}
-  std::swap(P, PNull); // expected-note@Inputs/system-header-simulator-cxx.h:978 {{Swapped null smart pointer 'PNull' with smart pointer 'P'}}
+  std::swap(P, PNull); // expected-note@Inputs/system-header-simulator-cxx.h:979 {{Swapped null smart pointer 'PNull' with smart pointer 'P'}}
   // expected-note@-1 {{Calling 'swap<A>'}}
   // expected-note@-2 {{Returning from 'swap<A>'}}
   P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
 struct StructWithSmartPtr { // expected-note {{Default constructed smart pointer 'S.P' is null}}
   std::unique_ptr<A> P;
void noNoteTagsForNonInterestingRegion() {
   std::unique_ptr<A> P2; // No note.
   P1.release(); // No note.
   P1.reset(); // No note.
   P1.swap(P2); // No note.
   P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
-void noNoteTagsForNonMatchingBugType() {
-  std::unique_ptr<A> P; // No note.
-  std::unique_ptr<A> P1; // No note.
-  P1 = std::move(P); // expected-note {{Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [cplusplus.Move]}}
-  // expected-note@-1 {{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
-}
-
 void derefOnRawPtrFromGetOnNullPtr() {
   std::unique_ptr<A> P; // FIXME: add note "Default constructed smart pointer 'P' is null"
   P.get()->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
   // expected-note@-1 {{Called C++ object pointer is null}}
 }
 
 void derefOnRawPtrFromGetOnValidPtr() {
   std::unique_ptr<A> P(new A());
   P.get()->foo(); // No warning.
 }
 
 void derefOnRawPtrFromGetOnUnknownPtr(std::unique_ptr<A> P) {
   P.get()->foo(); // No warning.
 }
+
+void derefOnMovedFromValidPtr() {
+  std::unique_ptr<A> PToMove(new A());  // expected-note {{Smart pointer 'PToMove' is constructed}}
+  // FIXME: above note should go away once we fix marking region not interested. 
+  std::unique_ptr<A> P;
+  P = std::move(PToMove); // expected-note {{Smart pointer 'PToMove' is null after being moved to 'P'}}
+  PToMove->foo(); // expected-warning {{Dereference of null smart pointer 'PToMove' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1 {{Dereference of null smart pointer 'PToMove'}}
+}
+
+void derefOnMovedToNullPtr() {
+  std::unique_ptr<A> PToMove(new A());
+  std::unique_ptr<A> P;
+  P = std::move(PToMove); // No note.
+  P->foo(); // No warning.
+}
+
+void derefOnNullPtrGotMovedFromValidPtr() {
+  std::unique_ptr<A> P(new A()); // expected-note {{Smart pointer 'P' is constructed}}
+  // FIXME: above note should go away once we fix marking region not interested. 
+  std::unique_ptr<A> PToMove; // expected-note {{Default constructed smart pointer 'PToMove' is null}}
+  P = std::move(PToMove); // expected-note {{Null pointer value move-assigned to 'P'}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1 {{Dereference of null smart pointer 'P'}}
+}
+
+void derefOnMovedUnknownPtr(std::unique_ptr<A> PToMove) {
+  std::unique_ptr<A> P;
+  P = std::move(PToMove); // expected-note {{Smart pointer 'PToMove' is null after; previous value moved to 'P'}}
+  PToMove->foo(); // expected-warning {{Dereference of null smart pointer 'PToMove' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1 {{Dereference of null smart pointer 'PToMove'}}
+}
+
+void derefOnAssignedNullPtrToNullSmartPtr() {
+  std::unique_ptr<A> P; // expected-note {{Default constructed smart pointer 'P' is null}}
+  P = nullptr; // expected-note {{Smart pointer 'P' is assigned to null}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1 {{Dereference of null smart pointer 'P'}}
+}
+
+void derefOnAssignedZeroToNullSmartPtr() {
+  std::unique_ptr<A> P(new A()); // expected-note {{Smart pointer 'P' is constructed}}
+  // FIXME: above note should go away once we fix marking region not interested. 
+  P = 0; // expected-note {{Smart pointer 'P' is assigned to null}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1 {{Dereference of null smart pointer 'P'}}
+}

clang/test/Analysis/smart-ptr.cpp

void derefAfterAssignment() {
     std::unique_ptr<A> Q;
     Q = std::move(P);
     Q->foo(); // no-warning
   }
   {
     std::unique_ptr<A> P;
     std::unique_ptr<A> Q;
     Q = std::move(P);
-    // TODO: Fix test with expecting warning after '=' operator overloading modeling.
-    Q->foo(); // no-warning
+    Q->foo(); // expected-warning {{Dereference of null smart pointer 'Q' [alpha.cplusplus.SmartPtr]}}
   }
 }
 
 void derefOnSwappedNullPtr() {
   std::unique_ptr<A> P(new A());
   std::unique_ptr<A> PNull;
   P.swap(PNull);
   PNull->foo(); // No warning.

 void derefOnRawPtrFromMultipleGetOnUnknownPtr(std::unique_ptr<A> P) {
   A *X = P.get();
   A *Y = P.get();
   clang_analyzer_eval(X == Y); // expected-warning{{TRUE}}
   if (!X) {
     Y->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
   }
 }
+
+void derefOnMovedFromValidPtr() {
+  std::unique_ptr<A> PToMove(new A());
+  std::unique_ptr<A> P;
+  P = std::move(PToMove);
+  PToMove->foo(); // expected-warning {{Dereference of null smart pointer 'PToMove' [alpha.cplusplus.SmartPtr]}}
+}
+
+void derefOnMovedToNullPtr() {
+  std::unique_ptr<A> PToMove(new A());
+  std::unique_ptr<A> P;
+  P = std::move(PToMove); // No note.
+  P->foo(); // No warning.
+}
+
+void derefOnNullPtrGotMovedFromValidPtr() {
+  std::unique_ptr<A> P(new A());
+  std::unique_ptr<A> PToMove;
+  P = std::move(PToMove);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+}
+
+void derefOnMovedFromUnknownPtr(std::unique_ptr<A> PToMove) {
+  std::unique_ptr<A> P;
+  P = std::move(PToMove);
+  P->foo(); // No warning.
+}
+
+void derefOnMovedUnknownPtr(std::unique_ptr<A> PToMove) {
+  std::unique_ptr<A> P;
+  P = std::move(PToMove);
+  PToMove->foo(); // expected-warning {{Dereference of null smart pointer 'PToMove' [alpha.cplusplus.SmartPtr]}}
+}
+
+void derefOnAssignedNullPtrToNullSmartPtr() {
+  std::unique_ptr<A> P;
+  P = nullptr;
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+}
+
+void derefOnAssignedZeroToNullSmartPtr() {
+  std::unique_ptr<A> P(new A());
+  P = 0;
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+}
+
+void derefOnAssignedNullToUnknowSmartPtr(std::unique_ptr<A> P) {
+  P = nullptr;
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+}
+
+std::unique_ptr<A> &&returnRValRefOfUniquePtr();
+
+void drefOnAssignedNullFromMethodPtrValidSmartPtr() {
+  std::unique_ptr<A> P(new A());
+  P = returnRValRefOfUniquePtr();
+  P->foo(); // No warning. 
+}