Skip to content

[alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. #92639

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.

Sign up for GitHub

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 2 commits into from
May 24, 2024
Merged

[alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. #92639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ae3b19
[alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncoun…
May 18, 2024
fa69297
Add tests for assigning to a static local variable and a global varia…
May 24, 2024
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
68 changes: 44 additions & 24 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,19 @@ class UncountedLocalVarsChecker
bool shouldVisitImplicitCode() const { return false; }

bool VisitVarDecl(VarDecl *V) {
Checker->visitVarDecl(V);
auto *Init = V->getInit();
if (Init && V->isLocalVarDecl())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method says "yes" to static locals. Are we ok with that?

(These methods are super confusing so I'd rather double-check.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, interesting. I think we do want to check static local variables as well since there is nothing preventing static local variable to be a raw pointer to a ref counted type, which is also bad.

Checker->visitVarDecl(V, Init);
return true;
}

bool VisitBinaryOperator(const BinaryOperator *BO) {
if (BO->isAssignmentOp()) {
if (auto *VarRef = dyn_cast<DeclRefExpr>(BO->getLHS())) {
if (auto *V = dyn_cast<VarDecl>(VarRef->getDecl()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you also like to skip globals here? (But you probably don't want to skip parameters here – only in the initializer case.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... I don't think so. I think any assignment to a global variable is also bad if it's a raw pointer to a ref counted type. It's generally an anti-pattern to avoid.

This makes me think that the checker should probably be renamed to "UncountedVarDeclChecker" or something.

Checker->visitVarDecl(V, BO->getRHS());
}
}
return true;
}

Expand Down Expand Up @@ -174,7 +186,7 @@ class UncountedLocalVarsChecker
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
}

void visitVarDecl(const VarDecl *V) const {
void visitVarDecl(const VarDecl *V, const Expr *Value) const {
if (shouldSkipVarDecl(V))
return;

Expand All @@ -184,12 +196,8 @@ class UncountedLocalVarsChecker

std::optional<bool> IsUncountedPtr = isUncountedPtr(ArgType);
if (IsUncountedPtr && *IsUncountedPtr) {
const Expr *const InitExpr = V->getInit();
if (!InitExpr)
return; // FIXME: later on we might warn on uninitialized vars too

if (tryToFindPtrOrigin(
InitExpr, /*StopAtFirstRefCountedObj=*/false,
Value, /*StopAtFirstRefCountedObj=*/false,
[&](const clang::Expr *InitArgOrigin, bool IsSafe) {
if (!InitArgOrigin)
return true;
Expand Down Expand Up @@ -232,34 +240,46 @@ class UncountedLocalVarsChecker
}))
return;

reportBug(V);
reportBug(V, Value);
}
}

bool shouldSkipVarDecl(const VarDecl *V) const {
assert(V);
if (!V->isLocalVarDecl())
return true;

if (BR->getSourceManager().isInSystemHeader(V->getLocation()))
return true;

return false;
return BR->getSourceManager().isInSystemHeader(V->getLocation());
}

void reportBug(const VarDecl *V) const {
void reportBug(const VarDecl *V, const Expr *Value) const {
assert(V);
SmallString<100> Buf;
llvm::raw_svector_ostream Os(Buf);

Os << "Local variable ";
printQuotedQualifiedName(Os, V);
Os << " is uncounted and unsafe.";

PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
Report->addRange(V->getSourceRange());
BR->emitReport(std::move(Report));
if (dyn_cast<ParmVarDecl>(V)) {
Os << "Assignment to an uncounted parameter ";
printQuotedQualifiedName(Os, V);
Os << " is unsafe.";

PathDiagnosticLocation BSLoc(Value->getExprLoc(), BR->getSourceManager());
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
Report->addRange(Value->getSourceRange());
BR->emitReport(std::move(Report));
} else {
if (V->hasLocalStorage())
Os << "Local variable ";
else if (V->isStaticLocal())
Os << "Static local variable ";
else if (V->hasGlobalStorage())
Os << "Global variable ";
else
Os << "Variable ";
printQuotedQualifiedName(Os, V);
Os << " is uncounted and unsafe.";

PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
Report->addRange(V->getSourceRange());
BR->emitReport(std::move(Report));
}
}
};
} // namespace
Expand Down
73 changes: 73 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,3 +216,76 @@ void foo() {
}

} // namespace conditional_op

namespace local_assignment_basic {

RefCountable *provide_ref_cntbl();

void foo(RefCountable* a) {
RefCountable* b = a;
// expected-warning@-1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
if (b->trivial())
b = provide_ref_cntbl();
}

void bar(RefCountable* a) {
RefCountable* b;
// expected-warning@-1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
b = provide_ref_cntbl();
}

void baz() {
RefPtr a = provide_ref_cntbl();
{
RefCountable* b = a.get();
// expected-warning@-1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
b = provide_ref_cntbl();
}
}

} // namespace local_assignment_basic

namespace local_assignment_to_parameter {

RefCountable *provide_ref_cntbl();
void someFunction();

void foo(RefCountable* a) {
a = provide_ref_cntbl();
// expected-warning@-1{{Assignment to an uncounted parameter 'a' is unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
someFunction();
a->method();
}

} // namespace local_assignment_to_parameter

namespace local_assignment_to_static_local {

RefCountable *provide_ref_cntbl();
void someFunction();

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

} // namespace local_assignment_to_static_local

namespace local_assignment_to_global {

RefCountable *provide_ref_cntbl();
void someFunction();

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

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

} // namespace local_assignment_to_global
Loading