[WebKit checkers] Don't treat virtual functions as safe. #129632
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this PR, WebKit checkers erroneously treated functions to be safe if it has a trivial body even if it was marked as virtual. In the case of a virtual function, it can have an override which does not pass the triviality check so we must not make such an assumption. This PR also restricts the allowed operator overloading while finding the pointer origin to just operators on smart pointer types: Ref, RefPtr, CheckedRef, CheckedPtr, RetainPtr, WeakPtr, WeakRef, unique_ptr, and UniqueRef.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesPrior to this PR, WebKit checkers erroneously treated functions to be safe if it has a trivial body even if it was marked as virtual. In the case of a virtual function, it can have an override which does not pass the triviality check so we must not make such an assumption. This PR also restricts the allowed operator overloading while finding the pointer origin to just operators on smart pointer types: Ref, RefPtr, CheckedRef, CheckedPtr, RetainPtr, WeakPtr, WeakRef, unique_ptr, and UniqueRef. Full diff: https://github.com/llvm/llvm-project/pull/129632.diff 6 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index dc86c4fcc64b1..34d3fa8381b38 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -87,9 +87,17 @@ bool tryToFindPtrOrigin(
}
if (auto *operatorCall = dyn_cast<CXXOperatorCallExpr>(E)) {
- if (operatorCall->getNumArgs() == 1) {
- E = operatorCall->getArg(0);
- continue;
+ if (auto *Callee = operatorCall->getDirectCallee()) {
+ auto ClsName = safeGetName(Callee->getParent());
+ if (isRefType(ClsName) || isCheckedPtr(ClsName) ||
+ isRetainPtr(ClsName) || ClsName == "unique_ptr" ||
+ ClsName == "UniqueRef" || ClsName == "WeakPtr" ||
+ ClsName == "WeakRef") {
+ if (operatorCall->getNumArgs() == 1) {
+ E = operatorCall->getArg(0);
+ continue;
+ }
+ }
}
}
@@ -206,7 +214,7 @@ bool EnsureFunctionAnalysis::isACallToEnsureFn(const clang::Expr *E) const {
if (!Callee)
return false;
auto *Body = Callee->getBody();
- if (!Body)
+ if (!Body || Callee->isVirtualAsWritten())
return false;
auto [CacheIt, IsNew] = Cache.insert(std::make_pair(Callee, false));
if (IsNew)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 7899b19854806..d4fe487e74f8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -479,6 +479,10 @@ class TrivialFunctionAnalysisVisitor
TrivialFunctionAnalysisVisitor(CacheTy &Cache) : Cache(Cache) {}
bool IsFunctionTrivial(const Decl *D) {
+ if (auto *FnDecl = dyn_cast<FunctionDecl>(D)) {
+ if (FnDecl->isVirtualAsWritten())
+ return false;
+ }
return WithCachedResult(D, [&]() {
if (auto *CtorDecl = dyn_cast<CXXConstructorDecl>(D)) {
for (auto *CtorInit : CtorDecl->inits()) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index fcc1a41dba78b..323d473665888 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -131,6 +131,9 @@ bool isRefType(const std::string &Name);
/// \returns true if \p Name is CheckedRef or CheckedPtr, false if not.
bool isCheckedPtr(const std::string &Name);
+/// \returns true if \p Name is RetainPtr or its variant, false if not.
+bool isRetainPtr(const std::string &Name);
+
/// \returns true if \p M is getter of a ref-counted class, false if not.
std::optional<bool> isGetterOfSafePtr(const clang::CXXMethodDecl *Method);
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index d633fbcbd798b..9d07c65da88af 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -172,7 +172,7 @@ class RawPtrRefCallArgsChecker
if (BR->getSourceManager().isInSystemHeader(CE->getExprLoc()))
return true;
- if (Callee && TFA.isTrivial(Callee))
+ if (Callee && TFA.isTrivial(Callee) && !Callee->isVirtualAsWritten())
return true;
if (CE->getNumArgs() == 0)
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 52854cd10f68c..07b6de21df80f 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -464,4 +464,17 @@ namespace local_var_for_singleton {
RefCountable* bar = singleton();
RefCountable* baz = otherSingleton();
}
+}
+
+namespace virtual_function {
+ struct SomeObject {
+ virtual RefCountable* provide() { return nullptr; }
+ virtual RefCountable* operator&() { return nullptr; }
+ };
+ void foo(SomeObject* obj) {
+ auto* bar = obj->provide();
+ // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ auto* baz = &*obj;
+ // expected-warning@-1{{Local variable 'baz' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ }
}
\ No newline at end of file
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
index fe7ce158eb8ba..0279e2c68ec6d 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
@@ -196,6 +196,10 @@ class ComplexNumber {
ComplexNumber& operator+();
const Number& real() const { return realPart; }
+ const Number& complex() const;
+
+ void ref() const;
+ void deref() const;
private:
Number realPart;
@@ -240,6 +244,11 @@ class SomeType : public BaseType {
using BaseType::BaseType;
};
+struct OtherObj {
+ unsigned v { 0 };
+ OtherObj* children[4] { nullptr };
+};
+
void __libcpp_verbose_abort(const char *__format, ...);
class RefCounted {
@@ -375,7 +384,7 @@ class RefCounted {
double y;
};
void trivial68() { point pt = { 1.0 }; }
- unsigned trivial69() { return offsetof(RefCounted, children); }
+ unsigned trivial69() { return offsetof(OtherObj, children); }
DerivedNumber* trivial70() { [[clang::suppress]] return static_cast<DerivedNumber*>(number); }
static RefCounted& singleton() {
@@ -467,6 +476,8 @@ class RefCounted {
unsigned nonTrivial22() { return ComplexNumber(123, "456").real().value(); }
unsigned nonTrivial23() { return DerivedNumber("123").value(); }
SomeType nonTrivial24() { return SomeType("123"); }
+ virtual void nonTrivial25() { }
+ virtual ComplexNumber* operator->() { return nullptr; }
static unsigned s_v;
unsigned v { 0 };
@@ -642,6 +653,10 @@ class UnrelatedClass {
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
getFieldTrivial().nonTrivial24();
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+ getFieldTrivial().nonTrivial25();
+ // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+ getFieldTrivial()->complex();
+ // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
}
void setField(RefCounted*);
|
|
Thanks for the review! |
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Mar 11, 2025
Prior to this PR, WebKit checkers erroneously treated functions to be safe if it has a trivial body even if it was marked as virtual. In the case of a virtual function, it can have an override which does not pass the triviality check so we must not make such an assumption. This PR also restricts the allowed operator overloading while finding the pointer origin to just operators on smart pointer types: Ref, RefPtr, CheckedRef, CheckedPtr, RetainPtr, WeakPtr, WeakRef, unique_ptr, and UniqueRef.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this PR, WebKit checkers erroneously treated functions to be safe if it has a trivial body even if it was marked as virtual. In the case of a virtual function, it can have an override which does not pass the triviality check so we must not make such an assumption.
This PR also restricts the allowed operator overloading while finding the pointer origin to just operators on smart pointer types: Ref, RefPtr, CheckedRef, CheckedPtr, RetainPtr, WeakPtr, WeakRef, unique_ptr, and UniqueRef.