[clang] Reland: [clang] Diagnose dangling issues for the "Container<GSLPointer>" case. #107213 (PR #108344)

Haojian Wu via cfe-commits cfe-commits at lists.llvm.org
Mon Sep 23 13:33:37 PDT 2024


https://github.com/hokein updated https://github.com/llvm/llvm-project/pull/108344

>From cda8759cb887b8a7d4a66e9b6fb4e543ab40ff5d Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Thu, 12 Sep 2024 09:27:03 +0200
Subject: [PATCH 1/6] Reapply "[clang] Diagnose dangling issues for the
 "Container<GSLPointer>" case. (#107213)"

This reverts commit 0683c4e839524c37fe4ddfa1bce1e31ba556041b.
---
 clang/docs/ReleaseNotes.rst                   |  2 +
 clang/include/clang/Basic/AttrDocs.td         | 14 ++++
 clang/lib/Sema/CheckExprLifetime.cpp          | 42 +++++++---
 .../Sema/warn-lifetime-analysis-nocfg.cpp     | 77 +++++++++++++++++++
 4 files changed, 126 insertions(+), 9 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8f7772baafcdb1..495fd4e7a31e90 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -333,6 +333,8 @@ Improvements to Clang's diagnostics
   local variables passed to function calls using the ``[[clang::musttail]]``
   attribute.
 
+- Clang now diagnoses cases where a dangling ``GSLOwner<GSLPointer>`` object is constructed, e.g. ``std::vector<string_view> v = {std::string()};`` (#GH100526).
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index f23a148e546fa3..53d88482698f00 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6696,6 +6696,20 @@ When the Owner's lifetime ends, it will consider the Pointer to be dangling.
     P.getInt(); // P is dangling
   }
 
+If a template class is annotated with ``[[gsl::Owner]]``, and the first
+instantiated template argument is a pointer type (raw pointer, or ``[[gsl::Pointer]]``),
+the analysis will consider the instantiated class as a container of the pointer.
+When constructing such an object from a GSL owner object, the analysis will
+assume that the container holds a pointer to the owner object. Consequently,
+when the owner object is destroyed, the pointer will be considered dangling.
+
+.. code-block:: c++
+
+   int f() {
+     std::vector<std::string_view> v = {std::string()}; // v holds a dangling pointer.
+     std::optional<std::string_view> o = std::string(); // o holds a dangling pointer.
+   }
+
 }];
 }
 
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index e9e39c11ffbaab..8b8b25ec94c699 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -271,6 +271,26 @@ static bool isInStlNamespace(const Decl *D) {
   return DC->isStdNamespace();
 }
 
+// Returns true if the given Record decl is a form of `GSLOwner<Pointer>`
+// type, e.g. std::vector<string_view>, std::optional<string_view>.
+static bool isContainerOfPointer(const RecordDecl *Container) {
+  if (const auto *CTSD =
+          dyn_cast_if_present<ClassTemplateSpecializationDecl>(Container)) {
+    if (!CTSD->hasAttr<OwnerAttr>()) // Container must be a GSL owner type.
+      return false;
+    const auto &TAs = CTSD->getTemplateArgs();
+    return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
+           (isRecordWithAttr<PointerAttr>(TAs[0].getAsType()) ||
+            TAs[0].getAsType()->isPointerType());
+  }
+  return false;
+}
+
+static bool isGSLOwner(QualType T) {
+  return isRecordWithAttr<OwnerAttr>(T) &&
+         !isContainerOfPointer(T->getAsRecordDecl());
+}
+
 static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
   if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee))
     if (isRecordWithAttr<PointerAttr>(Conv->getConversionType()) &&
@@ -280,7 +300,7 @@ static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
     return false;
   if (!isRecordWithAttr<PointerAttr>(
           Callee->getFunctionObjectParameterType()) &&
-      !isRecordWithAttr<OwnerAttr>(Callee->getFunctionObjectParameterType()))
+      !isGSLOwner(Callee->getFunctionObjectParameterType()))
     return false;
   if (Callee->getReturnType()->isPointerType() ||
       isRecordWithAttr<PointerAttr>(Callee->getReturnType())) {
@@ -418,7 +438,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
     // Once we initialized a value with a non gsl-owner reference, it can no
     // longer dangle.
     if (ReturnType->isReferenceType() &&
-        !isRecordWithAttr<OwnerAttr>(ReturnType->getPointeeType())) {
+        !isGSLOwner(ReturnType->getPointeeType())) {
       for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
         if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit ||
             PE.Kind == IndirectLocalPathEntry::LifetimeBoundCall)
@@ -473,12 +493,17 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
     if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
       VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
     else if (EnableGSLAnalysis && I == 0) {
+      // Perform GSL analysis for the first argument
       if (shouldTrackFirstArgument(Callee)) {
         VisitGSLPointerArg(Callee, Args[0]);
-      } else if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
-                 CCE &&
-                 CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) {
-        VisitGSLPointerArg(CCE->getConstructor(), Args[0]);
+      } else if (auto *Ctor = dyn_cast<CXXConstructExpr>(Call)) {
+        const auto *ClassD = Ctor->getConstructor()->getParent();
+        // Two cases:
+        //  a GSL pointer, e.g. std::string_view
+        //  a container of GSL pointer, e.g. std::vector<string_view>
+        if (ClassD->hasAttr<PointerAttr>() ||
+            (isContainerOfPointer(ClassD) && Callee->getNumParams() == 1))
+          VisitGSLPointerArg(Ctor->getConstructor(), Args[0]);
       }
     }
   }
