[clang] a918fa1 - [clang] Emit -Wdangling diagnoses for cases where a gsl-pointer is construct from a gsl-owner object in a container. (#104556)

[via cfe-commits]

Author: Haojian Wu
Date: 2024-09-06T12:37:21+02:00
New Revision: a918fa117acfbb20d29039cb8bddab159a8173dc

URL: https://github.com/llvm/llvm-project/commit/a918fa117acfbb20d29039cb8bddab159a8173dc
DIFF: https://github.com/llvm/llvm-project/commit/a918fa117acfbb20d29039cb8bddab159a8173dc.diff

LOG: [clang] Emit -Wdangling diagnoses for cases where a gsl-pointer is construct from a gsl-owner object in a container. (#104556)

The warning is not emitted for the case `string_view c =
std::vector<std::string>({""}).at(0);` because we bail out during the
visit of the LHS at [this
point](https://github.com/llvm/llvm-project/blob/5d2c324fea2d7cf86ec50e4bb6b680acf89b2ed5/clang/lib/Sema/CheckExprLifetime.cpp#L341-L343)
in the code.

This bailout was introduced in [this
commit](https://github.com/llvm/llvm-project/commit/bcd0798c47ca865f95226859893016a17402441e)
to address a false positive with
`vector<vector::iterator>({""}).at(0);`. This PR refines that fix by
ensuring that, for initialization involving a gsl-pointer, we only
consider constructor calls that take the gsl-owner object.

Fixes #100384.

Added: 
    

Modified: 
    clang/docs/ReleaseNotes.rst
    clang/lib/Sema/CheckExprLifetime.cpp
    clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Removed: 
    


################################################################################
diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a2e91fd648cce2..684484ccd298fb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -288,6 +288,8 @@ Improvements to Clang's diagnostics
 - The lifetimebound and GSL analysis in clang are coherent, allowing clang to
   detect more use-after-free bugs. (#GH100549).
 
+- Clang now diagnoses dangling cases where a gsl-pointer is constructed from a gsl-owner object inside a container (#GH100384).
+
 - Clang now warns for u8 character literals used in C23 with ``-Wpre-c23-compat`` instead of ``-Wpre-c++17-compat``.
 
 Improvements to Clang's time-trace

diff  --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 8f4d5d50669f14..f1507ebb9a5068 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -403,13 +403,17 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
       visitLocalsRetainedByInitializer(Path, Arg, Visit, true);
     Path.pop_back();
   };
-  auto VisitGSLPointerArg = [&](const Decl *D, Expr *Arg, bool Value) {
+  auto VisitGSLPointerArg = [&](const FunctionDecl *Callee, Expr *Arg) {
     // We are not interested in the temporary base objects of gsl Pointers:
     //   Temp().ptr; // Here ptr might not dangle.
     if (isa<MemberExpr>(Arg->IgnoreImpCasts()))
       return;
-    // Once we initialized a value with a reference, it can no longer dangle.
-    if (!Value) {
+    auto ReturnType = Callee->getReturnType();
+
+    // Once we initialized a value with a non gsl-owner reference, it can no
+    // longer dangle.
+    if (ReturnType->isReferenceType() &&
+        !isRecordWithAttr<OwnerAttr>(ReturnType->getPointeeType())) {
       for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
         if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit ||
             PE.Kind == IndirectLocalPathEntry::LifetimeBoundCall)
@@ -420,9 +424,10 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
         break;
       }
     }
-    Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit
-                          : IndirectLocalPathEntry::GslReferenceInit,
-                    Arg, D});
+    Path.push_back({ReturnType->isReferenceType()
+                        ? IndirectLocalPathEntry::GslReferenceInit
+                        : IndirectLocalPathEntry::GslPointerInit,
+                    Arg, Callee});
     if (Arg->isGLValue())
       visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
                                             Visit);
@@ -453,8 +458,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
     else if (EnableGSLAnalysis) {
       if (auto *CME = dyn_cast<CXXMethodDecl>(Callee);
           CME && shouldTrackImplicitObjectArg(CME))
-        VisitGSLPointerArg(Callee, ObjectArg,
-                           !Callee->getReturnType()->isReferenceType());
+        VisitGSLPointerArg(Callee, ObjectArg);
     }
   }
 
@@ -465,13 +469,11 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
       VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
     else if (EnableGSLAnalysis && I == 0) {
       if (shouldTrackFirstArgument(Callee)) {
-        VisitGSLPointerArg(Callee, Args[0],
-                           !Callee->getReturnType()->isReferenceType());
+        VisitGSLPointerArg(Callee, Args[0]);
       } else if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
                  CCE &&
                  CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) {
-        VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0],
-                           true);
+        VisitGSLPointerArg(CCE->getConstructor(), Args[0]);
       }
     }
   }

diff  --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 67d1ceaa02d039..59357d0730a7d9 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -275,6 +275,34 @@ int &danglingRawPtrFromLocal3() {
   return *o; // expected-warning {{reference to stack memory associated with local variable 'o' returned}}
 }
 
+// GH100384
+std::string_view containerWithAnnotatedElements() {
+  std::string_view c1 = std::vector<std::string>().at(0); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  c1 = std::vector<std::string>().at(0); // expected-warning {{object backing the pointer}}
+
+  // no warning on constructing from gsl-pointer
+  std::string_view c2 = std::vector<std::string_view>().at(0);
+
+  std::vector<std::string> local;
+  return local.at(0); // expected-warning {{address of stack memory associated with local variable}}
+}
+
+std::string_view localUniquePtr(int i) {
+  std::unique_ptr<std::string> c1;
+  if (i)
+    return *c1; // expected-warning {{address of stack memory associated with local variable}}
+  std::unique_ptr<std::string_view> c2;
+  return *c2; // expect no-warning.
+}
+
+std::string_view localOptional(int i) {
+  std::optional<std::string> o;
+  if (i)
+    return o.value(); // expected-warning {{address of stack memory associated with local variable}}
+  std::optional<std::string_view> abc;
+  return abc.value(); // expect no warning
+}
+
 const char *danglingRawPtrFromTemp() {
   return std::basic_string<char>().c_str(); // expected-warning {{returning address of local temporary object}}
 }