[clang] [clang] Extend lifetime analysis to support assignments for pointer-like objects. (PR #99032)

Haojian Wu via cfe-commits cfe-commits at lists.llvm.org
Tue Jul 16 05:59:48 PDT 2024


https://github.com/hokein created https://github.com/llvm/llvm-project/pull/99032

This is a follow-up patch to #96475 to detect dangling assignments for C++ pointer-like objects (classes annotated with the `[[gsl::Pointer]]`).  Fixes #63310.

Similar to the behavior for built-in pointer types, if a temporary owner (`[[gsl::Owner]]`) object is assigned to a pointer-like class object, and this temporary object is destroyed at the end of the full assignment expression, the assignee pointer is considered dangling. In such cases, clang will emit a warning:

```
/tmp/t.cpp:7:20: warning: object backing the pointer my_string_view will be destroyed at the end of the full-expression [-Wdangling-assignment-gsl]
    7 |   my_string_view = CreateString();
      |                    ^~~~~~~~~~~~~~
1 warning generated.
```

This new warning is `-Wdangling-assignment-gsl`. It is initially disabled, but I intend to enable it by default in clang 20.

I have initially tested this patch on our internal codebase, and it has identified many use-after-free bugs, primarily related to `string_view`.




>From 9ac7a9543878fc93ea036cfa533c25bbbceeddf2 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Tue, 16 Jul 2024 12:29:35 +0200
Subject: [PATCH] [clang] Extend lifetime analysis to detect dangling
 assignments between pointer-like objects.

---
 clang/docs/ReleaseNotes.rst                   |  3 ++
 clang/include/clang/Basic/DiagnosticGroups.td |  2 +
 .../clang/Basic/DiagnosticSemaKinds.td        |  3 ++
 clang/lib/Sema/CheckExprLifetime.cpp          | 46 +++++++++++++------
 clang/lib/Sema/SemaOverload.cpp               |  9 ++--
 .../warn-lifetime-analysis-nocfg-disabled.cpp |  4 ++
 .../Sema/warn-lifetime-analysis-nocfg.cpp     | 19 ++++++--
 clang/test/SemaCXX/warn-dangling-local.cpp    |  4 +-
 8 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 969856a8f978c..26dcd54d65547 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -713,6 +713,9 @@ Improvements to Clang's diagnostics
 
 - Clang now diagnoses integer constant expressions that are folded to a constant value as an extension in more circumstances. Fixes #GH59863
 
+- Clang now diagnoses dangling assignments for pointer-like objects (annotated with `[[gsl::Pointer]]`) under `-Wdangling-assignment-gsl` (off by default)
+  Fixes #GH63310.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 2241f8481484e..e0d97c6ce99db 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -449,6 +449,7 @@ def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;
 def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
 def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;
 def DanglingAssignment: DiagGroup<"dangling-assignment">;
+def DanglingAssignmentGsl : DiagGroup<"dangling-assignment-gsl">;
 def DanglingElse: DiagGroup<"dangling-else">;
 def DanglingField : DiagGroup<"dangling-field">;
 def DanglingInitializerList : DiagGroup<"dangling-initializer-list">;
@@ -457,6 +458,7 @@ def ReturnStackAddress : DiagGroup<"return-stack-address">;
 // Name of this warning in GCC
 def : DiagGroup<"return-local-addr", [ReturnStackAddress]>;
 def Dangling : DiagGroup<"dangling", [DanglingAssignment,
+                                      DanglingAssignmentGsl,
                                       DanglingField,
                                       DanglingInitializerList,
                                       DanglingGsl,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 52ff4b026a60e..94e6d5c1322ce 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10121,6 +10121,9 @@ def warn_dangling_lifetime_pointer : Warning<
   "object backing the pointer "
   "will be destroyed at the end of the full-expression">,
   InGroup<DanglingGsl>;
+def warn_dangling_lifetime_pointer_assignment : Warning<"object backing the "
+  "pointer %0 will be destroyed at the end of the full-expression">,
+  InGroup<DanglingAssignmentGsl>, DefaultIgnore;
 def warn_new_dangling_initializer_list : Warning<
   "array backing "
   "%select{initializer list subobject of the allocated object|"
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 995e4cbadacfe..ff32ef8c4a23c 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -191,7 +191,8 @@ struct IndirectLocalPathEntry {
     TemporaryCopy,
     LambdaCaptureInit,
     GslReferenceInit,
-    GslPointerInit
+    GslPointerInit,
+    GslPointerAssignment,
   } Kind;
   Expr *E;
   union {
@@ -337,7 +338,8 @@ static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
       for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
         if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit)
           continue;
-        if (PE.Kind == IndirectLocalPathEntry::GslPointerInit)
+        if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
+            PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
           return;
         break;
       }
@@ -937,6 +939,7 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
     case IndirectLocalPathEntry::TemporaryCopy:
     case IndirectLocalPathEntry::GslReferenceInit:
     case IndirectLocalPathEntry::GslPointerInit:
+    case IndirectLocalPathEntry::GslPointerAssignment:
       // These exist primarily to mark the path as not permitting or
       // supporting lifetime extension.
       break;
@@ -966,7 +969,8 @@ static bool pathOnlyInitializesGslPointer(IndirectLocalPath &Path) {
     if (It.Kind == IndirectLocalPathEntry::LifetimeBoundCall)
       continue;
     return It.Kind == IndirectLocalPathEntry::GslPointerInit ||
-           It.Kind == IndirectLocalPathEntry::GslReferenceInit;
+           It.Kind == IndirectLocalPathEntry::GslReferenceInit ||
+           It.Kind == IndirectLocalPathEntry::GslPointerAssignment;
   }
   return false;
 }
