Fix unique_ptr aggregate initialization false positives

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -231,12 +231,15 @@ class RefState {
 
   LLVM_DUMP_METHOD void dump(raw_ostream &OS) const {
     switch (K) {
-#define CASE(ID) case ID: OS << #ID; break;
-    CASE(Allocated)
-    CASE(AllocatedOfSizeZero)
-    CASE(Released)
-    CASE(Relinquished)
-    CASE(Escaped)
+#define CASE(ID)                                                               \
+  case ID:                                                                     \
+    OS << #ID;                                                                 \
+    break;
+      CASE(Allocated)
+      CASE(AllocatedOfSizeZero)
+      CASE(Released)
+      CASE(Relinquished)
+      CASE(Escaped)
     }
   }
 
@@ -414,7 +417,7 @@ class MallocChecker
   // This last frontend is associated with a single bug type which is not used
   // elsewhere and has a different bug category, so it's declared separately.
   CheckerFrontendWithBugType TaintedAllocChecker{"Tainted Memory Allocation",
-                                                 categories::TaintedData};
+                                                categories::TaintedData};
 
   using LeakInfo = std::pair<const ExplodedNode *, const MemRegion *>;
 
@@ -430,7 +433,7 @@ class MallocChecker
   ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond,
                             bool Assumption) const;
   void checkLocation(SVal l, bool isLoad, const Stmt *S,
-                     CheckerContext &C) const;
+                    CheckerContext &C) const;
 
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
                                     const InvalidatedSymbols &Escaped,
@@ -3012,7 +3015,7 @@ void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N,
 }
 
 void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
-                                     CheckerContext &C) const
+                                     CheckerContext &C) const 
 {
   ProgramStateRef state = C.getState();
   RegionStateTy OldRS = state->get<RegionState>();
@@ -3074,6 +3077,43 @@ void MallocChecker::checkPostCall(const CallEvent &Call,
     (*PostFN)(this, C.getState(), Call, C);
     return;
   }
+
+  ProgramStateRef State = C.getState();
+
+  if (const auto *Ctor = dyn_cast<CXXConstructorCall>(&Call)) {
+    // Ensure we are constructing a concrete object/subobject.
+    if (const MemRegion *ObjUnderConstr = Ctor->getCXXThisVal().getAsRegion()) {
+      ProgramStateRef NewState = State;
+
+      for (unsigned I = 0, E = Call.getNumArgs(); I != E; ++I) {
+        SVal ArgV = Call.getArgSVal(I);
+
+        SymbolRef Sym = ArgV.getAsSymbol();
+        if (!Sym)
+          continue;
+
+        // Look up current ref-state for this symbol in the RegionState map.
+        if (const RefState *RS = State->get<RegionState>(Sym)) {
+          // Only re-label symbols that are still owned allocations from C++
+          // new/new[].
+          if (RS->isAllocated() &&
+              (RS->getAllocationFamily().Kind == AF_CXXNew ||
+               RS->getAllocationFamily().Kind == AF_CXXNewArray)) {
+
+            // Mark as Relinquished at the constructor site: ownership moves
+            // into the constructed subobject. Pass the ctor's origin expr as
+            // the statement associated with this transition.
+            NewState = NewState->set<RegionState>(
+                Sym, RefState::getRelinquished(RS->getAllocationFamily(),
+                                               Ctor->getOriginExpr()));
+          }
+        }
+      }
+
+      if (NewState != State)
+        C.addTransition(NewState);
+    }
+  }
 }
 
 void MallocChecker::checkPreCall(const CallEvent &Call,
@@ -3358,9 +3398,8 @@ ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state,
 }
 
 bool MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly(
-                                              const CallEvent *Call,
-                                              ProgramStateRef State,
-                                              SymbolRef &EscapingSymbol) const {
+    const CallEvent *Call, ProgramStateRef State,
+    SymbolRef &EscapingSymbol) const {
   assert(Call);
   EscapingSymbol = nullptr;
 
@@ -3532,7 +3571,7 @@ ProgramStateRef MallocChecker::checkPointerEscape(ProgramStateRef State,
 ProgramStateRef MallocChecker::checkConstPointerEscape(ProgramStateRef State,
                                               const InvalidatedSymbols &Escaped,
                                               const CallEvent *Call,
-                                              PointerEscapeKind Kind) const {
+                                              PointerEscapeKind Kind) const { 
   // If a const pointer escapes, it may not be freed(), but it could be deleted.
   return checkPointerEscapeAux(State, Escaped, Call, Kind,
                                /*IsConstPointerEscape*/ true);
@@ -3724,61 +3763,61 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
         }
         Msg = OS.str();
         break;
-        }
-        case AF_None:
-          assert(false && "Unhandled allocation family!");
-          return nullptr;
-        }
+      }
+      case AF_None:
+        assert(false && "Unhandled allocation family!");
+        return nullptr;
+      }
 
