https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/81400
This PR makes alpha.webkit.UncountedCallArgsChecker eplicitly check the safety
of the object argument in a member function call. It also removes the exemption
of local variables from this checker so that each local variable's safety is
checked if it's used in a function call instead of relying on the local
variable checker to find those since local variable checker currently has
exemption for "for" and "if" statements.
>From b7121ce4f2ef69b4a410f2399fbd9d9525156b93 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@webkit.org>
Date: Sun, 11 Feb 2024 00:07:30 -0800
Subject: [PATCH] [alpha.webkit.UncountedCallArgsChecker] Check the safety of
the object argument in a member function call.
This PR makes alpha.webkit.UncountedCallArgsChecker eplicitly check the safety
of the object argument in
a member function call. It also removes the exemption of local variables from
this checker so that each
local variable's safety is checked if it's used in a function call instead of
relying on the local variable
checker to find those since local variable checker currently has exemption for
"for" and "if" statements.
---
.../Checkers/WebKit/ASTUtils.cpp | 2 +-
.../WebKit/UncountedCallArgsChecker.cpp | 68 +++++++++++++------
.../WebKit/call-args-inside-if-statement.cpp | 20 ++++++
3 files changed, 69 insertions(+), 21 deletions(-)
create mode 100644
clang/test/Analysis/Checkers/WebKit/call-args-inside-if-statement.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 4526fac64735bf..da0d52e361c946 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -85,7 +85,7 @@ bool isASafeCallArg(const Expr *E) {
assert(E);
if (auto *Ref = dyn_cast<DeclRefExpr>(E)) {
if (auto *D = dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
- if (isa<ParmVarDecl>(D) || D->isLocalVarDecl())
+ if (isa<ParmVarDecl>(D))
return true;
}
}
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index 31ccae8b097b89..fa6aeb4741d0b7 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -70,6 +70,14 @@ class UncountedCallArgsChecker
// or std::function call operator).
unsigned ArgIdx = isa<CXXOperatorCallExpr>(CE) &&
isa_and_nonnull<CXXMethodDecl>(F);
+ if (auto* MemberCallExpr = dyn_cast<CXXMemberCallExpr>(CE)) {
+ auto *E = MemberCallExpr->getImplicitObjectArgument();
+ auto *ArgType = MemberCallExpr->getObjectType().getTypePtrOrNull();
+ std::optional<bool> IsUncounted =
isUncounted(ArgType->getAsCXXRecordDecl());
+ if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E))
+ reportBugOnThis(E);
+ }
+
for (auto P = F->param_begin();
// FIXME: Also check variadic function parameters.
// FIXME: Also check default function arguments. Probably a
different
@@ -90,32 +98,36 @@ class UncountedCallArgsChecker
continue;
const auto *Arg = CE->getArg(ArgIdx);
-
- 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)
- continue;
-
- if (isa<CXXNullPtrLiteralExpr>(ArgOrigin.first)) {
- // foo(nullptr)
- continue;
- }
- if (isa<IntegerLiteral>(ArgOrigin.first)) {
- // FIXME: Check the value.
- // foo(NULL)
- continue;
- }
-
- if (isASafeCallArg(ArgOrigin.first))
+
+ if (isPtrOriginSafe(Arg))
continue;
reportBug(Arg, *P);
}
}
}
+
+ 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);
+ }
bool shouldSkipCall(const CallExpr *CE) const {
if (CE->getNumArgs() == 0)
@@ -183,6 +195,22 @@ class UncountedCallArgsChecker
Report->addRange(CallArg->getSourceRange());
BR->emitReport(std::move(Report));
}
+
+ void reportBugOnThis(const Expr *CallArg) const {
+ assert(CallArg);
+
+ SmallString<100> Buf;
+ llvm::raw_svector_ostream Os(Buf);
+
+ Os << "Call argument for 'this' parameter is uncounted and unsafe.";
+
+ const SourceLocation SrcLocToReport = CallArg->getSourceRange().getBegin();
+
+ PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager());
+ auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+ Report->addRange(CallArg->getSourceRange());
+ BR->emitReport(std::move(Report));
+ }
};
} // namespace
diff --git
a/clang/test/Analysis/Checkers/WebKit/call-args-inside-if-statement.cpp
b/clang/test/Analysis/Checkers/WebKit/call-args-inside-if-statement.cpp
new file mode 100644
index 00000000000000..6f7c959b2fccca
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-inside-if-statement.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_analyze_cc1
-analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
+
+#include "mock-types.h"
+
+class RefCounted {
+public:
+ void ref() const;
+ void deref() const;
+ void someFunction();
+};
+
+RefCounted* refCountedObj();
+
+void test()
+{
+ if (auto* obj = refCountedObj()) {
+ obj->someFunction();
+ // expected-warning@-1{{Call argument for 'this' parameter is uncounted
and unsafe}}
+ }
+}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