@@ -990,13 +1015,12 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
         //   int &p = *localUniquePtr;
         //   someContainer.add(std::move(localUniquePtr));
         //   return p;
-        IsLocalGslOwner = isRecordWithAttr<OwnerAttr>(L->getType());
+        IsLocalGslOwner = isGSLOwner(L->getType());
         if (pathContainsInit(Path) || !IsLocalGslOwner)
           return false;
       } else {
         IsGslPtrValueFromGslTempOwner =
-            MTE && !MTE->getExtendingDecl() &&
-            isRecordWithAttr<OwnerAttr>(MTE->getType());
+            MTE && !MTE->getExtendingDecl() && isGSLOwner(MTE->getType());
         // Skipping a chain of initializing gsl::Pointer annotated objects.
         // We are looking only for the final source to find out if it was
         // a local or temporary owner or the address of a local variable/param.
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 69e5395a78a57e..3ea54d769233a7 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -158,17 +158,30 @@ auto begin(C &c) -> decltype(c.begin());
 template<typename T, int N>
 T *begin(T (&array)[N]);
 
+using size_t = decltype(sizeof(0));
+
+template<typename T>
+struct initializer_list {
+  const T* ptr; size_t sz;
+};
 template <typename T>
 struct vector {
   typedef __gnu_cxx::basic_iterator<T> iterator;
   iterator begin();
   iterator end();
   const T *data() const;
+  vector();
+  vector(initializer_list<T> __l);
+  
+  template<typename InputIterator>
+	vector(InputIterator first, InputIterator __last);
+
   T &at(int n);
 };
 
 template<typename T>
 struct basic_string_view {
+  basic_string_view();
   basic_string_view(const T *);
   const T *begin() const;
 };
@@ -203,11 +216,21 @@ template<typename T>
 struct optional {
   optional();
   optional(const T&);
+
+  template<typename U = T>
+	optional(U&& t);
+
+  template<typename U>
+	optional(optional<U>&& __t);
+
   T &operator*() &;
   T &&operator*() &&;
   T &value() &;
   T &&value() &&;
 };
+template<typename T>
+optional<__decay(T)> make_optional(T&&);
+
 
 template<typename T>
 struct stack {
@@ -587,3 +610,57 @@ std::string_view test2() {
   return k.value(); // expected-warning {{address of stack memory associated}}
 }
 } // namespace GH108272
+
+namespace GH100526 {
+void test() {
+  std::vector<std::string_view> v1({std::string()}); // expected-warning {{object backing the pointer will be destroyed at the end}}
+  std::vector<std::string_view> v2({
+    std::string(), // expected-warning {{object backing the pointer will be destroyed at the end}}
+    std::string_view()
+  });
+  std::vector<std::string_view> v3({
+    std::string_view(),
+    std::string()  // expected-warning {{object backing the pointer will be destroyed at the end}}
+  });
+
+  std::optional<std::string_view> o1 = std::string(); // expected-warning {{object backing the pointer}}
+
+  std::string s;
+  // This is a tricky use-after-free case, what it does:
+  //   1. make_optional creates a temporary "optional<string>"" object
+  //   2. the temporary object owns the underlying string which is copied from s.
+  //   3. the t3 object holds the view to the underlying string of the temporary object.
+  std::optional<std::string_view> o2 = std::make_optional(s); // expected-warning {{object backing the pointer}}
+  std::optional<std::string_view> o3 = std::optional<std::string>(s); // expected-warning {{object backing the pointer}}
+  std::optional<std::string_view> o4 = std::optional<std::string_view>(s); 
+
+  // FIXME: should work for assignment cases
+  v1 = {std::string()};
+  o1 = std::string();
+
+  // no warning on copying pointers.
+  std::vector<std::string_view> n1 = {std::string_view()};
+  std::optional<std::string_view> n2 = {std::string_view()};
+  std::optional<std::string_view> n3 = std::string_view();
+  std::optional<std::string_view> n4 = std::make_optional(std::string_view());
+  const char* b = "";
+  std::optional<std::string_view> n5 = std::make_optional(b);
+  std::optional<std::string_view> n6 = std::make_optional("test");
+}
+
+std::vector<std::string_view> test2(int i) {
+  std::vector<std::string_view> t;
+  if (i)
+    return t; // this is fine, no dangling
+  return std::vector<std::string_view>(t.begin(), t.end());
+}
+
+std::optional<std::string_view> test3(int i) {
+  std::string s;
+  std::string_view sv;
+  if (i)
+   return s; // expected-warning {{address of stack memory associated}}
+  return sv; // fine
+}
+
+} // namespace GH100526

>From 856f0e6f077d3516779b1f93c40d9b1255703269 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Thu, 12 Sep 2024 10:18:46 +0200
Subject: [PATCH 2/6] Fix the false positive for make_optional(nullptr);

We should also consider isNullPtrType as the pointer type.
---
 clang/lib/Sema/CheckExprLifetime.cpp             | 3 ++-
 clang/test/Sema/warn-lifetime-analysis-nocfg.cpp | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 8b8b25ec94c699..aca68f9af6a1cf 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -281,7 +281,8 @@ static bool isContainerOfPointer(const RecordDecl *Container) {
     const auto &TAs = CTSD->getTemplateArgs();
     return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
            (isRecordWithAttr<PointerAttr>(TAs[0].getAsType()) ||
-            TAs[0].getAsType()->isPointerType());
+            TAs[0].getAsType()->isPointerType() ||
+            TAs[0].getAsType()->isNullPtrType());
   }
   return false;
 }
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 3ea54d769233a7..26e953205e551a 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -663,4 +663,8 @@ std::optional<std::string_view> test3(int i) {
   return sv; // fine
 }
 
