-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[alpha.webkit.UncountedCallArgsChecker] Check the safety of the object argument in a member function call. #81400
Conversation
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesThis 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. Full diff: https://github.com/llvm/llvm-project/pull/81400.diff 3 Files Affected:
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}}
+ }
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
c8ac9ad
to
04e1825
Compare
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Needs a rebase now though.
…t 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.
… don't use raw_svector_ostream.
2d78038
to
c15967f
Compare
Rebased. |
…on call. (llvm#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. (cherry picked from commit 3a49dfb)
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.