Skip to content

Commit

Permalink
[WebKit Checkers] Allow a guardian CheckedPtr/CheckedRef (llvm#110222)
Browse files Browse the repository at this point in the history
This PR makes WebKit checkers allow a guardian variable which is
CheckedPtr or CheckedRef as in addition to RefPtr or Ref.
  • Loading branch information
rniwa authored Oct 25, 2024
1 parent 81e536e commit 5c20891
Show file tree
Hide file tree
Showing 8 changed files with 177 additions and 20 deletions.
16 changes: 10 additions & 6 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@

namespace clang {

bool isSafePtr(clang::CXXRecordDecl *Decl) {
return isRefCounted(Decl) || isCheckedPtr(Decl);
}

bool tryToFindPtrOrigin(
const Expr *E, bool StopAtFirstRefCountedObj,
std::function<bool(const clang::Expr *, bool)> callback) {
Expand All @@ -31,7 +35,7 @@ bool tryToFindPtrOrigin(
}
if (auto *tempExpr = dyn_cast<CXXTemporaryObjectExpr>(E)) {
if (auto *C = tempExpr->getConstructor()) {
if (auto *Class = C->getParent(); Class && isRefCounted(Class))
if (auto *Class = C->getParent(); Class && isSafePtr(Class))
return callback(E, true);
break;
}
Expand All @@ -56,7 +60,7 @@ bool tryToFindPtrOrigin(
if (StopAtFirstRefCountedObj) {
if (auto *ConversionFunc =
dyn_cast_or_null<FunctionDecl>(cast->getConversionFunction())) {
if (isCtorOfRefCounted(ConversionFunc))
if (isCtorOfSafePtr(ConversionFunc))
return callback(E, true);
}
}
Expand All @@ -68,7 +72,7 @@ bool tryToFindPtrOrigin(
if (auto *call = dyn_cast<CallExpr>(E)) {
if (auto *memberCall = dyn_cast<CXXMemberCallExpr>(call)) {
if (auto *decl = memberCall->getMethodDecl()) {
std::optional<bool> IsGetterOfRefCt = isGetterOfRefCounted(decl);
std::optional<bool> IsGetterOfRefCt = isGetterOfSafePtr(decl);
if (IsGetterOfRefCt && *IsGetterOfRefCt) {
E = memberCall->getImplicitObjectArgument();
if (StopAtFirstRefCountedObj) {
Expand All @@ -87,15 +91,15 @@ bool tryToFindPtrOrigin(
}

if (auto *callee = call->getDirectCallee()) {
if (isCtorOfRefCounted(callee)) {
if (isCtorOfRefCounted(callee) || isCtorOfCheckedPtr(callee)) {
if (StopAtFirstRefCountedObj)
return callback(E, true);

E = call->getArg(0);
continue;
}

if (isRefType(callee->getReturnType()))
if (isSafePtrType(callee->getReturnType()))
return callback(E, true);

if (isSingleton(callee))
Expand All @@ -114,7 +118,7 @@ bool tryToFindPtrOrigin(
}
if (auto *ObjCMsgExpr = dyn_cast<ObjCMessageExpr>(E)) {
if (auto *Method = ObjCMsgExpr->getMethodDecl()) {
if (isRefType(Method->getReturnType()))
if (isSafePtrType(Method->getReturnType()))
return callback(E, true);
}
}
Expand Down
43 changes: 37 additions & 6 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,16 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
|| FunctionName == "Identifier";
}

bool isRefType(const clang::QualType T) {
bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
assert(F);
return isCheckedPtr(safeGetName(F));
}

bool isCtorOfSafePtr(const clang::FunctionDecl *F) {
return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F);
}

bool isSafePtrType(const clang::QualType T) {
QualType type = T;
while (!type.isNull()) {
if (auto *elaboratedT = type->getAs<ElaboratedType>()) {
Expand All @@ -145,7 +154,7 @@ bool isRefType(const clang::QualType T) {
if (auto *specialT = type->getAs<TemplateSpecializationType>()) {
if (auto *decl = specialT->getTemplateName().getAsTemplateDecl()) {
auto name = decl->getNameAsString();
return isRefType(name);
return isRefType(name) || isCheckedPtr(name);
}
return false;
}
Expand Down Expand Up @@ -177,6 +186,12 @@ std::optional<bool> isUncounted(const CXXRecordDecl* Class)
return (*IsRefCountable);
}

std::optional<bool> isUnchecked(const CXXRecordDecl *Class) {
if (isCheckedPtr(Class))
return false; // Cheaper than below
return isCheckedPtrCapable(Class);
}

std::optional<bool> isUncountedPtr(const QualType T) {
if (T->isPointerType() || T->isReferenceType()) {
if (auto *CXXRD = T->getPointeeCXXRecordDecl())
Expand All @@ -185,15 +200,26 @@ std::optional<bool> isUncountedPtr(const QualType T) {
return false;
}

std::optional<bool> isGetterOfRefCounted(const CXXMethodDecl* M)
{
std::optional<bool> isUnsafePtr(const QualType T) {
if (T->isPointerType() || T->isReferenceType()) {
if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
return isUncounted(CXXRD) || isUnchecked(CXXRD);
}
}
return false;
}

std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl *M) {
assert(M);

if (isa<CXXMethodDecl>(M)) {
const CXXRecordDecl *calleeMethodsClass = M->getParent();
auto className = safeGetName(calleeMethodsClass);
auto method = safeGetName(M);

if (isCheckedPtr(className) && (method == "get" || method == "ptr"))
return true;

if ((isRefType(className) && (method == "get" || method == "ptr")) ||
((className == "String" || className == "AtomString" ||
className == "AtomStringImpl" || className == "UniqueString" ||
Expand All @@ -205,7 +231,12 @@ std::optional<bool> isGetterOfRefCounted(const CXXMethodDecl* M)
// FIXME: Currently allowing any Ref<T> -> whatever cast.
if (isRefType(className)) {
if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
return isUncountedPtr(maybeRefToRawOperator->getConversionType());
return isUnsafePtr(maybeRefToRawOperator->getConversionType());
}

if (isCheckedPtr(className)) {
if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
return isUnsafePtr(maybeRefToRawOperator->getConversionType());
}
}
return false;
Expand Down Expand Up @@ -448,7 +479,7 @@ class TrivialFunctionAnalysisVisitor
if (!Callee)
return false;

std::optional<bool> IsGetterOfRefCounted = isGetterOfRefCounted(Callee);
std::optional<bool> IsGetterOfRefCounted = isGetterOfSafePtr(Callee);
if (IsGetterOfRefCounted && *IsGetterOfRefCounted)
return true;

Expand Down
22 changes: 17 additions & 5 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,30 @@ std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
/// class, false if not, std::nullopt if inconclusive.
std::optional<bool> isUncountedPtr(const clang::QualType T);

/// \returns true if Name is a RefPtr, Ref, or its variant, false if not.
bool isRefType(const std::string &Name);
/// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
/// variant, false if not.
bool isSafePtrType(const clang::QualType T);

/// \returns true if \p F creates ref-countable object from uncounted parameter,
/// false if not.
bool isCtorOfRefCounted(const clang::FunctionDecl *F);

/// \returns true if \p T is RefPtr, Ref, or its variant, false if not.
bool isRefType(const clang::QualType T);
/// \returns true if \p F creates checked ptr object from uncounted parameter,
/// false if not.
bool isCtorOfCheckedPtr(const clang::FunctionDecl *F);

/// \returns true if \p F creates ref-countable or checked ptr object from
/// uncounted parameter, false if not.
bool isCtorOfSafePtr(const clang::FunctionDecl *F);

/// \returns true if \p Name is RefPtr, Ref, or its variant, false if not.
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 M is getter of a ref-counted class, false if not.
std::optional<bool> isGetterOfRefCounted(const clang::CXXMethodDecl* Method);
std::optional<bool> isGetterOfSafePtr(const clang::CXXMethodDecl *Method);

/// \returns true if \p F is a conversion between ref-countable or ref-counted
/// pointer types.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ class UncountedCallArgsChecker
auto name = safeGetName(MD);
if (name == "ref" || name == "deref")
return;
if (name == "incrementPtrCount" || name == "decrementPtrCount")
return;
}
auto *E = MemberCallExpr->getImplicitObjectArgument();
QualType ArgType = MemberCallExpr->getObjectType().getCanonicalType();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ class UncountedLocalVarsChecker
if (MaybeGuardianArgCXXRecord) {
if (MaybeGuardian->isLocalVarDecl() &&
(isRefCounted(MaybeGuardianArgCXXRecord) ||
isCheckedPtr(MaybeGuardianArgCXXRecord) ||
isRefcountedStringsHack(MaybeGuardian)) &&
isGuardedScopeEmbeddedInGuardianScope(
V, MaybeGuardian))
Expand Down
46 changes: 46 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s

#include "mock-types.h"

RefCountableAndCheckable* makeObj();
CheckedRef<RefCountableAndCheckable> makeObjChecked();
void someFunction(RefCountableAndCheckable*);

namespace call_args_unchecked_uncounted {

static void foo() {
someFunction(makeObj());
// expected-warning@-1{{Call argument is uncounted and unsafe [alpha.webkit.UncountedCallArgsChecker]}}
}

} // namespace call_args_checked

namespace call_args_checked {

static void foo() {
CheckedPtr<RefCountableAndCheckable> ptr = makeObj();
someFunction(ptr.get());
}

static void bar() {
someFunction(CheckedPtr { makeObj() }.get());
}

static void baz() {
someFunction(makeObjChecked().ptr());
}

} // namespace call_args_checked

namespace call_args_default {

void someFunction(RefCountableAndCheckable* = makeObj());
// expected-warning@-1{{Call argument is uncounted and unsafe [alpha.webkit.UncountedCallArgsChecker]}}
void otherFunction(RefCountableAndCheckable* = makeObjChecked().ptr());

void foo() {
someFunction();
otherFunction();
}

}
16 changes: 13 additions & 3 deletions clang/test/Analysis/Checkers/WebKit/mock-types.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ template <typename T> struct CheckedRef {

public:
CheckedRef() : t{} {};
CheckedRef(T &t) : t(t) { t->incrementPtrCount(); }
CheckedRef(const CheckedRef& o) : t(o.t) { if (t) t->incrementPtrCount(); }
CheckedRef(T &t) : t(&t) { t.incrementPtrCount(); }
CheckedRef(const CheckedRef &o) : t(o.t) { if (t) t->incrementPtrCount(); }
~CheckedRef() { if (t) t->decrementPtrCount(); }
T &get() { return *t; }
T *ptr() { return t; }
Expand All @@ -135,7 +135,7 @@ template <typename T> struct CheckedPtr {
if (t)
t->incrementPtrCount();
}
CheckedPtr(Ref<T>&& o)
CheckedPtr(Ref<T> &&o)
: t(o.leakRef())
{ }
~CheckedPtr() {
Expand All @@ -156,4 +156,14 @@ class CheckedObj {
void decrementPtrCount();
};

class RefCountableAndCheckable {
public:
void incrementPtrCount() const;
void decrementPtrCount() const;
void ref() const;
void deref() const;
void method();
int trivial() { return 0; }
};

#endif
51 changes: 51 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,57 @@ void foo() {

} // namespace local_assignment_to_global

namespace local_refcountable_checkable_object {

RefCountableAndCheckable* provide_obj();

void local_raw_ptr() {
RefCountableAndCheckable* a = nullptr;
// expected-warning@-1{{Local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
a = provide_obj();
a->method();
}

void local_checked_ptr() {
CheckedPtr<RefCountableAndCheckable> a = nullptr;
a = provide_obj();
a->method();
}

void local_var_with_guardian_checked_ptr() {
CheckedPtr<RefCountableAndCheckable> a = provide_obj();
{
auto* b = a.get();
b->method();
}
}

void local_var_with_guardian_checked_ptr_with_assignment() {
CheckedPtr<RefCountableAndCheckable> a = provide_obj();
{
RefCountableAndCheckable* b = a.get();
// expected-warning@-1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
b = provide_obj();
b->method();
}
}

void local_var_with_guardian_checked_ref() {
CheckedRef<RefCountableAndCheckable> a = *provide_obj();
{
RefCountableAndCheckable& b = a;
b.method();
}
}

void static_var() {
static RefCountableAndCheckable* a = nullptr;
// expected-warning@-1{{Static local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
a = provide_obj();
}

} // namespace local_refcountable_checkable_object

namespace local_var_in_recursive_function {

struct TreeNode {
Expand Down

0 comments on commit 5c20891

Please sign in to comment.