llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: geoffreygaren
<details>
<summary>Changes</summary>
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
---
Full diff: https://github.com/llvm/llvm-project/pull/224877.diff
4 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+18-5)
- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h (+6-1)
- (modified)
clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp (+7-2)
- (modified) clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp (+24)
``````````diff
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
``````````
</details>
https://github.com/llvm/llvm-project/pull/224877
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
