[clang] [analyzer] Support determining origins in a conditional operator in WebKit checkers. (PR #91143)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Sun May 5 13:57:34 PDT 2024
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91143
>From 477ef4bc201240044051410ac2d2f33b10049d49 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Sun, 5 May 2024 13:40:10 -0700
Subject: [PATCH] [analyzer] Support determining origins in a conditional
operator in WebKit checkers.
This PR adds the support for determining the origin of a pointer in a conditional operator.
Because such an expression can have two distinct origins each of which needs to be visited,
this PR refactors tryToFindPtrOrigin to take a callback instead of returning a pair.
The callback is called for the second operand and the third operand of the conditioanl
operator (i.e. E2 and E3 in E1 ? E2 : E3).
Also treat nullptr and integer literal as safe pointer origins in the local variable checker.
---
.../Checkers/WebKit/ASTUtils.cpp | 23 ++++--
.../StaticAnalyzer/Checkers/WebKit/ASTUtils.h | 11 ++-
.../WebKit/UncountedCallArgsChecker.cpp | 36 +++++----
.../WebKit/UncountedLocalVarsChecker.cpp | 73 +++++++++++--------
.../Analysis/Checkers/WebKit/call-args.cpp | 14 ++++
.../Checkers/WebKit/uncounted-local-vars.cpp | 18 +++++
6 files changed, 113 insertions(+), 62 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index b36fa95bc73f3e..a80ba9ca7dfae1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -16,8 +16,9 @@
namespace clang {
-std::pair<const Expr *, bool>
-tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
+bool tryToFindPtrOrigin(
+ const Expr *E, bool StopAtFirstRefCountedObj,
+ std::function<bool(const clang::Expr *, bool)> callback) {
while (E) {
if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
E = tempExpr->getSubExpr();
@@ -27,12 +28,18 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
E = tempExpr->getSubExpr();
continue;
}
+ if (auto* Expr = dyn_cast<ConditionalOperator>(E)) {
+ return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj,
+ callback) &&
+ tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj,
+ callback);
+ }
if (auto *cast = dyn_cast<CastExpr>(E)) {
if (StopAtFirstRefCountedObj) {
if (auto *ConversionFunc =
dyn_cast_or_null<FunctionDecl>(cast->getConversionFunction())) {
if (isCtorOfRefCounted(ConversionFunc))
- return {E, true};
+ return callback(E, true);
}
}
// FIXME: This can give false "origin" that would lead to false negatives
@@ -47,7 +54,7 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
if (IsGetterOfRefCt && *IsGetterOfRefCt) {
E = memberCall->getImplicitObjectArgument();
if (StopAtFirstRefCountedObj) {
- return {E, true};
+ return callback(E, true);
}
continue;
}
@@ -64,17 +71,17 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
if (auto *callee = call->getDirectCallee()) {
if (isCtorOfRefCounted(callee)) {
if (StopAtFirstRefCountedObj)
- return {E, true};
+ return callback(E, true);
E = call->getArg(0);
continue;
}
if (isReturnValueRefCounted(callee))
- return {E, true};
+ return callback(E, true);
if (isSingleton(callee))
- return {E, true};
+ return callback(E, true);
if (isPtrConversion(callee)) {
E = call->getArg(0);
@@ -91,7 +98,7 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
break;
}
// Some other expression.
- return {E, false};
+ return callback(E, 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 e35ea4ef05dd17..e972924e0c523d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -13,6 +13,7 @@
#include "llvm/ADT/APInt.h"
#include "llvm/Support/Casting.h"
+#include <functional>
#include <string>
#include <utility>
@@ -48,10 +49,12 @@ class Expr;
/// represents ref-counted object during the traversal we return relevant
/// sub-expression and true.
///
-/// \returns subexpression that we traversed to and if \p
-/// StopAtFirstRefCountedObj is true we also return whether we stopped early.
-std::pair<const clang::Expr *, bool>
-tryToFindPtrOrigin(const clang::Expr *E, bool StopAtFirstRefCountedObj);
+/// 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.
+bool tryToFindPtrOrigin(
+ const clang::Expr *E, bool StopAtFirstRefCountedObj,
+ std::function<bool(const clang::Expr *, bool)> 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/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index 0f40ecc7ba3000..b9281d9a171b47 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -126,25 +126,23 @@ class UncountedCallArgsChecker
}
bool isPtrOriginSafe(const Expr *Arg) const {
- std::pair<const clang::Expr *, bool> ArgOrigin =
- tryToFindPtrOrigin(Arg, true);
-
- // Temporary ref-counted object created as part of the call argument
- // would outlive the call.
- if (ArgOrigin.second)
- return true;
-
- if (isa<CXXNullPtrLiteralExpr>(ArgOrigin.first)) {
- // foo(nullptr)
- return true;
- }
- if (isa<IntegerLiteral>(ArgOrigin.first)) {
- // FIXME: Check the value.
- // foo(NULL)
- return true;
- }
-
- return isASafeCallArg(ArgOrigin.first);
+ return tryToFindPtrOrigin(Arg, /*StopAtFirstRefCountedObj=*/true,
+ [](const clang::Expr* ArgOrigin, bool IsSafe) {
+ if (IsSafe)
+ return true;
+ if (isa<CXXNullPtrLiteralExpr>(ArgOrigin)) {
+ // foo(nullptr)
+ return true;
+ }
+ if (isa<IntegerLiteral>(ArgOrigin)) {
+ // FIXME: Check the value.
+ // foo(NULL)
+ return true;
+ }
+ if (isASafeCallArg(ArgOrigin))
+ return true;
+ return false;
+ });
}
bool shouldSkipCall(const CallExpr *CE) const {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 6036ad58cf253c..46147cc57deb92 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -188,39 +188,50 @@ class UncountedLocalVarsChecker
if (!InitExpr)
return; // FIXME: later on we might warn on uninitialized vars too
- const clang::Expr *const InitArgOrigin =
- tryToFindPtrOrigin(InitExpr, /*StopAtFirstRefCountedObj=*/false)
- .first;
- if (!InitArgOrigin)
+ if (tryToFindPtrOrigin(
+ InitExpr, /*StopAtFirstRefCountedObj=*/false,
+ [&](const clang::Expr* InitArgOrigin, bool IsSafe) {
+ if (!InitArgOrigin)
+ return true;
+
+ if (isa<CXXThisExpr>(InitArgOrigin))
+ return true;
+
+ if (isa<CXXNullPtrLiteralExpr>(InitArgOrigin))
+ return true;
+
+ if (isa<IntegerLiteral>(InitArgOrigin))
+ return true;
+
+ if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
+ if (auto *MaybeGuardian =
+ dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
+ const auto *MaybeGuardianArgType =
+ MaybeGuardian->getType().getTypePtr();
+ if (MaybeGuardianArgType) {
+ const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
+ MaybeGuardianArgType->getAsCXXRecordDecl();
+ if (MaybeGuardianArgCXXRecord) {
+ if (MaybeGuardian->isLocalVarDecl() &&
+ (isRefCounted(MaybeGuardianArgCXXRecord) ||
+ isRefcountedStringsHack(MaybeGuardian)) &&
+ isGuardedScopeEmbeddedInGuardianScope(
+ V, MaybeGuardian))
+ return true;
+ }
+ }
+
+ // Parameters are guaranteed to be safe for the duration of
+ // the call by another checker.
+ if (isa<ParmVarDecl>(MaybeGuardian))
+ return true;
+ }
+ }
+
+ return false;
+ }))
return;
- if (isa<CXXThisExpr>(InitArgOrigin))
- return;
-
- if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
- if (auto *MaybeGuardian =
- dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
- const auto *MaybeGuardianArgType =
- MaybeGuardian->getType().getTypePtr();
- if (MaybeGuardianArgType) {
- const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
- MaybeGuardianArgType->getAsCXXRecordDecl();
- if (MaybeGuardianArgCXXRecord) {
- if (MaybeGuardian->isLocalVarDecl() &&
- (isRefCounted(MaybeGuardianArgCXXRecord) ||
- isRefcountedStringsHack(MaybeGuardian)) &&
- isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian))
- return;
- }
- }
-
- // Parameters are guaranteed to be safe for the duration of the call
- // by another checker.
- if (isa<ParmVarDecl>(MaybeGuardian))
- return;
- }
- }
-
reportBug(V);
}
}
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index 2a4b6bb1f1063a..5d864a90f6da6b 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -333,3 +333,17 @@ namespace cxx_member_operator_call {
// expected-warning at -1{{Call argument for parameter 'bad' is uncounted and unsafe}}
}
}
+
+namespace call_with_ptr_on_ref {
+ Ref<RefCountable> provideProtected();
+ void bar(RefCountable* bad);
+ bool baz();
+ void foo(bool v) {
+ bar(v ? nullptr : provideProtected().ptr());
+ bar(baz() ? provideProtected().ptr() : nullptr);
+ bar(v ? provide() : provideProtected().ptr());
+ // expected-warning at -1{{Call argument for parameter 'bad' is uncounted and unsafe}}
+ bar(v ? provideProtected().ptr() : provide());
+ // expected-warning at -1{{Call argument for parameter 'bad' is uncounted and unsafe}}
+ }
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 00673e91f471ea..00cea6a3b58945 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -187,3 +187,21 @@ void bar() {
}
} // namespace ignore_for_if
+
+namespace conditional_op {
+RefCountable *provide_ref_ctnbl();
+bool bar();
+
+void foo() {
+ RefCountable *a = bar() ? nullptr : provide_ref_ctnbl();
+ // expected-warning at -1{{Local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ RefPtr<RefCountable> b = provide_ref_ctnbl();
+ {
+ RefCountable* c = bar() ? nullptr : b.get();
+ c->method();
+ RefCountable* d = bar() ? b.get() : nullptr;
+ d->method();
+ }
+}
+
+} // namespace conditional_op
\ No newline at end of file
More information about the cfe-commits
mailing list