https://github.com/geoffreygaren updated https://github.com/llvm/llvm-project/pull/224877
>From 7c1e7ae06cff3a73b85a8f105ecbb46eb1b84c7b Mon Sep 17 00:00:00 2001 From: Geoff Garen <[email protected]> Date: Tue, 25 Aug 2026 19:29:20 -0700 Subject: [PATCH] [WebKit Checkers] Trace through temporaries in tryToFindPtrOrigin RefPtr checking skips temporaries, reporting a path through any temporary as unsafe. This is mostly correct, but not always. For example, the following is a false positive: // makeKey() returns a temporary RefCountable* p = condition(makeKey()) ? guardian.ptr() : nullptr; In the upcoming Borrow checker, it's even more important to trace through temporaries because not tracing an expression can drop a `lifetimebound` link, resulting in false **negatives**. This patch adds tracing through temporaries. The logic is: * In function call arguments, temporaries are lifetime safe because the full expression does not end until the call returns * In ranged for loops, temporaries are lifetime safe because lifetime extends past the full expression to the duration of the loop (C++ P2718) * Otherwise, temporaries are not lifetime safe Assisted-by: Claude --- .../Checkers/WebKit/ASTUtils.cpp | 98 ++++++++++++------- .../StaticAnalyzer/Checkers/WebKit/ASTUtils.h | 12 ++- .../WebKit/RawPtrRefCallArgsChecker.cpp | 5 +- .../WebKit/RawPtrRefLocalVarsChecker.cpp | 8 +- .../Checkers/WebKit/uncounted-local-vars.cpp | 24 +++++ 5 files changed, 104 insertions(+), 43 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index eeb0d8535d8ff..39411c8c76c2c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -15,6 +15,7 @@ #include "clang/AST/ExprObjC.h" #include "clang/AST/StmtVisitor.h" #include <optional> +#include <utility> namespace clang { @@ -22,24 +23,33 @@ bool isSafePtr(clang::CXXRecordDecl *Decl) { return isRefCounted(Decl) || isCheckedPtr(Decl); } -bool tryToFindPtrOrigin( - const Expr *E, bool StopAtFirstRefCountedObj, - std::function<bool(const clang::CXXRecordDecl *)> isSafePtr, - std::function<bool(const clang::QualType)> isSafePtrType, - std::function<bool(const clang::Decl *)> isSafeGlobalDecl, - std::function<bool(const clang::Expr *, bool)> callback) { +static bool +findPtrOriginImpl(const Expr *E, bool StopAtFirstRefCountedObj, + std::function<bool(const clang::CXXRecordDecl *)> isSafePtr, + std::function<bool(const clang::QualType)> isSafePtrType, + std::function<bool(const clang::Decl *)> isSafeGlobalDecl, + std::function<bool(const clang::Expr *, bool /*IsSafe*/, + bool /*CrossedShortLivedTemporary*/)> + callback, + bool CrossedShortLivedTemporary) { while (E) { if (auto *DRE = dyn_cast<DeclRefExpr>(E)) { if (auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) { auto QT = VD->getType(); auto IsImmortal = safeGetName(VD) == "NSApp"; if (VD->hasGlobalStorage() && (IsImmortal || QT.isConstQualified())) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); if (VD->hasGlobalStorage() && isSafeGlobalDecl(VD)) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } } + if (auto *Cleanups = dyn_cast<ExprWithCleanups>(E)) { + E = Cleanups->getSubExpr(); + continue; + } if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) { + if (tempExpr->getStorageDuration() == SD_FullExpression) + CrossedShortLivedTemporary = true; E = tempExpr->getSubExpr(); continue; } @@ -50,13 +60,13 @@ bool tryToFindPtrOrigin( if (auto *tempExpr = dyn_cast<CXXConstructExpr>(E)) { if (auto *C = tempExpr->getConstructor()) { if (auto *Class = C->getParent(); Class && isSafePtr(Class)) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); break; } } if (auto *TempExpr = dyn_cast<CXXUnresolvedConstructExpr>(E)) { if (isSafePtrType(TempExpr->getTypeAsWritten())) - return callback(TempExpr, true); + return callback(TempExpr, /*IsSafe=*/true, CrossedShortLivedTemporary); } if (auto *POE = dyn_cast<PseudoObjectExpr>(E)) { if (auto *RF = POE->getResultExpr()) { @@ -73,22 +83,22 @@ bool tryToFindPtrOrigin( continue; } if (auto *Expr = dyn_cast<ConditionalOperator>(E)) { - return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj, - isSafePtr, isSafePtrType, isSafeGlobalDecl, - callback) && - tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj, - isSafePtr, isSafePtrType, isSafeGlobalDecl, - callback); + return findPtrOriginImpl(Expr->getTrueExpr(), StopAtFirstRefCountedObj, + isSafePtr, isSafePtrType, isSafeGlobalDecl, + callback, CrossedShortLivedTemporary) && + findPtrOriginImpl(Expr->getFalseExpr(), StopAtFirstRefCountedObj, + isSafePtr, isSafePtrType, isSafeGlobalDecl, + callback, CrossedShortLivedTemporary); } if (auto *cast = dyn_cast<CastExpr>(E)) { if (StopAtFirstRefCountedObj) { if (auto *ConversionFunc = dyn_cast_or_null<FunctionDecl>(cast->getConversionFunction())) { if (isCtorOfSafePtr(ConversionFunc)) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } if (isa<CXXFunctionalCastExpr>(E) && isSafePtrType(cast->getType())) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } // FIXME: This can give false "origin" that would lead to false negatives // in checkers. See https://reviews.llvm.org/D37023 for reference. @@ -100,12 +110,12 @@ bool tryToFindPtrOrigin( if (Callee->hasAttr<CFReturnsRetainedAttr>() || Callee->hasAttr<NSReturnsRetainedAttr>() || Callee->hasAttr<NSReturnsAutoreleasedAttr>()) { - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } } if (isSafePtrType(call->getType())) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); if (auto *memberCall = dyn_cast<CXXMemberCallExpr>(call)) { if (auto *decl = memberCall->getMethodDecl()) { @@ -113,7 +123,7 @@ bool tryToFindPtrOrigin( if (IsGetterOfRefCt && *IsGetterOfRefCt) { E = memberCall->getImplicitObjectArgument(); if (StopAtFirstRefCountedObj) { - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } continue; } @@ -142,7 +152,7 @@ bool tryToFindPtrOrigin( if (auto *callee = call->getDirectCallee()) { if (isCtorOfSafePtr(callee)) { if (StopAtFirstRefCountedObj) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); E = call->getArg(0); continue; @@ -154,10 +164,10 @@ bool tryToFindPtrOrigin( } if (isSafePtrType(callee->getReturnType())) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); if (isSingleton(callee)) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); if (callee->isInStdNamespace() && safeGetName(callee) == "forward") { E = call->getArg(0); @@ -174,11 +184,11 @@ bool tryToFindPtrOrigin( Name == "NSStringFromSelector" || Name == "NSSelectorFromString" || Name == "NSStringFromClass" || Name == "NSClassFromString" || Name == "NSStringFromProtocol" || Name == "NSProtocolFromString") - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } else if (auto *CalleeE = call->getCallee()) { if (auto *E = dyn_cast<DeclRefExpr>(CalleeE->IgnoreParenCasts())) { if (isSingleton(E->getFoundDecl())) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } if (auto *MemberExpr = dyn_cast<CXXDependentScopeMemberExpr>(CalleeE)) { @@ -186,7 +196,7 @@ bool tryToFindPtrOrigin( auto MemberName = MemberExpr->getMember().getAsString(); bool IsGetter = MemberName == "get" || MemberName == "ptr"; if (Base && isSafePtrType(Base->getType()) && IsGetter) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } } @@ -201,7 +211,8 @@ bool tryToFindPtrOrigin( if (auto *RD = dyn_cast<RecordType>(SubstType)) { if (auto *CXX = dyn_cast<CXXRecordDecl>(RD->getDecl())) if (isSafePtr(CXX)) - return callback(E, true); + return callback(E, /*IsSafe=*/true, + CrossedShortLivedTemporary); } } } @@ -211,22 +222,23 @@ bool tryToFindPtrOrigin( if (auto *ObjCMsgExpr = dyn_cast<ObjCMessageExpr>(E)) { if (auto *Method = ObjCMsgExpr->getMethodDecl()) { if (isSafePtrType(Method->getReturnType())) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } auto Selector = ObjCMsgExpr->getSelector(); auto NameForFirstSlot = Selector.getNameForSlot(0); if ((NameForFirstSlot == "class" || NameForFirstSlot == "superclass") && !Selector.getNumArgs()) - return callback(E, true); + return callback(E, /*IsSafe=*/true, CrossedShortLivedTemporary); } if (auto *ObjCProtocol = dyn_cast<ObjCProtocolExpr>(E)) - return callback(ObjCProtocol, true); + return callback(ObjCProtocol, /*IsSafe=*/true, + CrossedShortLivedTemporary); if (auto *ObjCDict = dyn_cast<ObjCDictionaryLiteral>(E)) - return callback(ObjCDict, true); + return callback(ObjCDict, /*IsSafe=*/true, CrossedShortLivedTemporary); if (auto *ObjCArray = dyn_cast<ObjCArrayLiteral>(E)) - return callback(ObjCArray, true); + return callback(ObjCArray, /*IsSafe=*/true, CrossedShortLivedTemporary); if (auto *ObjCStr = dyn_cast<ObjCStringLiteral>(E)) - return callback(ObjCStr, true); + return callback(ObjCStr, /*IsSafe=*/true, CrossedShortLivedTemporary); if (auto *unaryOp = dyn_cast<UnaryOperator>(E)) { // FIXME: Currently accepts ANY unary operator. Is it OK? E = unaryOp->getSubExpr(); @@ -234,14 +246,28 @@ bool tryToFindPtrOrigin( } if (auto *BoxedExpr = dyn_cast<ObjCBoxedExpr>(E)) { if (StopAtFirstRefCountedObj) - return callback(BoxedExpr, true); + return callback(BoxedExpr, /*IsSafe=*/true, CrossedShortLivedTemporary); E = BoxedExpr->getSubExpr(); continue; } break; } // Some other expression. - return callback(E, false); + return callback(E, /*IsSafe=*/false, CrossedShortLivedTemporary); +} + +bool tryToFindPtrOrigin( + const Expr *E, bool StopAtFirstRefCountedObj, + std::function<bool(const clang::CXXRecordDecl *)> isSafePtr, + std::function<bool(const clang::QualType)> isSafePtrType, + std::function<bool(const clang::Decl *)> isSafeGlobalDecl, + std::function<bool(const clang::Expr *, bool /*IsSafe*/, + bool /*CrossedShortLivedTemporary*/)> + callback) { + return findPtrOriginImpl(E, StopAtFirstRefCountedObj, std::move(isSafePtr), + std::move(isSafePtrType), + std::move(isSafeGlobalDecl), std::move(callback), + /*CrossedShortLivedTemporary=*/false); } bool isASafeCallArg(const Expr *E) { diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h index fc2c43f33037e..47a86adb8453b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h @@ -49,15 +49,19 @@ class Expr; /// represents ref-counted object during the traversal we return relevant /// sub-expression and true. /// -/// Calls \p callback with the subexpression that we traversed to and if \p -/// StopAtFirstRefCountedObj is true we also specify whether we stopped early. -/// Returns false if any of calls to callbacks returned false. Otherwise true. +/// Calls \p callback for each origin the traversal reaches, passing the +/// subexpression, whether the traversal recognized it as a safe origin, and +/// whether the path to it crossed a temporary that dies at the end of the +/// full-expression. Returns false if any of calls to callbacks returned false. +/// Otherwise true. bool tryToFindPtrOrigin( const clang::Expr *E, bool StopAtFirstRefCountedObj, std::function<bool(const clang::CXXRecordDecl *)> isSafePtr, std::function<bool(const clang::QualType)> isSafePtrType, std::function<bool(const clang::Decl *)> isSafeGlobalDecl, - std::function<bool(const clang::Expr *, bool)> callback); + std::function<bool(const clang::Expr *, bool /*IsSafe*/, + bool /*CrossedShortLivedTemporary*/)> + callback); /// For \p E referring to a ref-countable/-counted pointer/reference we return /// whether it's a safe call argument. Examples: function parameter or diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp index 7e5261723014a..654621665bde6 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp @@ -252,7 +252,10 @@ class RawPtrRefCallArgsChecker [&](const clang::Decl *D) { return Model->isSafeDecl(D, BR->getSourceManager()); }, - [&](const clang::Expr *ArgOrigin, bool IsSafe) { + // A temporary on the path to an argument's origin is safe: the full + // expression does not end until the call returns. + [&](const clang::Expr *ArgOrigin, bool IsSafe, + bool /*CrossedShortLivedTemporary*/) { if (IsSafe) return true; if (isNullPtr(ArgOrigin)) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp index b83f1e3ea00b7..8884cee77a9ad 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp @@ -351,10 +351,14 @@ class RawPtrRefLocalVarsChecker [&](const clang::Decl *D) { return Model->isSafeDecl(D, BR->getSourceManager()); }, - [&](const clang::Expr *InitArgOrigin, bool IsSafe) { - if (!InitArgOrigin || IsSafe) + [&](const clang::Expr *InitArgOrigin, bool IsSafe, + bool CrossedShortLivedTemporary) { + if (!InitArgOrigin) return true; + if (IsSafe) + return !CrossedShortLivedTemporary; + if (isa<CXXThisExpr>(InitArgOrigin)) return true; diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp index c6c75968ae924..2a3d9f2fefab8 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -799,3 +799,27 @@ namespace using_reexported_ref_deref { } } + +namespace short_lived_temporaries { + +Ref<RefCountable> provide_ref(); +bool condition(const Ref<RefCountable> &); + +void dying_ref_temporary() { + RefCountable *bar = provide_ref().ptr(); + // expected-warning@-1{{Local variable 'bar' is a raw pointer to RefPtr-capable type 'RefCountable' [alpha.webkit.UncountedLocalVarsChecker]}} + someFunction(); + bar->method(); +} + +void unrelated_temporary_traces_to_guardian(RefCountable &obj) { + Ref<RefCountable> guardian(obj); + { + RefCountable *bar = condition(provide_ref()) ? guardian.ptr() : nullptr; + someFunction(); + if (bar) + bar->method(); + } +} + +} // namespace short_lived_temporaries _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
