https://github.com/geoffreygaren created
https://github.com/llvm/llvm-project/pull/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
>From db2f325b26f14fca4ff0316cbe18ac030f1c1249 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 | 23 ++++++++++++++----
.../StaticAnalyzer/Checkers/WebKit/ASTUtils.h | 7 +++++-
.../WebKit/RawPtrRefLocalVarsChecker.cpp | 9 +++++--
.../Checkers/WebKit/uncounted-local-vars.cpp | 24 +++++++++++++++++++
4 files changed, 55 insertions(+), 8 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index eeb0d8535d8ff..2d9c88de10d11 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -27,7 +27,8 @@ bool tryToFindPtrOrigin(
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)> callback,
+ bool *CrossedShortLivedTemporary) {
while (E) {
if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
if (auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) {
@@ -39,7 +40,14 @@ bool tryToFindPtrOrigin(
return callback(E, true);
}
}
+ if (auto *Cleanups = dyn_cast<ExprWithCleanups>(E)) {
+ E = Cleanups->getSubExpr();
+ continue;
+ }
if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
+ if (CrossedShortLivedTemporary &&
+ tempExpr->getStorageDuration() == SD_FullExpression)
+ *CrossedShortLivedTemporary = true;
E = tempExpr->getSubExpr();
continue;
}
@@ -73,12 +81,17 @@ bool tryToFindPtrOrigin(
continue;
}
if (auto *Expr = dyn_cast<ConditionalOperator>(E)) {
- return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj,
- isSafePtr, isSafePtrType, isSafeGlobalDecl,
- callback) &&
+ const bool ShortLivedOnEntry =
+ CrossedShortLivedTemporary && *CrossedShortLivedTemporary;
+ bool TrueResult = tryToFindPtrOrigin(
+ Expr->getTrueExpr(), StopAtFirstRefCountedObj, isSafePtr,
+ isSafePtrType, isSafeGlobalDecl, callback,
CrossedShortLivedTemporary);
+ if (CrossedShortLivedTemporary)
+ *CrossedShortLivedTemporary = ShortLivedOnEntry;
+ return TrueResult &&
tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj,
isSafePtr, isSafePtrType, isSafeGlobalDecl,
- callback);
+ callback, CrossedShortLivedTemporary);
}
if (auto *cast = dyn_cast<CastExpr>(E)) {
if (StopAtFirstRefCountedObj) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
index fc2c43f33037e..ef1b400120b77 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -52,12 +52,17 @@ class Expr;
/// 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.
+///
+/// If \p CrossedShortLivedTemporary is non-null, it is set to true when the
+/// path to the reported origin crosses a temporary that dies at the end of the
+/// full-expression.
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)> callback,
+ bool *CrossedShortLivedTemporary = nullptr);
/// 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/RawPtrRefLocalVarsChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index b83f1e3ea00b7..a3632187a4510 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -342,6 +342,7 @@ class RawPtrRefLocalVarsChecker
bool isPtrOriginSafe(const VarDecl *V, const Expr *Value,
const Decl *DeclWithIssue) const {
+ bool CrossedShortLivedTemporary = false;
return tryToFindPtrOrigin(
Value, /*StopAtFirstRefCountedObj=*/false,
[&](const clang::CXXRecordDecl *Record) {
@@ -352,9 +353,12 @@ class RawPtrRefLocalVarsChecker
return Model->isSafeDecl(D, BR->getSourceManager());
},
[&](const clang::Expr *InitArgOrigin, bool IsSafe) {
- if (!InitArgOrigin || IsSafe)
+ if (!InitArgOrigin)
return true;
+ if (IsSafe)
+ return !CrossedShortLivedTemporary;
+
if (isa<CXXThisExpr>(InitArgOrigin))
return true;
@@ -377,7 +381,8 @@ class RawPtrRefLocalVarsChecker
return true;
return false;
- });
+ },
+ &CrossedShortLivedTemporary);
}
bool hasGuardian(const VarDecl *V, const Expr *InitArgOrigin,
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
