-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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()) | ||
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())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
@@ -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; | ||
|
||
|
@@ -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; | ||
|
@@ -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 | ||
|
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.