[clang] Diagnose dangling issues for the "Container<GSLPointer>" case.

[hokein]

This pull request enhances the GSL lifetime analysis to detect situations where a dangling Container<GSLPointer> object is constructed:

std::vector<std::string_view> bad = {std::string()}; // dangling

The assignment case is not yet supported, but they will be addressed in a follow-up.

Fixes #100526 (excluding the push_back case).

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

This pull request enhances the GSL lifetime analysis to detect situations where a dangling Container<GSLPointer> object is constructed:

std::vector<std::string_view> bad = {std::string()}; // dangling

The assignment case is not yet supported, but they will be addressed in a follow-up.

Fixes #100526 (excluding the push_back case).

---

Full diff: https://github.com/llvm/llvm-project/pull/107213.diff

2 Files Affected:

• (modified) clang/lib/Sema/CheckExprLifetime.cpp (+23-5)
• (modified) clang/test/Sema/warn-lifetime-analysis-nocfg.cpp (+44)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index f7540a6e3a8979..a58ea39c6bd5e5 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -363,10 +363,14 @@ static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
     if (ATL.getAttrAs<LifetimeBoundAttr>())
       return true;
   }
-
   return isNormalAsisgnmentOperator(FD);
 }
 
+bool isFirstTemplateArgumentGSLPointer(const TemplateArgumentList &TAs) {
+  return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type &&
+         isRecordWithAttr<PointerAttr>(TAs[0].getAsType());
+}
+
 // Visit lifetimebound or gsl-pointer arguments.
 static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
                                        LocalVisitor Visit,
@@ -470,10 +474,24 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
         VisitGSLPointerArg(Callee, Args[0],
                            !Callee->getReturnType()->isReferenceType());
       } else {
-        if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
-            CCE && CCE->getConstructor()->getParent()->hasAttr<PointerAttr>())
-          VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0],
-                             true);
+        if (auto *Ctor = dyn_cast<CXXConstructExpr>(Call)) {
+          const auto *ClassD = Ctor->getConstructor()->getParent();
+          // Constructing the Container<GSLPointer> case (e.g.
+          // std::optional<string_view>) case.
+          if (const auto *CTSD =
+                  dyn_cast<ClassTemplateSpecializationDecl>(ClassD)) {
+            if (isFirstTemplateArgumentGSLPointer(CTSD->getTemplateArgs()) &&
+                CTSD->hasAttr<OwnerAttr>()) {
+              VisitGSLPointerArg(Ctor->getConstructor()->getParamDecl(0),
+                                 Args[0], true);
+              return;
+            }
+          }
+          // Constructing the GSLPointer (e.g. std::string_view) case.
+          if (ClassD->hasAttr<PointerAttr>())
+            VisitGSLPointerArg(Ctor->getConstructor()->getParamDecl(0), Args[0],
+                               true);
+        }
       }
     }
   }
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index cd1904db327105..dd0ed138c99676 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -158,17 +158,26 @@ 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);
   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 +212,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<T> make_optional(T&&);
+
 
 template<typename T>
 struct stack {
@@ -499,3 +518,28 @@ std::string_view test2(int i, std::optional<std::string_view> a) {
   return std::move(a.value());
 }
 }
+
+namespace GH100526 {
+void test() {
+  std::vector<std::string_view> t1 = {std::string()}; // expected-warning {{object backing the pointer will be destroyed at the end}}
+  std::optional<std::string_view> t2 = 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> t3 = std::make_optional(s); // expected-warning {{object backing the pointer}}
+
+  // FIXME: should work for assignment cases
+  t1 = {std::string()};
+  t2 = 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::make_optional(std::string_view());
+
+}
+
+} // namespace GH100526

[Xazax-hun]

LGTM! Thanks!

[usx95]

Thanks. This looks quite useful.

Since this is not limited to a hardcoded set of types/stl-containers, this is a substantial change in the behaviour of general Owner<Pointer> types and warrants extra documentation both in GSL analysis docs and ReleaseNotes.

[usx95]

This GSL analysis is done only on the first arg.
Can we separate this out and move outside the loop ?

// Perform GSL analysis for the first argument
bool PerformGSLAnalysis = EnableLifetimeWarnings && !CheckCoroCall && !Callee->getParamDecl(0)->hasAttr<LifetimeBoundAttr>()
if (PerformGSLAnalysis) {
...

[hokein]

The logic here is tightly coupled with the lifetimebound attribute. If the first argument is already visited due to the lifetimebound attribute, we avoid performing the GSL analysis on it again to prevent duplication. Therefore, both checks need to remain within the same loop.

[usx95]

Isn't bool PerformGSLAnalysisForFirstArg = EnableLifetimeWarnings && !CheckCoroCall && !Callee->getParamDecl(0)->hasAttr<LifetimeBoundAttr>() the sufficient condition to check outside the loop?

[hokein]

Since this is not limited to a hardcoded set of types/stl-containers, this is a substantial change in the behaviour of general Owner types and warrants extra documentation both in GSL analysis docs and ReleaseNotes.

Done.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[hokein]

After further testing, this change introduces a new false positive -- we emit a return-stack-address warning for the following case, this is not correct.

std::vector<std::string_view> kk() {
  std::vector<std::string_view> s;
  return s;
}

Looking now.

[usx95]

Thanks. LG. Some comments on adding more tests.

[hokein]

I have fixed the false positives, and added testcases for them. (I have tested this patch on our internal codebase, no new false positives being found).

[usx95]

LGTM. Thanks.

[aeubanks]

this looks like it has false positives: https://crbug.com/366074092

can we revert in the meantime?

[hokein]

Thanks, it looks like this introduces a new false positive:

#include <optional>
std::optional<int*> func(int a) {
  if (a)
   return std::make_optional(nullptr); // emit a dangling.
}

I'm going to revert it.