[clang] [clang] Diagnose dangling issues for the "Container<GSLPointer>" case. (PR #107213)
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 4 13:28:16 PDT 2024
https://github.com/hokein updated https://github.com/llvm/llvm-project/pull/107213
>From 0d9a5971121bf66608625de3514db346876d9091 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Wed, 28 Aug 2024 09:59:41 +0200
Subject: [PATCH 1/2] [clang] Diagnose dangling issues for
"Container<GSLPointer>" case.
We teach the GSL lifetime analysis to handle this code path, particuly
when constructing the container<GSLPointer> object from a GSLOwner.
---
clang/lib/Sema/CheckExprLifetime.cpp | 29 +++++++++---
.../Sema/warn-lifetime-analysis-nocfg.cpp | 44 +++++++++++++++++++
2 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 1482711cc2839e..70d1de27fa086e 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -361,10 +361,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) {
@@ -465,14 +469,27 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
if (shouldTrackFirstArgument(Callee)) {
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);
+ } else 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);
}
}
}
+ }
}
/// Visit the locals that would be reachable through a reference bound to the
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 67d1ceaa02d039..91756e80ef9e8b 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 {
@@ -525,3 +544,28 @@ void test() {
std::string_view svjkk1 = ReturnStringView(StrCat("bar", "x")); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
}
} // namespace GH100549
+
+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
>From fe0ca71b1d5cc3ff696e374b5b5bd03f748944aa Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Wed, 4 Sep 2024 21:40:55 +0200
Subject: [PATCH 2/2] Add comments and add release note.
---
clang/docs/ReleaseNotes.rst | 2 ++
clang/include/clang/Basic/AttrDocs.td | 14 ++++++++
clang/lib/Sema/CheckExprLifetime.cpp | 32 +++++++++----------
.../Sema/warn-lifetime-analysis-nocfg.cpp | 13 +++++---
4 files changed, 40 insertions(+), 21 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1520f7a2916aae..cdde3fa08242a7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -278,6 +278,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 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 ef077db298831f..a43b330b4f7e6f 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6690,6 +6690,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 [[gsl::Pointer]] type, 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 70d1de27fa086e..89a96a68ce8d4b 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -361,12 +361,21 @@ 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());
+// Returns true if the given Record decl is a form of `GSLOwner<GSLPointer>`
+// type, e.g. std::vector<string_view>, std::optional<string_view>.
+static bool isContainerOfGSLPointer(const CXXRecordDecl *Container) {
+ if (const auto *CTSD = dyn_cast<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());
+ }
+ return false;
}
// Visit lifetimebound or gsl-pointer arguments.
@@ -471,19 +480,10 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
!Callee->getReturnType()->isReferenceType());
} else 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>())
+ // 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>() || isContainerOfGSLPointer(ClassD))
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 91756e80ef9e8b..42b5131440c4f3 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -547,19 +547,22 @@ void test() {
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::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(), std::string_view()}); // expected-warning {{object backing the pointer will be destroyed at the end}}
+ 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> t3 = std::make_optional(s); // expected-warning {{object backing the pointer}}
+ std::optional<std::string_view> o2 = std::make_optional(s); // expected-warning {{object backing the pointer}}
// FIXME: should work for assignment cases
- t1 = {std::string()};
- t2 = std::string();
+ v1 = {std::string()};
+ o1 = std::string();
// no warning on copying pointers.
std::vector<std::string_view> n1 = {std::string_view()};
More information about the cfe-commits
mailing list