+std::optional<int*> test4(int a) {
+  return std::make_optional(nullptr); // fine
+}
+
 } // namespace GH100526

>From bfcfd1ea9ab1ba378f3dcebfa2252ee0d937ad34 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Thu, 12 Sep 2024 22:57:50 +0200
Subject: [PATCH 3/6] fix another false positive caused by the interaction
 between the gsl::Owner and lifetimebound attributes.

---
 clang/lib/Sema/CheckExprLifetime.cpp          | 59 +++++++++++++++----
 .../Sema/warn-lifetime-analysis-nocfg.cpp     |  8 +++
 2 files changed, 54 insertions(+), 13 deletions(-)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index aca68f9af6a1cf..e5dff7fc941ca2 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -271,6 +271,11 @@ static bool isInStlNamespace(const Decl *D) {
   return DC->isStdNamespace();
 }
 
+static bool isPointerLikeType(QualType Type) {
+  return isRecordWithAttr<PointerAttr>(Type) || Type->isPointerType() ||
+         Type->isNullPtrType();
+}
+
 // Returns true if the given Record decl is a form of `GSLOwner<Pointer>`
 // type, e.g. std::vector<string_view>, std::optional<string_view>.
 static bool isContainerOfPointer(const RecordDecl *Container) {
@@ -280,9 +285,19 @@ static bool isContainerOfPointer(const RecordDecl *Container) {
       return false;
     const auto &TAs = CTSD->getTemplateArgs();
     return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
-           (isRecordWithAttr<PointerAttr>(TAs[0].getAsType()) ||
-            TAs[0].getAsType()->isPointerType() ||
-            TAs[0].getAsType()->isNullPtrType());
+           isPointerLikeType(TAs[0].getAsType());
+  }
+  return false;
+}
+// Returns true if the given Record is `std::initializer_list<pointer>`.
+static bool isStdInitializerListOfPointer(const RecordDecl *RD) {
+  if (const auto *CTSD =
+          dyn_cast_if_present<ClassTemplateSpecializationDecl>(RD)) {
+    const auto &TAs = CTSD->getTemplateArgs();
+    return isInStlNamespace(RD) && RD->getIdentifier() &&
+           RD->getName() == "initializer_list" && TAs.size() > 0 &&
+           TAs[0].getKind() == TemplateArgument::Type &&
+           isPointerLikeType(TAs[0].getAsType());
   }
   return false;
 }
@@ -303,8 +318,7 @@ static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
           Callee->getFunctionObjectParameterType()) &&
       !isGSLOwner(Callee->getFunctionObjectParameterType()))
     return false;
