Author: Haojian Wu Date: 2024-09-25T14:12:49+02:00 New Revision: fe06a6daae6be85d47cd1e51654e91f9ac6e63d7
URL: https://github.com/llvm/llvm-project/commit/fe06a6daae6be85d47cd1e51654e91f9ac6e63d7 DIFF: https://github.com/llvm/llvm-project/commit/fe06a6daae6be85d47cd1e51654e91f9ac6e63d7.diff LOG: Reland: [clang] Diagnose dangling issues for the "Container<GSLPointer>" case. #107213 (#108344) This relands #107213, with with fixes to address false positives (`make_optional(nullptr)`). Added: Modified: clang/docs/ReleaseNotes.rst clang/include/clang/Basic/AttrDocs.td 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 5923888383022a..14907e7db18de3 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -336,6 +336,8 @@ Improvements to Clang's diagnostics local variables passed to function calls using the ``[[clang::musttail]]`` attribute. +- 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 f23a148e546fa3..53d88482698f00 100644 --- a/clang/include/clang/Basic/AttrDocs.td +++ b/clang/include/clang/Basic/AttrDocs.td @@ -6696,6 +6696,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<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 e9e39c11ffbaab..009b8d000e6b0e 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -271,6 +271,49 @@ static bool isInStlNamespace(const Decl *D) { return DC->isStdNamespace(); } +static bool isPointerLikeType(QualType Type) { + return isRecordWithAttr<PointerAttr>(Type) || Type->isPointerType() || + Type->isNullPtrType(); +} + +// Returns true if the given Record decl is a form of `GSLOwner<Pointer>` +// type, e.g. std::vector<string_view>, std::optional<string_view>. +static bool isContainerOfPointer(const RecordDecl *Container) { + if (const auto *CTSD = + dyn_cast_if_present<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 && + isPointerLikeType(TAs[0].getAsType()); + } + return false; +} +static bool isContainerOfOwner(const RecordDecl *Container) { + const auto *CTSD = + dyn_cast_if_present<ClassTemplateSpecializationDecl>(Container); + if (!CTSD) + return false; + 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<OwnerAttr>(TAs[0].getAsType()); +} + +// Returns true if the given Record is `std::initializer_list<pointer>`. +static bool isStdInitializerListOfPointer(const RecordDecl *RD) { + if (const auto *CTSD = + dyn_cast_if_present<ClassTemplateSpecializationDecl>(RD)) { + const auto &TAs = CTSD->getTemplateArgs(); + return isInStlNamespace(RD) && RD->getIdentifier() && + RD->getName() == "initializer_list" && TAs.size() > 0 && + TAs[0].getKind() == TemplateArgument::Type && + isPointerLikeType(TAs[0].getAsType()); + } + return false; +} + static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee)) if (isRecordWithAttr<PointerAttr>(Conv->getConversionType()) && @@ -282,8 +325,7 @@ static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { Callee->getFunctionObjectParameterType()) && !isRecordWithAttr<OwnerAttr>(Callee->getFunctionObjectParameterType())) return false; - if (Callee->getReturnType()->isPointerType() || - isRecordWithAttr<PointerAttr>(Callee->getReturnType())) { + if (isPointerLikeType(Callee->getReturnType())) { if (!Callee->getIdentifier()) return false; return llvm::StringSwitch<bool>(Callee->getName()) @@ -331,6 +373,103 @@ static bool shouldTrackFirstArgument(const FunctionDecl *FD) { return false; } +// Returns true if the given constructor is a copy-like constructor, such as +// `Ctor(Owner<U>&&)` or `Ctor(const Owner<U>&)`. +static bool isCopyLikeConstructor(const CXXConstructorDecl *Ctor) { + if (!Ctor || Ctor->param_size() != 1) + return false; + const auto *ParamRefType = + Ctor->getParamDecl(0)->getType()->getAs<ReferenceType>(); + if (!ParamRefType) + return false; + + // Check if the first parameter type is "Owner<U>". + if (const auto *TST = + ParamRefType->getPointeeType()->getAs<TemplateSpecializationType>()) + return TST->getTemplateName() + .getAsTemplateDecl() + ->getTemplatedDecl() + ->hasAttr<OwnerAttr>(); + return false; +} + +// Returns true if we should perform the GSL analysis on the first argument for +// the given constructor. +static bool +shouldTrackFirstArgumentForConstructor(const CXXConstructExpr *Ctor) { + const auto *LHSRecordDecl = Ctor->getConstructor()->getParent(); + + // Case 1, construct a GSL pointer, e.g. std::string_view + // Always inspect when LHS is a pointer. + if (LHSRecordDecl->hasAttr<PointerAttr>()) + return true; + + if (Ctor->getConstructor()->getNumParams() != 1 || + !isContainerOfPointer(LHSRecordDecl)) + return false; + + // Now, the LHS is an Owner<Pointer> type, e.g., std::vector<string_view>. + // + // At a high level, we cannot precisely determine what the nested pointer + // owns. However, by analyzing the RHS owner type, we can use heuristics to + // infer ownership information. These heuristics are designed to be + // conservative, minimizing false positives while still providing meaningful + // diagnostics. + // + // While this inference isn't perfect, it helps catch common use-after-free + // patterns. + auto RHSArgType = Ctor->getArg(0)->getType(); + const auto *RHSRD = RHSArgType->getAsRecordDecl(); + // LHS is constructed from an intializer_list. + // + // std::initializer_list is a proxy object that provides access to the backing + // array. We perform analysis on it to determine if there are any dangling + // temporaries in the backing array. + // E.g. std::vector<string_view> abc = {string()}; + if (isStdInitializerListOfPointer(RHSRD)) + return true; + + // RHS must be an owner. + if (!isRecordWithAttr<OwnerAttr>(RHSArgType)) + return false; + + // Bail out if the RHS is Owner<Pointer>. + // + // We cannot reliably determine what the LHS nested pointer owns -- it could + // be the entire RHS or the nested pointer in RHS. To avoid false positives, + // we skip this case, such as: + // std::stack<std::string_view> s(std::deque<std::string_view>{}); + // + // TODO: this also has a false negative, it doesn't catch the case like: + // std::optional<span<int*>> os = std::vector<int*>{} + if (isContainerOfPointer(RHSRD)) + return false; + + // Assume that the nested Pointer is constructed from the nested Owner. + // E.g. std::optional<string_view> sv = std::optional<string>(s); + if (isContainerOfOwner(RHSRD)) + return true; + + // Now, the LHS is an Owner<Pointer> and the RHS is an Owner<X>, where X is + // neither an `Owner` nor a `Pointer`. + // + // Use the constructor's signature as a hint. If it is a copy-like constructor + // `Owner1<Pointer>(Owner2<X>&&)`, we assume that the nested pointer is + // constructed from X. In such cases, we do not diagnose, as `X` is not an + // owner, e.g. + // std::optional<string_view> sv = std::optional<Foo>(); + if (const auto *PrimaryCtorTemplate = + Ctor->getConstructor()->getPrimaryTemplate(); + PrimaryCtorTemplate && + isCopyLikeConstructor(dyn_cast_if_present<CXXConstructorDecl>( + PrimaryCtorTemplate->getTemplatedDecl()))) { + return false; + } + // Assume that the nested pointer is constructed from the whole RHS. + // E.g. optional<string_view> s = std::string(); + return true; +} + // Return true if this is an "normal" assignment operator. // We assuments that a normal assingment operator always returns *this, that is, // an lvalue reference that is the same type as the implicit object parameter @@ -473,12 +612,12 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>()) 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<CXXConstructExpr>(Call); - CCE && - CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) { - VisitGSLPointerArg(CCE->getConstructor(), Args[0]); + } else if (auto *Ctor = dyn_cast<CXXConstructExpr>(Call); + Ctor && shouldTrackFirstArgumentForConstructor(Ctor)) { + VisitGSLPointerArg(Ctor->getConstructor(), Args[0]); } } } diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index 69e5395a78a57e..c6272a775a28fa 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<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); + + template<typename InputIterator> + vector(InputIterator first, InputIterator __last); + 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 +216,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<__decay(T)> make_optional(T&&); + template<typename T> struct stack { @@ -587,3 +610,170 @@ std::string_view test2() { return k.value(); // expected-warning {{address of stack memory associated}} } } // namespace GH108272 + +namespace GH100526 { +void test() { + 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(), // expected-warning {{object backing the pointer will be destroyed at the end}} + std::string_view() + }); + 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> o2 = std::make_optional(s); // expected-warning {{object backing the pointer}} + std::optional<std::string_view> o3 = std::optional<std::string>(s); // expected-warning {{object backing the pointer}} + std::optional<std::string_view> o4 = std::optional<std::string_view>(s); + + // FIXME: should work for assignment cases + v1 = {std::string()}; + o1 = 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::string_view(); + std::optional<std::string_view> n4 = std::make_optional(std::string_view()); + const char* b = ""; + std::optional<std::string_view> n5 = std::make_optional(b); + std::optional<std::string_view> n6 = std::make_optional("test"); +} + +std::vector<std::string_view> test2(int i) { + std::vector<std::string_view> t; + if (i) + return t; // this is fine, no dangling + return std::vector<std::string_view>(t.begin(), t.end()); +} + +class Foo { + public: + operator std::string_view() const { return ""; } +}; +class [[gsl::Owner]] FooOwner { + public: + operator std::string_view() const { return ""; } +}; +std::optional<Foo> GetFoo(); +std::optional<FooOwner> GetFooOwner(); + +template <typename T> +struct [[gsl::Owner]] Container1 { + Container1(); +}; +template <typename T> +struct [[gsl::Owner]] Container2 { + template<typename U> + Container2(const Container1<U>& C2); +}; + +std::optional<std::string_view> test3(int i) { + std::string s; + std::string_view sv; + if (i) + return s; // expected-warning {{address of stack memory associated}} + return sv; // fine + Container2<std::string_view> c1 = Container1<Foo>(); // no diagnostic as Foo is not an Owner. + Container2<std::string_view> c2 = Container1<FooOwner>(); // expected-warning {{object backing the pointer will be destroyed}} + return GetFoo(); // fine, we don't know Foo is owner or not, be conservative. + return GetFooOwner(); // expected-warning {{returning address of local temporary object}} +} + +std::optional<int*> test4(int a) { + return std::make_optional(nullptr); // fine +} + + +template <typename T> +struct [[gsl::Owner]] StatusOr { + const T &valueLB() const [[clang::lifetimebound]]; + const T &valueNoLB() const; +}; + +template<typename T> +struct [[gsl::Pointer]] Span { + Span(const std::vector<T> &V); + + const int& getFieldLB() const [[clang::lifetimebound]]; + const int& getFieldNoLB() const; +}; + + +/////// From Owner<Pointer> /////// + +// Pointer from Owner<Pointer> +std::string_view test5() { + std::string_view a = StatusOr<std::string_view>().valueLB(); // expected-warning {{object backing the pointer will be dest}} +return StatusOr<std::string_view>().valueLB(); // expected-warning {{returning address of local temporary}} + + // No dangling diagnostics on non-lifetimebound methods. + std::string_view b = StatusOr<std::string_view>().valueNoLB(); + return StatusOr<std::string_view>().valueNoLB(); +} + +// Pointer<Pointer> from Owner<Pointer> +// Prevent regression GH108463 +Span<int*> test6(std::vector<int*> v) { + Span<int *> dangling = std::vector<int*>(); // expected-warning {{object backing the pointer}} + return v; // expected-warning {{address of stack memory}} +} + +/////// From Owner<Owner<Pointer>> /////// + +// Pointer from Owner<Owner<Pointer>> +int* test7(StatusOr<StatusOr<int*>> aa) { + // No dangling diagnostic on pointer. + return aa.valueLB().valueLB(); // OK. +} + +// Owner<Pointer> from Owner<Owner<Pointer>> +std::vector<int*> test8(StatusOr<std::vector<int*>> aa) { + return aa.valueLB(); // OK, no pointer being construct on this case. + return aa.valueNoLB(); +} + +// Pointer<Pointer> from Owner<Owner<Pointer>> +Span<int*> test9(StatusOr<std::vector<int*>> aa) { + return aa.valueLB(); // expected-warning {{address of stack memory associated}} + return aa.valueNoLB(); // OK. +} + +/////// From Owner<Owner> /////// + +// Pointer<Owner>> from Owner<Owner> +Span<std::string> test10(StatusOr<std::vector<std::string>> aa) { + return aa.valueLB(); // expected-warning {{address of stack memory}} + return aa.valueNoLB(); // OK. +} + +/////// From Owner<Pointer<Owner>> /////// + +// Pointer<Owner>> from Owner<Pointer<Owner>> +Span<std::string> test11(StatusOr<Span<std::string>> aa) { + return aa.valueLB(); // expected-warning {{address of stack memory}} + return aa.valueNoLB(); // OK. +} + +// Lifetimebound and gsl::Pointer. +const int& test12(Span<int> a) { + return a.getFieldLB(); // expected-warning {{reference to stack memory associated}} + return a.getFieldNoLB(); // OK. +} + +void test13() { + // FIXME: RHS is Owner<Pointer>, we skip this case to avoid false positives. + std::optional<Span<int*>> abc = std::vector<int*>{}; + + std::optional<Span<int>> t = std::vector<int> {}; // expected-warning {{object backing the pointer will be destroyed}} +} + +} // namespace GH100526 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