@@ -975,7 +979,8 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
                                   const InitializedEntity *InitEntity,
                                   const InitializedEntity *ExtendingEntity,
                                   LifetimeKind LK,
-                                  const AssignedEntity *AEntity, Expr *Init) {
+                                  const AssignedEntity *AEntity, Expr *Init,
+                                  bool EnableLifetimeWarnings) {
   assert((AEntity && LK == LK_Assignment) ||
          (InitEntity && LK != LK_Assignment));
   // If this entity doesn't have an interesting lifetime, don't bother looking
@@ -1073,14 +1078,16 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
     }
 
     case LK_Assignment: {
-      if (!MTE)
+      if (!MTE || pathContainsInit(Path))
         return false;
       assert(shouldLifetimeExtendThroughPath(Path) ==
                  PathLifetimeKind::NoExtend &&
              "No lifetime extension for assignments");
-      if (!pathContainsInit(Path))
-        SemaRef.Diag(DiagLoc, diag::warn_dangling_pointer_assignment)
-            << AEntity->LHS << DiagRange;
+      SemaRef.Diag(DiagLoc,
+                   IsGslPtrInitWithGslTempOwner
+                       ? diag::warn_dangling_lifetime_pointer_assignment
+                       : diag::warn_dangling_pointer_assignment)
+          << AEntity->LHS << DiagRange;
       return false;
     }
     case LK_MemInitializer: {
@@ -1226,6 +1233,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
       case IndirectLocalPathEntry::TemporaryCopy:
       case IndirectLocalPathEntry::GslPointerInit:
       case IndirectLocalPathEntry::GslReferenceInit:
+      case IndirectLocalPathEntry::GslPointerAssignment:
         // FIXME: Consider adding a note for these.
         break;
 
@@ -1265,9 +1273,11 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
     return false;
   };
 
-  bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored(
-      diag::warn_dangling_lifetime_pointer, SourceLocation());
   llvm::SmallVector<IndirectLocalPathEntry, 8> Path;
+  if (EnableLifetimeWarnings && LK == LK_Assignment &&
+      isRecordWithAttr<PointerAttr>(AEntity->LHS->getType()))
+    Path.push_back({IndirectLocalPathEntry::GslPointerAssignment, Init});
+
   if (Init->isGLValue())
     visitLocalsRetainedByReferenceBinding(Path, Init, RK_ReferenceBinding,
                                           TemporaryVisitor,
@@ -1284,16 +1294,26 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
   auto LTResult = getEntityLifetime(&Entity);
   LifetimeKind LK = LTResult.getInt();
   const InitializedEntity *ExtendingEntity = LTResult.getPointer();
-  checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init);
+  bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored(
+      diag::warn_dangling_lifetime_pointer, SourceLocation());
+  checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init,
+                        EnableLifetimeWarnings);
 }
 
 void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity,
                        Expr *Init) {
-  if (!Entity.LHS->getType()->isPointerType()) // builtin pointer type
+  bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored(
+      diag::warn_dangling_lifetime_pointer, SourceLocation());
+  bool RunAnalysis = Entity.LHS->getType()->isPointerType() ||
+                     (EnableLifetimeWarnings &&
+                      isRecordWithAttr<PointerAttr>(Entity.LHS->getType()));
+
+  if (!RunAnalysis)
     return;
+
   checkExprLifetimeImpl(SemaRef, /*InitEntity=*/nullptr,
                         /*ExtendingEntity=*/nullptr, LK_Assignment, &Entity,
-                        Init);
+                        Init, EnableLifetimeWarnings);
 }
 
 } // namespace clang::sema
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 074062ebbb594..1db06ac05ddea 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "CheckExprLifetime.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/CXXInheritance.h"
@@ -14714,10 +14715,12 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
                                 FnDecl))
           return ExprError();
 
