llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> Add support for `std::unique_ptr::release()` in lifetime analysis to avoid false positives when ownership is manually transferred via `release()`. - Added a new function `isUniquePtrRelease()` to detect when `std::unique_ptr::release()` is called - Modified `handleInvalidatingCall()` to mark the unique_ptr as moved when release() is called When manually transferring ownership using `std::unique_ptr::release()`, the lifetime analysis would previously generate false positive use-after-free warnings. This change treats `release()` as a move operation, correctly modeling the ownership transfer semantics and reducing false positives in code that manually manages ownership. --- Full diff: https://github.com/llvm/llvm-project/pull/180230.diff 5 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h (+5) - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+11) - (modified) clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp (+20) - (modified) clang/test/Sema/Inputs/lifetime-analysis.h (+1) - (modified) clang/test/Sema/warn-lifetime-safety.cpp (+12) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h index ad3a48667cf2a..d306093cdc8fc 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h @@ -66,6 +66,11 @@ bool isGslPointerType(QualType QT); // Tells whether the type is annotated with [[gsl::Owner]]. bool isGslOwnerType(QualType QT); +// Returns true if the given method is std::unique_ptr::release(). +// This is treated as a move in lifetime analysis to avoid false-positives +// when ownership is manually transferred. +bool isUniquePtrRelease(const CXXMethodDecl *MD); + // Returns true if the given method invalidates iterators or references to // container elements (e.g. vector::push_back). bool isContainerInvalidationMethod(const CXXMethodDecl &MD); diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index be7886b093bb2..5564b8d04e2d0 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -546,6 +546,17 @@ void FactsGenerator::handleMovedArgsInCall(const FunctionDecl *FD, void FactsGenerator::handleInvalidatingCall(const Expr *Call, const FunctionDecl *FD, ArrayRef<const Expr *> Args) { + // std::unique_ptr::release() transfers ownership. + // Treat it as a move to prevent false-positive warnings when the unique_ptr + // destructor runs after ownership has been transferred. + if (const auto *Method = dyn_cast<CXXMethodDecl>(FD); + Method && isUniquePtrRelease(Method)) { + const Expr *UniquePtrExpr = Args[0]; + OriginList *MovedOrigins = getOriginsList(*UniquePtrExpr); + if (MovedOrigins) + CurrentBlockFacts.push_back(FactMgr.createFact<MovedOriginFact>( + UniquePtrExpr, MovedOrigins->getOuterOriginID())); + } const auto *MD = dyn_cast<CXXMethodDecl>(FD); if (!MD || !MD->isInstance() || !isContainerInvalidationMethod(*MD)) return; diff --git a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp index 51196efd3116e..c95c9f088d49e 100644 --- a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp +++ b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp @@ -255,6 +255,26 @@ template <typename T> static bool isRecordWithAttr(QualType Type) { bool isGslPointerType(QualType QT) { return isRecordWithAttr<PointerAttr>(QT); } bool isGslOwnerType(QualType QT) { return isRecordWithAttr<OwnerAttr>(QT); } +static bool isStdUniquePtr(const CXXRecordDecl *RD) { + if (!RD || !RD->isInStdNamespace()) + return false; + + StringRef Name; + if (const auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD)) + Name = CTSD->getSpecializedTemplate()->getName(); + else if (RD->getIdentifier()) + Name = RD->getName(); + else + return false; + + return Name == "unique_ptr"; +} + +bool isUniquePtrRelease(const CXXMethodDecl *MD) { + return MD && MD->getIdentifier() && MD->getName() == "release" && + MD->getNumParams() == 0 && isStdUniquePtr(MD->getParent()); +} + bool isContainerInvalidationMethod(const CXXMethodDecl &MD) { const CXXRecordDecl *RD = MD.getParent(); if (!isInStlNamespace(RD)) diff --git a/clang/test/Sema/Inputs/lifetime-analysis.h b/clang/test/Sema/Inputs/lifetime-analysis.h index 38a28e8dcc49c..f30db1a29b149 100644 --- a/clang/test/Sema/Inputs/lifetime-analysis.h +++ b/clang/test/Sema/Inputs/lifetime-analysis.h @@ -129,6 +129,7 @@ struct unique_ptr { unique_ptr(); unique_ptr(unique_ptr<T>&&); ~unique_ptr(); + T* release(); T &operator*(); T *get() const; }; diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp index 5ad856529ffdf..8f52ff27bc6fd 100644 --- a/clang/test/Sema/warn-lifetime-safety.cpp +++ b/clang/test/Sema/warn-lifetime-safety.cpp @@ -1456,6 +1456,18 @@ void wrong_use_of_move_is_permissive() { } // expected-note {{destroyed here}} (void)p; // expected-note {{later used here}} } + +void take(int*); +void test_release_no_uaf() { + int* r; + // Calling release() marks p as moved from, so its destruction doesn't invalidate r. + { + std::unique_ptr<int> p; + r = p.get(); // expected-warning-re {{object whose reference {{.*}} may have been moved}} + take(p.release()); // expected-note {{potentially moved here}} + } // expected-note {{destroyed here}} + (void)*r; // expected-note {{later used here}} +} } // namespace strict_warn_on_move // Implicit this annotations with redecls. `````````` </details> https://github.com/llvm/llvm-project/pull/180230 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