-        // Record the stack frame that is _responsible_ for this memory release
-        // event. This will be used by the false positive suppression heuristics
-        // that recognize the release points of reference-counted objects.
-        //
-        // Usually (e.g. in C) we say that the _responsible_ stack frame is the
-        // current innermost stack frame:
-        ReleaseFunctionLC = CurrentLC->getStackFrame();
-        // ...but if the stack contains a destructor call, then we say that the
-        // outermost destructor stack frame is the _responsible_ one:
-        for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-          if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
-            if (isReferenceCountingPointerDestructor(DD)) {
-              // This immediately looks like a reference-counting destructor.
-              // We're bad at guessing the original reference count of the
-              // object, so suppress the report for now.
-              BR.markInvalid(getTag(), DD);
-
-              // After report is considered invalid there is no need to proceed
-              // futher.
-              return nullptr;
-            }
-
-            // Switch suspection to outer destructor to catch patterns like:
-            // (note that class name is distorted to bypass
-            // isReferenceCountingPointerDestructor() logic)
-            //
-            // SmartPointr::~SmartPointr() {
-            //  if (refcount.fetch_sub(1) == 1)
-            //    release_resources();
-            // }
-            // void SmartPointr::release_resources() {
-            //   free(buffer);
-            // }
-            //
-            // This way ReleaseFunctionLC will point to outermost destructor and
-            // it would be possible to catch wider range of FP.
-            //
-            // NOTE: it would be great to support smth like that in C, since
-            // currently patterns like following won't be supressed:
-            //
-            // void doFree(struct Data *data) { free(data); }
-            // void putData(struct Data *data)
-            // {
-            //   if (refPut(data))
-            //     doFree(data);
-            // }
-            ReleaseFunctionLC = LC->getStackFrame();
+      // Record the stack frame that is _responsible_ for this memory release
+      // event. This will be used by the false positive suppression heuristics
+      // that recognize the release points of reference-counted objects.
+      //
+      // Usually (e.g. in C) we say that the _responsible_ stack frame is the
+      // current innermost stack frame:
+      ReleaseFunctionLC = CurrentLC->getStackFrame();
+      // ...but if the stack contains a destructor call, then we say that the
+      // outermost destructor stack frame is the _responsible_ one:
+      for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+        if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
+          if (isReferenceCountingPointerDestructor(DD)) {
+            // This immediately looks like a reference-counting destructor.
+            // We're bad at guessing the original reference count of the
+            // object, so suppress the report for now.
+            BR.markInvalid(getTag(), DD);
+
+            // After report is considered invalid there is no need to proceed
+            // futher.
+            return nullptr;
           }
+
+          // Switch suspection to outer destructor to catch patterns like:
+          // (note that class name is distorted to bypass
+          // isReferenceCountingPointerDestructor() logic)
+          //
+          // SmartPointr::~SmartPointr() {
+          //  if (refcount.fetch_sub(1) == 1)
+          //    release_resources();
+          // }
+          // void SmartPointr::release_resources() {
+          //   free(buffer);
+          // }
+          //
+          // This way ReleaseFunctionLC will point to outermost destructor and
+          // it would be possible to catch wider range of FP.
+          //
+          // NOTE: it would be great to support smth like that in C, since
+          // currently patterns like following won't be supressed:
+          //
+          // void doFree(struct Data *data) { free(data); }
+          // void putData(struct Data *data)
+          // {
+          //   if (refPut(data))
+          //     doFree(data);
+          // }
+          ReleaseFunctionLC = LC->getStackFrame();
         }
+      }
 
     } else if (isRelinquished(RSCurr, RSPrev, S)) {
       Msg = "Memory ownership is transferred";

clang/test/Analysis/NewDeleteLeaks.cpp

@@ -11,9 +11,30 @@
 // RUN:   -analyzer-checker=unix \
 // RUN:   -analyzer-config \
 // RUN:     unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes=true
+// RUN: %clang_analyze_cc1 -std=c++20 -analyzer-checker=cplusplus.NewDeleteLeaks -verify %s
 
 #include "Inputs/system-header-simulator-for-malloc.h"
 
+// Minimal move, no headers needed, C++11+
+namespace nstd {
+
+template <class T>
+struct remove_reference { using type = T; };
+template <class T>
+struct remove_reference<T&> { using type = T; };
+template <class T>
+struct remove_reference<T&&> { using type = T; };
+
+template <class T>
+constexpr typename remove_reference<T>::type&& move(T&& t) noexcept {
+    using U = typename remove_reference<T>::type;
+    return static_cast<U&&>(t);
+}
+
+} // namespace nstd
+
+
+
 //===----------------------------------------------------------------------===//
 // Report for which we expect NoOwnershipChangeVisitor to add a new note.
 //===----------------------------------------------------------------------===//
@@ -218,3 +239,138 @@ void caller() {
   (void)n;
 } // no-warning: No potential memory leak here, because that's been already reported.
 } // namespace symbol_reaper_lifetime