-  if (Callee->getReturnType()->isPointerType() ||
-      isRecordWithAttr<PointerAttr>(Callee->getReturnType())) {
+  if (isPointerLikeType(Callee->getReturnType())) {
     if (!Callee->getIdentifier())
       return false;
     return llvm::StringSwitch<bool>(Callee->getName())
@@ -352,6 +366,30 @@ static bool shouldTrackFirstArgument(const FunctionDecl *FD) {
   return false;
 }
 
+// Returns true if we should perform the GSL analysis on the first argument for
+// the given constructor.
+static bool
+shouldTrackFirstArgumentForConstructor(const CXXConstructExpr *Ctor) {
+  const auto *ClassD = Ctor->getConstructor()->getParent();
+
+  auto FirstArgType = Ctor->getArg(0)->getType();
+  // Case 1, construct a GSL pointer, e.g. std::string_view
+  if (ClassD->hasAttr<PointerAttr>())
+    return true;
+
+  // case 2: construct a container of pointer (std::vector<std::string_view>)
+  // from an owner or a std::initilizer_list.
+  //
+  // std::initializer_list is a proxy object that provides access to the backing
+  // array. We perform analysis on it to determine if there are any dangling
+  // temporaries in the backing array.
+  if (Ctor->getConstructor()->getNumParams() != 1 ||
+      !isContainerOfPointer(ClassD))
+    return false;
+  return isGSLOwner(FirstArgType) ||
+         isStdInitializerListOfPointer(FirstArgType->getAsRecordDecl());
+}
+
 // Return true if this is an "normal" assignment operator.
 // We assuments that a normal assingment operator always returns *this, that is,
 // an lvalue reference that is the same type as the implicit object parameter
@@ -497,14 +535,9 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
       // Perform GSL analysis for the first argument
       if (shouldTrackFirstArgument(Callee)) {
         VisitGSLPointerArg(Callee, Args[0]);
-      } else if (auto *Ctor = dyn_cast<CXXConstructExpr>(Call)) {
-        const auto *ClassD = Ctor->getConstructor()->getParent();
-        // Two cases:
-        //  a GSL pointer, e.g. std::string_view
-        //  a container of GSL pointer, e.g. std::vector<string_view>
-        if (ClassD->hasAttr<PointerAttr>() ||
-            (isContainerOfPointer(ClassD) && Callee->getNumParams() == 1))
-          VisitGSLPointerArg(Ctor->getConstructor(), Args[0]);
+      } else if (auto *Ctor = dyn_cast<CXXConstructExpr>(Call);
+                 Ctor && shouldTrackFirstArgumentForConstructor(Ctor)) {
+        VisitGSLPointerArg(Ctor->getConstructor(), Args[0]);
       }
     }
   }
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 26e953205e551a..3bfbc741d0561b 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -667,4 +667,12 @@ std::optional<int*> test4(int a) {
   return std::make_optional(nullptr); // fine
 }
 
+template <typename T>
+struct [[gsl::Owner]] StatusOr {
+  const T &value() [[clang::lifetimebound]];
+};
+std::vector<int*> test5(StatusOr<std::vector<int*>> aa) {
+  return aa.value(); // fine
+}
+
 } // namespace GH100526

>From 6e110aeaaaee14e53e38ead827cec97d80422ee7 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Fri, 13 Sep 2024 21:58:18 +0200
Subject: [PATCH 4/6] Fix a newly-introcuded false negative.

And add more tests per the comment. The new false negative was caused by
replacing the instances of "!isRecordWithAttr<OwnerAttr>" in `TemporaryVisitor`
with "isGSLOwner".
---
 clang/lib/Sema/CheckExprLifetime.cpp          | 39 +++++----
 .../Sema/warn-lifetime-analysis-nocfg.cpp     | 83 +++++++++++++++++--
 2 files changed, 97 insertions(+), 25 deletions(-)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index e5dff7fc941ca2..cb8534c56d9644 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -302,11 +302,6 @@ static bool isStdInitializerListOfPointer(const RecordDecl *RD) {
   return false;
 }
 
