Skip to content

Introduce a new WebKit checker for a unchecked local variable #113708

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 3 commits into from
Nov 1, 2024
Merged
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
47 changes: 42 additions & 5 deletions clang/docs/analyzer/checkers.rst
Original file line number Diff line number Diff line change
@@ -3584,7 +3584,7 @@ These are examples of cases that we consider safe:
RefCountable* uncounted = this; // ok
}

Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that an argument is safe or it's considered if not a bug then bug-prone.
Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that a local variable is safe or it's considered unsafe.

.. code-block:: cpp

@@ -3603,11 +3603,48 @@ Here are some examples of situations that we warn about as they *might* be poten
RefCountable* uncounted = counted.get(); // warn
}

We don't warn about these cases - we don't consider them necessarily safe but since they are very common and usually safe we'd introduce a lot of false positives otherwise:
- variable defined in condition part of an ```if``` statement
- variable defined in init statement condition of a ```for``` statement
alpha.webkit.UncheckedLocalVarsChecker
""""""""""""""""""""""""""""""""""""""
The goal of this rule is to make sure that any unchecked local variable is backed by a CheckedPtr or CheckedRef with lifetime that is strictly larger than the scope of the unchecked local variable. To be on the safe side we require the scope of an unchecked variable to be embedded in the scope of CheckedPtr/CheckRef object that backs it.

These are examples of cases that we consider safe:

.. code-block:: cpp

For the time being we also don't warn about uninitialized uncounted local variables.
void foo1() {
CheckedPtr<RefCountable> counted;
// The scope of uncounted is EMBEDDED in the scope of counted.
{
RefCountable* uncounted = counted.get(); // ok
}
}

void foo2(CheckedPtr<RefCountable> counted_param) {
RefCountable* uncounted = counted_param.get(); // ok
}

void FooClass::foo_method() {
RefCountable* uncounted = this; // ok
}

Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that a local variable is safe or it's considered unsafe.

.. code-block:: cpp

void foo1() {
RefCountable* uncounted = new RefCountable; // warn
}

RefCountable* global_uncounted;
void foo2() {
RefCountable* uncounted = global_uncounted; // warn
}

void foo3() {
RefPtr<RefCountable> counted;
// The scope of uncounted is not EMBEDDED in the scope of counted.
RefCountable* uncounted = counted.get(); // warn
}

Debug Checkers
---------------
4 changes: 4 additions & 0 deletions clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
Original file line number Diff line number Diff line change
@@ -1764,4 +1764,8 @@ def UncountedLocalVarsChecker : Checker<"UncountedLocalVarsChecker">,
HelpText<"Check uncounted local variables.">,
Documentation<HasDocumentation>;

