[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