-static bool isGSLOwner(QualType T) {
-  return isRecordWithAttr<OwnerAttr>(T) &&
-         !isContainerOfPointer(T->getAsRecordDecl());
-}
-
 static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
   if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee))
     if (isRecordWithAttr<PointerAttr>(Conv->getConversionType()) &&
@@ -316,7 +311,7 @@ static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
     return false;
   if (!isRecordWithAttr<PointerAttr>(
           Callee->getFunctionObjectParameterType()) &&
-      !isGSLOwner(Callee->getFunctionObjectParameterType()))
+      !isRecordWithAttr<OwnerAttr>(Callee->getFunctionObjectParameterType()))
     return false;
   if (isPointerLikeType(Callee->getReturnType())) {
     if (!Callee->getIdentifier())
@@ -372,22 +367,29 @@ static bool
 shouldTrackFirstArgumentForConstructor(const CXXConstructExpr *Ctor) {
   const auto *ClassD = Ctor->getConstructor()->getParent();
 
-  auto FirstArgType = Ctor->getArg(0)->getType();
   // Case 1, construct a GSL pointer, e.g. std::string_view
   if (ClassD->hasAttr<PointerAttr>())
     return true;
 
-  // case 2: construct a container of pointer (std::vector<std::string_view>)
-  // from an owner or a std::initilizer_list.
-  //
-  // std::initializer_list is a proxy object that provides access to the backing
-  // array. We perform analysis on it to determine if there are any dangling
-  // temporaries in the backing array.
+  auto FirstArgType = Ctor->getArg(0)->getType();
+  // Case 2, construct a container of pointer (std::vector<std::string_view>)
+  // from a std::initilizer_list or an GSL owner
   if (Ctor->getConstructor()->getNumParams() != 1 ||
       !isContainerOfPointer(ClassD))
     return false;
-  return isGSLOwner(FirstArgType) ||
-         isStdInitializerListOfPointer(FirstArgType->getAsRecordDecl());
+
+  // For the typical case: `std::vector<std::string_view> abc = {std::string()};`
+  // std::initializer_list is a proxy object that provides access to the backing
+  // array. We perform analysis on it to determine if there are any dangling
+  // temporaries in the backing array.
+  if (isStdInitializerListOfPointer(FirstArgType->getAsRecordDecl()))
+    return true;
+  // For the case: `std::optional<std::string_view> abc = std::string();`
+  // When constructing from a container of pointers, it's less likely to result
+  // in a dangling pointer. Therefore, we try to be conservative to not track
+  // the argument futher to avoid false positives.
+  return isRecordWithAttr<OwnerAttr>(FirstArgType) &&
+         !isContainerOfPointer(FirstArgType->getAsRecordDecl());
 }
 
 // Return true if this is an "normal" assignment operator.
@@ -477,7 +479,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
     // Once we initialized a value with a non gsl-owner reference, it can no
     // longer dangle.
     if (ReturnType->isReferenceType() &&
-        !isGSLOwner(ReturnType->getPointeeType())) {
+        !isRecordWithAttr<OwnerAttr>(ReturnType->getPointeeType())) {
       for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
         if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit ||
             PE.Kind == IndirectLocalPathEntry::LifetimeBoundCall)
@@ -1049,12 +1051,13 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
         //   int &p = *localUniquePtr;
         //   someContainer.add(std::move(localUniquePtr));
         //   return p;
-        IsLocalGslOwner = isGSLOwner(L->getType());
+        IsLocalGslOwner = isRecordWithAttr<OwnerAttr>(L->getType());
         if (pathContainsInit(Path) || !IsLocalGslOwner)
           return false;
       } else {
         IsGslPtrValueFromGslTempOwner =
-            MTE && !MTE->getExtendingDecl() && isGSLOwner(MTE->getType());
+            MTE && !MTE->getExtendingDecl() &&
+            isRecordWithAttr<OwnerAttr>(MTE->getType());
         // Skipping a chain of initializing gsl::Pointer annotated objects.
         // We are looking only for the final source to find out if it was
         // a local or temporary owner or the address of a local variable/param.
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 3bfbc741d0561b..b4bba479580c70 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -172,7 +172,7 @@ struct vector {
   const T *data() const;
   vector();
   vector(initializer_list<T> __l);
-  
+
   template<typename InputIterator>
 	vector(InputIterator first, InputIterator __last);
 
@@ -218,10 +218,10 @@ struct optional {
   optional(const T&);
 
   template<typename U = T>
-	optional(U&& t);
+  optional(U&& t);
 
   template<typename U>
-	optional(optional<U>&& __t);
+  optional(optional<U>&& __t);
 
   T &operator*() &;
   T &&operator*() &&;
@@ -632,7 +632,7 @@ void test() {
   //   3. the t3 object holds the view to the underlying string of the temporary object.
   std::optional<std::string_view> o2 = std::make_optional(s); // expected-warning {{object backing the pointer}}
   std::optional<std::string_view> o3 = std::optional<std::string>(s); // expected-warning {{object backing the pointer}}
-  std::optional<std::string_view> o4 = std::optional<std::string_view>(s); 
+  std::optional<std::string_view> o4 = std::optional<std::string_view>(s);
 
   // FIXME: should work for assignment cases
   v1 = {std::string()};
@@ -667,12 +667,81 @@ std::optional<int*> test4(int a) {
   return std::make_optional(nullptr); // fine
 }
 
+
 template <typename T>
 struct [[gsl::Owner]] StatusOr {
-  const T &value() [[clang::lifetimebound]];
+  const T &valueLB() const [[clang::lifetimebound]];
+  const T &valueNoLB() const;
+};
+
+template<typename T>
+struct [[gsl::Pointer]] Span {
+  Span(const std::vector<T> &V);
+
+  const int& getFieldLB() const [[clang::lifetimebound]];
+  const int& getFieldNoLB() const;
 };
-std::vector<int*> test5(StatusOr<std::vector<int*>> aa) {
-  return aa.value(); // fine
+
+
+/////// From Owner<Pointer> ///////
+
+// Pointer from Owner<Pointer>
+std::string_view test5() {
+  std::string_view a = StatusOr<std::string_view>().valueLB(); // expected-warning {{object backing the pointer will be dest}}
+return StatusOr<std::string_view>().valueLB(); // expected-warning {{returning address of local temporary}}
+
+  // No dangling diagnostics on non-lifetimebound methods.
+  std::string_view b = StatusOr<std::string_view>().valueNoLB();
+  return StatusOr<std::string_view>().valueNoLB();
+}
+
+// Pointer<Pointer> from Owner<Pointer>
+// Prevent regression GH108463
+Span<int*> test6(std::vector<int*> v) {
+  Span<int *> dangling = std::vector<int*>(); // expected-warning {{object backing the pointer}}
+  return v; // expected-warning {{address of stack memory}}
+}
+
+/////// From Owner<Owner<Pointer>> ///////
+
+// Pointer from Owner<Owner<Pointer>>
+int* test7(StatusOr<StatusOr<int*>> aa) {
+  // No dangling diagnostic on pointer.
+  return aa.valueLB().valueLB(); // OK.
+}
+
+// Owner<Pointer> from Owner<Owner<Pointer>>
+std::vector<int*> test8(StatusOr<std::vector<int*>> aa) {
+  return aa.valueLB(); // OK, no pointer being construct on this case.
+  return aa.valueNoLB();
+}
+
+// Pointer<Pointer> from Owner<Owner<Pointer>>
+Span<int*> test9(StatusOr<std::vector<int*>> aa) {
+  return aa.valueLB(); // expected-warning {{address of stack memory associated}}
+  return aa.valueNoLB(); // OK.
+}
+
+/////// From Owner<Owner> ///////
+
+// Pointer<Owner>> from Owner<Owner>
+Span<std::string> test10(StatusOr<std::vector<std::string>> aa) {
+  return aa.valueLB(); // expected-warning {{address of stack memory}}
+  return aa.valueNoLB(); // OK.
+}
+
+/////// From Owner<Pointer<Owner>> ///////
+
+// Pointer<Owner>> from Owner<Pointer<Owner>>
+Span<std::string> test11(StatusOr<Span<std::string>> aa) {
+  return aa.valueLB(); // expected-warning {{address of stack memory}}
+  return aa.valueNoLB(); // OK.
+}
+
+// Lifetimebound and gsl::Pointer.
+const int& test12(Span<int> a) {
+  return a.getFieldLB(); // expected-warning {{reference to stack memory associated}}
+  return a.getFieldNoLB(); // OK.
 }
 
 } // namespace GH100526

>From b2756decdcac797ee8dc77513cc65522c4b0497e Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Thu, 19 Sep 2024 12:03:55 +0200
Subject: [PATCH 5/6] Refine the heuristic on inferring the nested pointer
 onwnership.

Fix another new false positive
---
 clang/lib/Sema/CheckExprLifetime.cpp          | 106 +++++++++++++++---
 .../Sema/warn-lifetime-analysis-nocfg.cpp     |  25 +++++
 2 files changed, 117 insertions(+), 14 deletions(-)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index cb8534c56d9644..c71faa064b2e7c 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -289,6 +289,18 @@ static bool isContainerOfPointer(const RecordDecl *Container) {
   }
   return false;
 }
+static bool isContainerOfOwner(const RecordDecl *Container) {
+  if (const auto *CTSD =
+          dyn_cast_if_present<ClassTemplateSpecializationDecl>(Container)) {
+    if (!CTSD->hasAttr<OwnerAttr>()) // Container must be a GSL owner type.
+      return false;
+    const auto &TAs = CTSD->getTemplateArgs();
+    return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
+           isRecordWithAttr<OwnerAttr>(TAs[0].getAsType());
+  }
+  return false;
+}
+
 // Returns true if the given Record is `std::initializer_list<pointer>`.
 static bool isStdInitializerListOfPointer(const RecordDecl *RD) {
   if (const auto *CTSD =
@@ -361,35 +373,101 @@ static bool shouldTrackFirstArgument(const FunctionDecl *FD) {
   return false;
 }
 
+// Returns true if the given constructor is a copy-like constructor, such as
+// `Ctor(Owner<U>&&)` or `Ctor(const Owner<U>&)`.
+static bool isCopyLikeConstructor(const CXXConstructorDecl *Ctor) {
+  if (!Ctor || Ctor->param_size() != 1)
+    return false;
+  const auto *ParamRefType =
+      Ctor->getParamDecl(0)->getType()->getAs<ReferenceType>();
+  if (!ParamRefType)
+    return false;
+
+  // Check if the first parameter type "Owner<U>".
+  if (const auto *TST =
+          ParamRefType->getPointeeType()->getAs<TemplateSpecializationType>())
+    return TST->getTemplateName()
+        .getAsTemplateDecl()
+        ->getTemplatedDecl()
+        ->hasAttr<OwnerAttr>();
+  return false;
+}
+
 // Returns true if we should perform the GSL analysis on the first argument for
 // the given constructor.
 static bool
 shouldTrackFirstArgumentForConstructor(const CXXConstructExpr *Ctor) {
-  const auto *ClassD = Ctor->getConstructor()->getParent();
+  const auto *LHSRecordDecl = Ctor->getConstructor()->getParent();
 
   // Case 1, construct a GSL pointer, e.g. std::string_view
-  if (ClassD->hasAttr<PointerAttr>())
+  // Always inspect when LHS is a pointer.
+  if (LHSRecordDecl->hasAttr<PointerAttr>())
     return true;
 
-  auto FirstArgType = Ctor->getArg(0)->getType();
-  // Case 2, construct a container of pointer (std::vector<std::string_view>)
-  // from a std::initilizer_list or an GSL owner
   if (Ctor->getConstructor()->getNumParams() != 1 ||
-      !isContainerOfPointer(ClassD))
+      !isContainerOfPointer(LHSRecordDecl))
     return false;
 
-  // For the typical case: `std::vector<std::string_view> abc = {std::string()};`
+  // Now, the LHS is an Owner<Pointer> type, e.g., std::vector<string_view>.
+  //
+  // At a high level, we cannot precisely determine what the nested pointer
+  // owns. However, by analyzing the RHS owner type, we can use heuristics to
+  // infer ownership information. These heuristics are designed to be
+  // conservative, minimizing false positives while still providing meaningful
+  // diagnostics.
+  //
+  // While this inference isn't perfect, it helps catch common use-after-free
+  // patterns.
+  auto RHSArgType = Ctor->getArg(0)->getType();
+  const auto *RHSRD = RHSArgType->getAsRecordDecl();
+  // LHS is constructed from an intializer_list.
+  //
   // std::initializer_list is a proxy object that provides access to the backing
   // array. We perform analysis on it to determine if there are any dangling
   // temporaries in the backing array.
-  if (isStdInitializerListOfPointer(FirstArgType->getAsRecordDecl()))
+  // E.g. std::vector<string_view> abc = {string()};
+  if (isStdInitializerListOfPointer(RHSRD))
+    return true;
+
+  // RHS must be an owner.
+  if (!isRecordWithAttr<OwnerAttr>(RHSArgType))
+    return false;
+
+  // Bail out if the RHS is Owner<Pointer>.
+  //
+  // We cannot reliably determine what the LHS nested pointer owns -- it could
+  // be the entire RHS or the nested pointer in RHS. To avoid false positives,
+  // we skip this case, such as:
+  //   std::stack<std::string_view> s(std::deque<std::string_view>{});
+  //
+  // TODO: this also has a false negative, it doesn't catch the case like:
+  //   std::optional<span<int*>> os = std::vector<int*>{}
+  if (isContainerOfPointer(RHSRD))
+    return false;
+
+  // Assume that the nested Pointer is constructed from the nested Owner.
+  // E.g. std::optional<string_view> sv = std::optional<string>(s);
+  if (isContainerOfOwner(RHSRD))
     return true;
-  // For the case: `std::optional<std::string_view> abc = std::string();`
-  // When constructing from a container of pointers, it's less likely to result
-  // in a dangling pointer. Therefore, we try to be conservative to not track
-  // the argument futher to avoid false positives.
-  return isRecordWithAttr<OwnerAttr>(FirstArgType) &&
-         !isContainerOfPointer(FirstArgType->getAsRecordDecl());
+
+  // Now, the LHS is an Owner<Pointer> and the RHS is an Owner<X>,  where X is
+  // neither an `Owner` nor a `Pointer`.
+  //
+  // Use the constructor's signature as a hint. If it is a copy-like constructor
+  // `Owner1<Pointer>(Owner2<X>&&)`, we assume that the nested pointer is
+  // constructed from X. In such cases, we do not diagnose, as `X` is not an
+  // owner, e.g.
+  //   std::optional<string_view> sv = std::optional<Foo>();
+  if (const auto *PrimaryCtorTemplate =
+          Ctor->getConstructor()->getPrimaryTemplate();
+      PrimaryCtorTemplate &&
+      isCopyLikeConstructor(dyn_cast_if_present<CXXConstructorDecl>(
+          PrimaryCtorTemplate->getTemplatedDecl()))) {
+    return false;
+  }
+  // Assume that the nested pointer is constructed from the whole RHS.
+  // E.g. optional<string_view> s = std::string();
+  return true;
 }
 
 // Return true if this is an "normal" assignment operator.
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index b4bba479580c70..fc143bfc9491b7 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -655,12 +655,37 @@ std::vector<std::string_view> test2(int i) {
   return std::vector<std::string_view>(t.begin(), t.end());
 }
 
+class Foo {
+  public:
+   operator std::string_view() const { return ""; }
+};
+class [[gsl::Owner]] FooOwner {
+  public:
+   operator std::string_view() const { return ""; }
+};
+std::optional<Foo> GetFoo();
+std::optional<FooOwner> GetFooOwner();
+
+template <typename T>
+struct [[gsl::Owner]] Container1 {
+   Container1();
+};
+template <typename T>
+struct [[gsl::Owner]] Container2 {
+  template<typename U>
+  Container2(const Container1<U>& C2);
+};
+
 std::optional<std::string_view> test3(int i) {
   std::string s;
   std::string_view sv;
   if (i)
    return s; // expected-warning {{address of stack memory associated}}
   return sv; // fine
+  Container2<std::string_view> c1 = Container1<Foo>(); // no diagnostic as Foo is not an Owner.
+  Container2<std::string_view> c2 = Container1<FooOwner>(); // expected-warning {{object backing the pointer will be destroyed}}
+  return GetFoo(); // fine, we don't know Foo is owner or not, be conservative.
+  return GetFooOwner(); // expected-warning {{returning address of local temporary object}}
 }
 
 std::optional<int*> test4(int a) {

>From 55f9462a21fb36c59d28988022045521d587025b Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Mon, 23 Sep 2024 22:29:27 +0200
Subject: [PATCH 6/6] Address review comments.

---
 clang/lib/Sema/CheckExprLifetime.cpp          | 20 +++++++++----------
 .../Sema/warn-lifetime-analysis-nocfg.cpp     |  7 +++++++
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index c71faa064b2e7c..009b8d000e6b0e 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -290,15 +290,15 @@ static bool isContainerOfPointer(const RecordDecl *Container) {
   return false;
 }
 static bool isContainerOfOwner(const RecordDecl *Container) {
-  if (const auto *CTSD =
-          dyn_cast_if_present<ClassTemplateSpecializationDecl>(Container)) {
-    if (!CTSD->hasAttr<OwnerAttr>()) // Container must be a GSL owner type.
-      return false;
-    const auto &TAs = CTSD->getTemplateArgs();
-    return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
-           isRecordWithAttr<OwnerAttr>(TAs[0].getAsType());
-  }
-  return false;
+  const auto *CTSD =
+      dyn_cast_if_present<ClassTemplateSpecializationDecl>(Container);
+  if (!CTSD)
+    return false;
+  if (!CTSD->hasAttr<OwnerAttr>()) // Container must be a GSL owner type.
+    return false;
+  const auto &TAs = CTSD->getTemplateArgs();
+  return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
+         isRecordWithAttr<OwnerAttr>(TAs[0].getAsType());
 }
 
 // Returns true if the given Record is `std::initializer_list<pointer>`.
@@ -383,7 +383,7 @@ static bool isCopyLikeConstructor(const CXXConstructorDecl *Ctor) {
   if (!ParamRefType)
     return false;
 
-  // Check if the first parameter type "Owner<U>".
+  // Check if the first parameter type is "Owner<U>".
   if (const auto *TST =
           ParamRefType->getPointeeType()->getAs<TemplateSpecializationType>())
     return TST->getTemplateName()
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index fc143bfc9491b7..c6272a775a28fa 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -769,4 +769,11 @@ const int& test12(Span<int> a) {
   return a.getFieldNoLB(); // OK.
 }
 
+void test13() {
+  // FIXME: RHS is Owner<Pointer>, we skip this case to avoid false positives.
+  std::optional<Span<int*>> abc = std::vector<int*>{};
+
+  std::optional<Span<int>> t = std::vector<int> {}; // expected-warning {{object backing the pointer will be destroyed}}
+}
+
 } // namespace GH100526



More information about the cfe-commits mailing list