[clang] 9906c58 - [WebKit Checkers] Trace through temporaries in tryToFindPtrOrigin (#224877)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 21 13:50:03 PDT 2026
Author: geoffreygaren
Date: 2026-09-21T13:49:56-07:00
New Revision: 9906c580517b6e22df73a6f7206de551e4883a52
URL: https://github.com/llvm/llvm-project/commit/9906c580517b6e22df73a6f7206de551e4883a52
DIFF: https://github.com/llvm/llvm-project/commit/9906c580517b6e22df73a6f7206de551e4883a52.diff
LOG: [WebKit Checkers] Trace through temporaries in tryToFindPtrOrigin (#224877)
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
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index eeb0d8535d8ff..2717ccfe53fee 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,35 @@ bool isSafePtr(clang::CXXRecordDecl *Decl) {
return isRefCounted(Decl) || isCheckedPtr(Decl);
}
-bool tryToFindPtrOrigin(
+static bool tryToFindPtrOriginImpl(
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) {
+ std::function<bool(const clang::Expr *, bool /*IsSafe*/,
+ bool /*OriginDependsOnFullExpressionTemporary*/)>
+ callback,
+ bool OriginDependsOnFullExpressionTemporary) {
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,
+ OriginDependsOnFullExpressionTemporary);
if (VD->hasGlobalStorage() && isSafeGlobalDecl(VD))
- return callback(E, true);
+ return callback(E, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
}
}
+ if (auto *Cleanups = dyn_cast<ExprWithCleanups>(E)) {
+ E = Cleanups->getSubExpr();
+ continue;
+ }
if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
+ if (tempExpr->getStorageDuration() == SD_FullExpression)
+ OriginDependsOnFullExpressionTemporary = true;
E = tempExpr->getSubExpr();
continue;
}
@@ -50,13 +62,15 @@ 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,
+ OriginDependsOnFullExpressionTemporary);
break;
}
}
if (auto *TempExpr = dyn_cast<CXXUnresolvedConstructExpr>(E)) {
if (isSafePtrType(TempExpr->getTypeAsWritten()))
- return callback(TempExpr, true);
+ return callback(TempExpr, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
}
if (auto *POE = dyn_cast<PseudoObjectExpr>(E)) {
if (auto *RF = POE->getResultExpr()) {
@@ -73,22 +87,26 @@ 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 tryToFindPtrOriginImpl(Expr->getTrueExpr(),
+ StopAtFirstRefCountedObj, isSafePtr,
+ isSafePtrType, isSafeGlobalDecl, callback,
+ OriginDependsOnFullExpressionTemporary) &&
+ tryToFindPtrOriginImpl(Expr->getFalseExpr(),
+ StopAtFirstRefCountedObj, isSafePtr,
+ isSafePtrType, isSafeGlobalDecl, callback,
+ OriginDependsOnFullExpressionTemporary);
}
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,
+ OriginDependsOnFullExpressionTemporary);
}
if (isa<CXXFunctionalCastExpr>(E) && isSafePtrType(cast->getType()))
- return callback(E, true);
+ return callback(E, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
}
// FIXME: This can give false "origin" that would lead to false negatives
// in checkers. See https://reviews.llvm.org/D37023 for reference.
@@ -100,12 +118,14 @@ bool tryToFindPtrOrigin(
if (Callee->hasAttr<CFReturnsRetainedAttr>() ||
Callee->hasAttr<NSReturnsRetainedAttr>() ||
Callee->hasAttr<NSReturnsAutoreleasedAttr>()) {
- return callback(E, true);
+ return callback(E, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
}
}
if (isSafePtrType(call->getType()))
- return callback(E, true);
+ return callback(E, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
if (auto *memberCall = dyn_cast<CXXMemberCallExpr>(call)) {
if (auto *decl = memberCall->getMethodDecl()) {
@@ -113,7 +133,8 @@ bool tryToFindPtrOrigin(
if (IsGetterOfRefCt && *IsGetterOfRefCt) {
E = memberCall->getImplicitObjectArgument();
if (StopAtFirstRefCountedObj) {
- return callback(E, true);
+ return callback(E, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
}
continue;
}
@@ -142,7 +163,8 @@ bool tryToFindPtrOrigin(
if (auto *callee = call->getDirectCallee()) {
if (isCtorOfSafePtr(callee)) {
if (StopAtFirstRefCountedObj)
- return callback(E, true);
+ return callback(E, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
E = call->getArg(0);
continue;
@@ -154,10 +176,12 @@ bool tryToFindPtrOrigin(
}
if (isSafePtrType(callee->getReturnType()))
- return callback(E, true);
+ return callback(E, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
if (isSingleton(callee))
- return callback(E, true);
+ return callback(E, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
if (callee->isInStdNamespace() && safeGetName(callee) == "forward") {
E = call->getArg(0);
@@ -174,11 +198,13 @@ bool tryToFindPtrOrigin(
Name == "NSStringFromSelector" || Name == "NSSelectorFromString" ||
Name == "NSStringFromClass" || Name == "NSClassFromString" ||
Name == "NSStringFromProtocol" || Name == "NSProtocolFromString")
- return callback(E, true);
+ return callback(E, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
} 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,
+ OriginDependsOnFullExpressionTemporary);
}
if (auto *MemberExpr = dyn_cast<CXXDependentScopeMemberExpr>(CalleeE)) {
@@ -186,7 +212,8 @@ 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,
+ OriginDependsOnFullExpressionTemporary);
}
}
@@ -201,7 +228,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,
+ OriginDependsOnFullExpressionTemporary);
}
}
}
@@ -211,22 +239,28 @@ 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,
+ OriginDependsOnFullExpressionTemporary);
}
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,
+ OriginDependsOnFullExpressionTemporary);
}
if (auto *ObjCProtocol = dyn_cast<ObjCProtocolExpr>(E))
- return callback(ObjCProtocol, true);
+ return callback(ObjCProtocol, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
if (auto *ObjCDict = dyn_cast<ObjCDictionaryLiteral>(E))
- return callback(ObjCDict, true);
+ return callback(ObjCDict, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
if (auto *ObjCArray = dyn_cast<ObjCArrayLiteral>(E))
- return callback(ObjCArray, true);
+ return callback(ObjCArray, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
if (auto *ObjCStr = dyn_cast<ObjCStringLiteral>(E))
- return callback(ObjCStr, true);
+ return callback(ObjCStr, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
if (auto *unaryOp = dyn_cast<UnaryOperator>(E)) {
// FIXME: Currently accepts ANY unary operator. Is it OK?
E = unaryOp->getSubExpr();
@@ -234,14 +268,30 @@ bool tryToFindPtrOrigin(
}
if (auto *BoxedExpr = dyn_cast<ObjCBoxedExpr>(E)) {
if (StopAtFirstRefCountedObj)
- return callback(BoxedExpr, true);
+ return callback(BoxedExpr, /*IsSafe=*/true,
+ OriginDependsOnFullExpressionTemporary);
E = BoxedExpr->getSubExpr();
continue;
}
break;
}
// Some other expression.
- return callback(E, false);
+ return callback(E, /*IsSafe=*/false, OriginDependsOnFullExpressionTemporary);
+}
+
+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 /*OriginDependsOnFullExpressionTemporary*/)>
+ callback) {
+ return tryToFindPtrOriginImpl(
+ E, StopAtFirstRefCountedObj, std::move(isSafePtr),
+ std::move(isSafePtrType), std::move(isSafeGlobalDecl),
+ std::move(callback),
+ /*OriginDependsOnFullExpressionTemporary=*/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..5507dd239affb 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -49,15 +49,20 @@ 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 passed through a temporary that dies at the end of
+/// the full-expression; in that case the origin's lifetime guarantee cannot
+/// be assumed to extend past 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 /*OriginDependsOnFullExpressionTemporary*/)>
+ 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..3c5a2f708bff7 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 /*OriginDependsOnFullExpressionTemporary*/) {
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..2d34ed9e4fae3 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 OriginDependsOnFullExpressionTemporary) {
+ if (!InitArgOrigin)
return true;
+ if (IsSafe)
+ return !OriginDependsOnFullExpressionTemporary;
+
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 at -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
More information about the cfe-commits
mailing list