[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

132

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

139

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

351

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

384

I also find the names of the variables confusing.

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

391

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

419

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

351

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

399

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

132

+1

[vrnithinkumar]

• Addressing review comments

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

351

Changed to AssignOp

384

Changed to OtherSmartPtrRegion and OtherInnerPtr

391

Added return

399

Changed to "after being moved to"

419

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

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

132

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

139

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

[NoQ]

Looks amazing now.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

 // SmartPtrModeling.cpp - Model behavior of C++ smart pointers - C++ ------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
checkRegionChanges(ProgramStateRef State,
                      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;
+  bool handleEqOp(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}};
 };
bool SmartPtrModeling::isNullAfterMoveMethod(const CallEvent &Call) const {
   // 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)) {
     const MemRegion *ThisR =
         cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
 
if (Call.getNumArgs() == 0) {
           OS << " is constructed using a null value";
         else
           OS << " is constructed";
       }));
     }
     return true;
   }
 
+  if (handleEqOp(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);
       }));
 }
 
+bool SmartPtrModeling::handleEqOp(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 *ArgRegion = OC->getArgSVal(0).getAsRegion();
+  // In case of 'nullptr' or '0' assigned
+  if (!ArgRegion) {
+    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 *ArgRegionVal = State->get<TrackedRegionMap>(ArgRegion);
+  if (ArgRegionVal) {
+    State = State->set<TrackedRegionMap>(ThisRegion, *ArgRegionVal);
+    auto NullVal = C.getSValBuilder().makeNull();
+    State = State->set<TrackedRegionMap>(ArgRegion, NullVal);
+    bool IsArgValNull = ArgRegionVal->isZeroConstant();
+
+    C.addTransition(State, C.getNoteTag([ThisRegion, ArgRegion, IsArgValNull](
+                                            PathSensitiveBugReport &BR,
+                                            llvm::raw_ostream &OS) {
+      if (&BR.getBugType() != smartptr::getNullDereferenceBugType())
+        return;
+      if (BR.isInteresting(ArgRegion)) {
+        OS << "Smart pointer ";
+        ArgRegion->printPretty(OS);
+        OS << " is null after moved to ";
+        ThisRegion->printPretty(OS);
+      }
+      if (BR.isInteresting(ThisRegion) && IsArgValNull) {
+        OS << "Smart pointer ";
+        ThisRegion->printPretty(OS);
+        OS << " is null after a null value moved from ";
+        ArgRegion->printPretty(OS);
+        BR.markInteresting(ArgRegion);
+      }
+    }));
+  } 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>(ArgRegion, NullVal);
+    C.addTransition(
+        State, C.getNoteTag([ArgRegion, ThisRegion](PathSensitiveBugReport &BR,
+                                                    llvm::raw_ostream &OS) {
+          if (&BR.getBugType() != smartptr::getNullDereferenceBugType() ||
+              !BR.isInteresting(ArgRegion))
+            return;
+          OS << "Smart pointer ";
+          ArgRegion->printPretty(OS);
+          OS << " is null after moved and assigned to ";
+          ThisRegion->printPretty(OS);
+        }));
+  }
+  return true;
+}
+
 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 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 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 {{Smart pointer 'P' is null after a null value moved from 'PToMove'}}
+  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 moved and assigned 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 derefOnSwappedValidPtr() {
   std::unique_ptr<A> PValid(new A());
   P.swap(PValid);
   (*P).foo(); // No warning.
   PValid->foo(); // No warning.
   std::swap(P, PValid);
   P->foo(); // No warning.
   PValid->foo(); // No warning.
 }
+
+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. 
+}