Skip to content

[WebKit checkers] Don't treat virtual functions as safe. #129632

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

Merged
merged 1 commit into from
Mar 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
}

Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
13 changes: 13 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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]}}
}
}
17 changes: 16 additions & 1 deletion clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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 };
Expand Down Expand Up @@ -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*);
Expand Down