+
+
+// Minimal RAII class that properly deletes its pointer.
+class Bar {
+public:
+  explicit Bar(int *ptr) : ptr_(ptr) {}
+  ~Bar() {
+    if (ptr_) {
+      delete ptr_;
+      ptr_ = nullptr;
+    }
+  }
+
+  Bar(const Bar &) = delete;
+  Bar &operator=(const Bar &) = delete;
+
+  Bar(Bar &&other) noexcept : ptr_(other.ptr_) { other.ptr_ = nullptr; }
+  Bar &operator=(Bar &&other) noexcept {
+    if (this != &other) {
+      delete ptr_;
+      ptr_ = other.ptr_;
+      other.ptr_ = nullptr;
+    }
+    return *this;
+  }
+
+  int operator*() const { return *ptr_; }
+
+private:
+  int *ptr_;
+};
+
+// Factory returning a prvalue Bar that owns a freshly allocated int.
+static Bar make_bar(int v) { return Bar(new int(v)); }
+
+struct Foo {
+  Bar a;
+  Bar b;
+};
+
+struct FooWithConstructor {
+  Bar a;
+  Bar b;
+  FooWithConstructor(Bar &&original_a, Bar &&original_b)
+      : a(nstd::move(original_a)), b(nstd::move(original_b)) {}
+};
+
+//===----------------------------------------------------------------------===//
+// No-false-positive regression tests: these must be silent
+//===----------------------------------------------------------------------===//
+
+namespace prvalue_aggregate_transfer {
+
+void ok_aggregate_from_factory() {
+  Foo foo = {make_bar(1), make_bar(2)}; // expected-no-diagnostics
+}
+
+void ok_aggregate_from_temporary_exprs() {
+  Foo foo = {Bar(new int(1)), Bar(new int(2))}; // expected-no-diagnostics
+}
+
+void ok_ctor_from_factory_rvalues() {
+  FooWithConstructor foo = {make_bar(1), make_bar(2)}; // expected-no-diagnostics
+}
+
+} // namespace prvalue_aggregate_transfer
+
+//===----------------------------------------------------------------------===//
+// True-positive regression tests: these should still warn
+//===----------------------------------------------------------------------===//
+
+class BarNoDelete {
+public:
+  explicit BarNoDelete(int *ptr) : ptr_(ptr) {}
+  ~BarNoDelete() {} // intentionally missing delete -> leak
+
+  BarNoDelete(const BarNoDelete &) = delete;
+  BarNoDelete &operator=(const BarNoDelete &) = delete;
+
+  BarNoDelete(BarNoDelete &&other) noexcept : ptr_(other.ptr_) {
+    other.ptr_ = nullptr;
+  }
+  BarNoDelete &operator=(BarNoDelete &&other) noexcept {
+    if (this != &other) {
+      // no delete of old ptr_ on purpose
+      ptr_ = other.ptr_;
+      other.ptr_ = nullptr;
+    }
+    return *this;
+  }
+
+private:
+  int *ptr_;
+};
+
+static BarNoDelete make_bar_nd(int v) { return BarNoDelete(new int(v)); }
+
+struct FooND {
+  BarNoDelete a;
+  BarNoDelete b;
+};
+
+namespace prvalue_aggregate_positive {
+
+void leak_aggregate_from_factory() {
+  FooND f = {make_bar_nd(1), make_bar_nd(2)};
+  // expected-warning@-1 {{Potential memory leak}}
+}
+
+void leak_direct_member() {
+  BarNoDelete b(new int(3));
+  // expected-warning@-1 {{Potential memory leak}}
+}
+
+} // namespace prvalue_aggregate_positive
+
+//===----------------------------------------------------------------------===//
+// Guard tests: neighboring behaviors that must remain intact
+// These ensure we didn't weaken unrelated diagnostics (mismatch/double-delete).
+//===----------------------------------------------------------------------===//
+
+namespace guards {
+
+void mismatch_array_delete() {
+  int *p = new int[4];
+  delete p; // expected-warning {{mismatched deallocation: 'delete' should be 'delete[]'}}
+}
+
+void double_delete() {
+  int *p = new int(1);
+  delete p;
+  delete p; // expected-warning {{Attempt to free released memory}}
+}
+
+} // namespace guards