diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 250821a9f9c45..59ccdf1e15cd8 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -298,6 +298,8 @@ Improvements to Clang's diagnostics - Clang now warns for u8 character literals used in C23 with ``-Wpre-c23-compat`` instead of ``-Wpre-c++17-compat``. +- Clang now diagnoses cases where a dangling ``GSLOwner`` object is constructed, e.g. ``std::vector 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 546e5100b79dd..9f72456d2da67 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 pointer type (raw pointer, or ``[[gsl::Pointer]]``), +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 v = {std::string()}; // v holds a dangling pointer. + std::optional o = std::string(); // o holds a dangling pointer. + } + }]; } diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index f1507ebb9a506..c8e703036c132 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -267,6 +267,26 @@ static bool isInStlNamespace(const Decl *D) { return DC->isStdNamespace(); } +// Returns true if the given Record decl is a form of `GSLOwner` +// type, e.g. std::vector, std::optional. +static bool isContainerOfPointer(const RecordDecl *Container) { + if (const auto *CTSD = + dyn_cast_if_present(Container)) { + if (!CTSD->hasAttr()) // Container must be a GSL owner type. + return false; + const auto &TAs = CTSD->getTemplateArgs(); + return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type && + (isRecordWithAttr(TAs[0].getAsType()) || + TAs[0].getAsType()->isPointerType()); + } + return false; +} + +static bool isGSLOwner(QualType T) { + return isRecordWithAttr(T) && + !isContainerOfPointer(T->getAsRecordDecl()); +} + static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { if (auto *Conv = dyn_cast_or_null(Callee)) if (isRecordWithAttr(Conv->getConversionType())) @@ -275,7 +295,7 @@ static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { return false; if (!isRecordWithAttr( Callee->getFunctionObjectParameterType()) && - !isRecordWithAttr(Callee->getFunctionObjectParameterType())) + !isGSLOwner(Callee->getFunctionObjectParameterType())) return false; if (Callee->getReturnType()->isPointerType() || isRecordWithAttr(Callee->getReturnType())) { @@ -413,7 +433,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, // Once we initialized a value with a non gsl-owner reference, it can no // longer dangle. if (ReturnType->isReferenceType() && - !isRecordWithAttr(ReturnType->getPointeeType())) { + !isGSLOwner(ReturnType->getPointeeType())) { for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) { if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit || PE.Kind == IndirectLocalPathEntry::LifetimeBoundCall) @@ -468,12 +488,17 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr()) VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]); else if (EnableGSLAnalysis && I == 0) { + // Perform GSL analysis for the first argument if (shouldTrackFirstArgument(Callee)) { VisitGSLPointerArg(Callee, Args[0]); - } else if (auto *CCE = dyn_cast(Call); - CCE && - CCE->getConstructor()->getParent()->hasAttr()) { - VisitGSLPointerArg(CCE->getConstructor(), Args[0]); + } else if (auto *Ctor = dyn_cast(Call)) { + const auto *ClassD = Ctor->getConstructor()->getParent(); + // Two cases: + // a GSL pointer, e.g. std::string_view + // a container of GSL pointer, e.g. std::vector + if (ClassD->hasAttr() || + (isContainerOfPointer(ClassD) && Callee->getNumParams() == 1)) + VisitGSLPointerArg(Ctor->getConstructor(), Args[0]); } } } @@ -990,13 +1015,12 @@ static void checkExprLifetimeImpl(Sema &SemaRef, // int &p = *localUniquePtr; // someContainer.add(std::move(localUniquePtr)); // return p; - IsLocalGslOwner = isRecordWithAttr(L->getType()); + IsLocalGslOwner = isGSLOwner(L->getType()); if (pathContainsInit(Path) || !IsLocalGslOwner) return false; } else { IsGslPtrValueFromGslTempOwner = - MTE && !MTE->getExtendingDecl() && - isRecordWithAttr(MTE->getType()); + MTE && !MTE->getExtendingDecl() && isGSLOwner(MTE->getType()); // Skipping a chain of initializing gsl::Pointer annotated objects. // We are looking only for the final source to find out if it was // a local or temporary owner or the address of a local variable/param. diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index 59357d0730a7d..234e06f069074 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -158,17 +158,30 @@ auto begin(C &c) -> decltype(c.begin()); template T *begin(T (&array)[N]); +using size_t = decltype(sizeof(0)); + +template +struct initializer_list { + const T* ptr; size_t sz; +}; template struct vector { typedef __gnu_cxx::basic_iterator iterator; iterator begin(); iterator end(); const T *data() const; + vector(); + vector(initializer_list __l); + + template + vector(InputIterator first, InputIterator __last); + T &at(int n); }; template struct basic_string_view { + basic_string_view(); basic_string_view(const T *); const T *begin() const; }; @@ -203,11 +216,21 @@ template struct optional { optional(); optional(const T&); + + template + optional(U&& t); + + template + optional(optional&& __t); + T &operator*() &; T &&operator*() &&; T &value() &; T &&value() &&; }; +template +optional<__decay(T)> make_optional(T&&); + template struct stack { @@ -553,3 +576,57 @@ 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 v1({std::string()}); // expected-warning {{object backing the pointer will be destroyed at the end}} + std::vector v2({ + std::string(), // expected-warning {{object backing the pointer will be destroyed at the end}} + std::string_view() + }); + std::vector v3({ + std::string_view(), + std::string() // expected-warning {{object backing the pointer will be destroyed at the end}} + }); + + std::optional 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"" 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 o2 = std::make_optional(s); // expected-warning {{object backing the pointer}} + std::optional o3 = std::optional(s); // expected-warning {{object backing the pointer}} + std::optional o4 = std::optional(s); + + // FIXME: should work for assignment cases + v1 = {std::string()}; + o1 = std::string(); + + // no warning on copying pointers. + std::vector n1 = {std::string_view()}; + std::optional n2 = {std::string_view()}; + std::optional n3 = std::string_view(); + std::optional n4 = std::make_optional(std::string_view()); + const char* b = ""; + std::optional n5 = std::make_optional(b); + std::optional n6 = std::make_optional("test"); +} + +std::vector test2(int i) { + std::vector t; + if (i) + return t; // this is fine, no dangling + return std::vector(t.begin(), t.end()); +} + +std::optional test3(int i) { + std::string s; + std::string_view sv; + if (i) + return s; // expected-warning {{address of stack memory associated}} + return sv; // fine +} + +} // namespace GH100526