-        // Check for a self move.
-        if (Op == OO_Equal)
+        if (Op == OO_Equal) {
+          // Check for a self move.
           DiagnoseSelfMove(Args[0], Args[1], OpLoc);
-
+          // lifetime check.
+          checkExprLifetime(*this, AssignedEntity{Args[0]}, Args[1]);
+        }
         if (ImplicitThis) {
           QualType ThisType = Context.getPointerType(ImplicitThis->getType());
           QualType ThisTypeFromDecl = Context.getPointerType(
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp
index 60b8f3ddedcd1..d1266027bdd34 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp
@@ -21,3 +21,7 @@ MyIntPointer g() {
   MyIntOwner o;
   return o; // No warning, it is disabled.
 }
+
+void h(MyIntPointer p) {
+  p = MyIntOwner(); //  No warning, it is disabled.
+}
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index b3ca173c1fdbc..09dfb2b5d96a8 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -120,11 +120,13 @@ MyLongPointerFromConversion global2;
 
 void initLocalGslPtrWithTempOwner() {
   MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
-  p = MyIntOwner{}; // TODO ?
-  global = MyIntOwner{}; // TODO ?
+  MyIntPointer pp = p = MyIntOwner{}; // expected-warning {{object backing the pointer p will be}}
+  p = MyIntOwner{}; // expected-warning {{object backing the pointer p }}
+  pp = p; // no warning
+  global = MyIntOwner{}; // expected-warning {{object backing the pointer global }}
   MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
-  p2 = MyLongOwnerWithConversion{}; // TODO ?
-  global2 = MyLongOwnerWithConversion{}; // TODO ?
+  p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer p2 }}
+  global2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer global2 }}
 }
 
 namespace __gnu_cxx {
@@ -170,6 +172,7 @@ struct basic_string_view {
   basic_string_view(const T *);
   const T *begin() const;
 };
+using string_view = basic_string_view<char>;
 
 template<class _Mystr> struct iter {
     iter& operator-=(int);
@@ -188,7 +191,7 @@ struct basic_string {
   operator basic_string_view<T> () const;
   using const_iterator = iter<T>;
 };
-
+using string = basic_string<char>;
 
 template<typename T>
 struct unique_ptr {
@@ -346,6 +349,12 @@ void handleTernaryOperator(bool cond) {
     std::basic_string_view<char> v = cond ? def : ""; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
 }
 
+std::string operator+(std::string_view s1, std::string_view s2);
+void danglingStringviewAssignment(std::string_view a1, std::string_view a2) {
+  a1 = std::string(); // expected-warning {{object backing}}
+  a2 = a1 + a1; // expected-warning {{object backing}}
+}
+
 std::reference_wrapper<int> danglingPtrFromNonOwnerLocal() {
   int i = 5;
   return i; // TODO
diff --git a/clang/test/SemaCXX/warn-dangling-local.cpp b/clang/test/SemaCXX/warn-dangling-local.cpp
index 2808a4c01f88d..5ad5013b6f025 100644
--- a/clang/test/SemaCXX/warn-dangling-local.cpp
+++ b/clang/test/SemaCXX/warn-dangling-local.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -std=c++11 %s
+// RUN: %clang_cc1 -verify -std=c++11 -Wdangling-assignment-gsl %s
 
 using T = int[];
 
@@ -34,6 +34,6 @@ struct basic_string {
 };
 }  // namespace std
 void test(const char* a) {
-  // verify we're emitting the `-Wdangling-assignment` warning.
+  // verify we're emitting the `-Wdangling-assignment-gsl` warning.
   a = std::basic_string().c_str(); // expected-warning {{object backing the pointer a will be destroyed at the end of the full-expression}}
 }



More information about the cfe-commits mailing list