def UncheckedLocalVarsChecker : Checker<"UncheckedLocalVarsChecker">,
HelpText<"Check unchecked local variables.">,
Documentation<HasDocumentation>;

} // end alpha.webkit
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -136,7 +136,7 @@ add_clang_library(clangStaticAnalyzerCheckers
WebKit/RefCntblBaseVirtualDtorChecker.cpp
WebKit/UncountedCallArgsChecker.cpp
WebKit/UncountedLambdaCapturesChecker.cpp
WebKit/UncountedLocalVarsChecker.cpp
WebKit/RawPtrRefLocalVarsChecker.cpp

LINK_LIBS
clangAST
Original file line number Diff line number Diff line change
@@ -200,6 +200,14 @@ std::optional<bool> isUncountedPtr(const QualType T) {
return false;
}

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

std::optional<bool> isUnsafePtr(const QualType T) {
if (T->isPointerType() || T->isReferenceType()) {
if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
4 changes: 4 additions & 0 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
Original file line number Diff line number Diff line change
@@ -63,6 +63,10 @@ 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 \p T is either a raw pointer or reference to an unchecked
/// class, false if not, std::nullopt if inconclusive.
std::optional<bool> isUncheckedPtr(const clang::QualType T);

/// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
/// variant, false if not.
bool isSafePtrType(const clang::QualType T);
Original file line number Diff line number Diff line change
@@ -165,15 +165,18 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
return false;
}

class UncountedLocalVarsChecker
class RawPtrRefLocalVarsChecker
: public Checker<check::ASTDecl<TranslationUnitDecl>> {
BugType Bug{this,
"Uncounted raw pointer or reference not provably backed by "
"ref-counted variable",
"WebKit coding guidelines"};
BugType Bug;
mutable BugReporter *BR;

public:
RawPtrRefLocalVarsChecker(const char *description)
: Bug(this, description, "WebKit coding guidelines") {}

virtual std::optional<bool> isUnsafePtr(const QualType T) const = 0;
virtual const char *ptrKind() const = 0;

void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
BugReporter &BRArg) const {
BR = &BRArg;
@@ -182,14 +185,14 @@ class UncountedLocalVarsChecker
// visit template instantiations or lambda classes. We
// want to visit those, so we make our own RecursiveASTVisitor.
struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
const UncountedLocalVarsChecker *Checker;
const RawPtrRefLocalVarsChecker *Checker;
Decl *DeclWithIssue{nullptr};

TrivialFunctionAnalysis TFA;

using Base = RecursiveASTVisitor<LocalVisitor>;

explicit LocalVisitor(const UncountedLocalVarsChecker *Checker)
explicit LocalVisitor(const RawPtrRefLocalVarsChecker *Checker)
: Checker(Checker) {
assert(Checker);
}
@@ -261,7 +264,7 @@ class UncountedLocalVarsChecker
if (shouldSkipVarDecl(V))
return;

std::optional<bool> IsUncountedPtr = isUncountedPtr(V->getType());
std::optional<bool> IsUncountedPtr = isUnsafePtr(V->getType());
if (IsUncountedPtr && *IsUncountedPtr) {
if (tryToFindPtrOrigin(
Value, /*StopAtFirstRefCountedObj=*/false,
@@ -324,7 +327,7 @@ class UncountedLocalVarsChecker
llvm::raw_svector_ostream Os(Buf);

if (dyn_cast<ParmVarDecl>(V)) {
Os << "Assignment to an uncounted parameter ";
Os << "Assignment to an " << ptrKind() << " parameter ";
printQuotedQualifiedName(Os, V);
Os << " is unsafe.";

@@ -342,7 +345,7 @@ class UncountedLocalVarsChecker
else
Os << "Variable ";
printQuotedQualifiedName(Os, V);
Os << " is uncounted and unsafe.";
Os << " is " << ptrKind() << " and unsafe.";

PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
@@ -352,6 +355,29 @@ class UncountedLocalVarsChecker
}
}
};

class UncountedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
public:
UncountedLocalVarsChecker()
: RawPtrRefLocalVarsChecker("Uncounted raw pointer or reference not "
"provably backed by ref-counted variable") {}
std::optional<bool> isUnsafePtr(const QualType T) const final {
return isUncountedPtr(T);
}
const char *ptrKind() const final { return "uncounted"; }
};

class UncheckedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
public:
UncheckedLocalVarsChecker()
: RawPtrRefLocalVarsChecker("Unchecked raw pointer or reference not "
"provably backed by checked variable") {}
std::optional<bool> isUnsafePtr(const QualType T) const final {
return isUncheckedPtr(T);
}
const char *ptrKind() const final { return "unchecked"; }
};

} // namespace

void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
@@ -361,3 +387,11 @@ void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
bool ento::shouldRegisterUncountedLocalVarsChecker(const CheckerManager &) {
return true;
}

void ento::registerUncheckedLocalVarsChecker(CheckerManager &Mgr) {
Mgr.registerChecker<UncheckedLocalVarsChecker>();
}

bool ento::shouldRegisterUncheckedLocalVarsChecker(const CheckerManager &) {
return true;
}
2 changes: 2 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/mock-types.h
Original file line number Diff line number Diff line change
@@ -186,6 +186,8 @@ class CheckedObj {
public:
void incrementPtrCount();
void decrementPtrCount();
void method();
int trivial() { return 123; }
};

class RefCountableAndCheckable {
342 changes: 342 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/unchecked-local-vars.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,342 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncheckedLocalVarsChecker -verify %s

#include "mock-types.h"
#include "mock-system-header.h"

void someFunction();

namespace raw_ptr {
void foo() {
CheckedObj *bar;
// FIXME: later on we might warn on uninitialized vars too
}

void bar(CheckedObj *) {}
} // namespace raw_ptr

namespace reference {
void foo_ref() {
CheckedObj automatic;
CheckedObj &bar = automatic;
// expected-warning@-1{{Local variable 'bar' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
someFunction();
bar.method();
}

void foo_ref_trivial() {
CheckedObj automatic;
CheckedObj &bar = automatic;
}

void bar_ref(CheckedObj &) {}
} // namespace reference

namespace guardian_scopes {
void foo1() {
CheckedPtr<CheckedObj> foo;
{ CheckedObj *bar = foo.get(); }
}

void foo2() {
CheckedPtr<CheckedObj> foo;
// missing embedded scope here
CheckedObj *bar = foo.get();
// expected-warning@-1{{Local variable 'bar' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
someFunction();
bar->method();
}

void foo3() {
CheckedPtr<CheckedObj> foo;
{
{ CheckedObj *bar = foo.get(); }
}
}

void foo4() {
{
CheckedPtr<CheckedObj> foo;
{ CheckedObj *bar = foo.get(); }
}
}

void foo5() {
CheckedPtr<CheckedObj> foo;
auto* bar = foo.get();
bar->trivial();
}

void foo6() {
CheckedPtr<CheckedObj> foo;
auto* bar = foo.get();
// expected-warning@-1{{Local variable 'bar' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
bar->method();
}

struct SelfReferencingStruct {
SelfReferencingStruct* ptr;
CheckedObj* obj { nullptr };
};

void foo7(CheckedObj* obj) {
SelfReferencingStruct bar = { &bar, obj };
bar.obj->method();
}

} // namespace guardian_scopes

namespace auto_keyword {
class Foo {
CheckedObj *provide_ref_ctnbl();

void evil_func() {
CheckedObj *bar = provide_ref_ctnbl();
// expected-warning@-1{{Local variable 'bar' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
auto *baz = provide_ref_ctnbl();
// expected-warning@-1{{Local variable 'baz' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
auto *baz2 = this->provide_ref_ctnbl();
// expected-warning@-1{{Local variable 'baz2' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
[[clang::suppress]] auto *baz_suppressed = provide_ref_ctnbl(); // no-warning
}

void func() {
CheckedObj *bar = provide_ref_ctnbl();
// expected-warning@-1{{Local variable 'bar' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
if (bar)
bar->method();
}
};
} // namespace auto_keyword

namespace guardian_casts {
void foo1() {
CheckedPtr<CheckedObj> foo;
{
CheckedObj *bar = downcast<CheckedObj>(foo.get());
bar->method();
}
foo->method();
}

void foo2() {
CheckedPtr<CheckedObj> foo;
{
CheckedObj *bar =
static_cast<CheckedObj *>(downcast<CheckedObj>(foo.get()));
someFunction();
}
}
} // namespace guardian_casts

namespace guardian_ref_conversion_operator {
void foo() {
CheckedRef<CheckedObj> rc;
{
CheckedObj &rr = rc;
rr.method();
someFunction();
}
}
} // namespace guardian_ref_conversion_operator

namespace ignore_for_if {
CheckedObj *provide_ref_ctnbl() { return nullptr; }

void foo() {
// no warnings
if (CheckedObj *a = provide_ref_ctnbl())
a->trivial();
for (CheckedObj *b = provide_ref_ctnbl(); b != nullptr;)
b->trivial();
CheckedObj *array[1];
for (CheckedObj *c : array)
c->trivial();
while (CheckedObj *d = provide_ref_ctnbl())
d->trivial();
do {
CheckedObj *e = provide_ref_ctnbl();
e->trivial();
} while (1);
someFunction();
}

void bar() {
if (CheckedObj *a = provide_ref_ctnbl()) {
// expected-warning@-1{{Local variable 'a' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
a->method();
}
for (CheckedObj *b = provide_ref_ctnbl(); b != nullptr;) {
// expected-warning@-1{{Local variable 'b' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
b->method();
}
CheckedObj *array[1];
for (CheckedObj *c : array) {
// expected-warning@-1{{Local variable 'c' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
c->method();
}

while (CheckedObj *d = provide_ref_ctnbl()) {
// expected-warning@-1{{Local variable 'd' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
d->method();
}
do {
CheckedObj *e = provide_ref_ctnbl();
// expected-warning@-1{{Local variable 'e' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
e->method();
} while (1);
someFunction();
}

} // namespace ignore_for_if

namespace ignore_system_headers {

CheckedObj *provide_checkable();

void system_header() {
localVar<CheckedObj>(provide_checkable);
}

} // ignore_system_headers

namespace conditional_op {
CheckedObj *provide_checkable();
bool bar();

void foo() {
CheckedObj *a = bar() ? nullptr : provide_checkable();
// expected-warning@-1{{Local variable 'a' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
CheckedPtr<CheckedObj> b = provide_checkable();
{
CheckedObj* c = bar() ? nullptr : b.get();
c->method();
CheckedObj* d = bar() ? b.get() : nullptr;
d->method();
}
}

} // namespace conditional_op

namespace local_assignment_basic {

CheckedObj *provide_checkable();

void foo(CheckedObj* a) {
CheckedObj* b = a;
// expected-warning@-1{{Local variable 'b' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
if (b->trivial())
b = provide_checkable();
}

void bar(CheckedObj* a) {
CheckedObj* b;
// expected-warning@-1{{Local variable 'b' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
b = provide_checkable();
}

void baz() {
CheckedPtr a = provide_checkable();
{
CheckedObj* b = a.get();
// expected-warning@-1{{Local variable 'b' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
b = provide_checkable();
}
}

} // namespace local_assignment_basic

namespace local_assignment_to_parameter {

CheckedObj *provide_checkable();
void someFunction();

void foo(CheckedObj* a) {
a = provide_checkable();
// expected-warning@-1{{Assignment to an unchecked parameter 'a' is unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
someFunction();
a->method();
}

} // namespace local_assignment_to_parameter

namespace local_assignment_to_static_local {

CheckedObj *provide_checkable();
void someFunction();

void foo() {
static CheckedObj* a = nullptr;
// expected-warning@-1{{Static local variable 'a' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
a = provide_checkable();
someFunction();
a->method();
}

} // namespace local_assignment_to_static_local

namespace local_assignment_to_global {

CheckedObj *provide_ref_cntbl();
void someFunction();

CheckedObj* g_a = nullptr;
// expected-warning@-1{{Global variable 'local_assignment_to_global::g_a' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}

void foo() {
g_a = provide_ref_cntbl();
someFunction();
g_a->method();
}

} // 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 unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
a = provide_obj();
a->method();
}

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

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

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

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

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

} // namespace local_refcountable_checkable_object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect a similar warning for assignments to struct members or class fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, struct members and class fields are checked by NoUncountedMemberChecker / NoUncheckedPtrMemberChecker.

Original file line number Diff line number Diff line change
@@ -144,7 +144,7 @@ static_library("Checkers") {
"WebKit/RefCntblBaseVirtualDtorChecker.cpp",
"WebKit/UncountedCallArgsChecker.cpp",
"WebKit/UncountedLambdaCapturesChecker.cpp",
"WebKit/UncountedLocalVarsChecker.cpp",
"WebKit/RawPtrRefLocalVarsChecker.cpp",
"cert/InvalidPtrChecker.cpp",
]
}
Loading