Skip to content

[alpha.webkit.UnretainedLambdaCapturesChecker] Add the support for protectedSelf #132518

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
Apr 9, 2025
Merged

[alpha.webkit.UnretainedLambdaCapturesChecker] Add the support for protectedSelf #132518

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
05fd2f7
[alpha.webkit.UnretainedLambdaCapturesChecker] Add the support for pr…
rniwa Mar 21, 2025
9893ede
Add a test case for capturing self by value.
rniwa Apr 9, 2025
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
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
bool isCtorOfRetainPtr(const clang::FunctionDecl *F) {
const std::string &FunctionName = safeGetName(F);
return FunctionName == "RetainPtr" || FunctionName == "adoptNS" ||
FunctionName == "adoptCF";
FunctionName == "adoptCF" || FunctionName == "retainPtr";
}

bool isCtorOfSafePtr(const clang::FunctionDecl *F) {
Expand Down
61 changes: 50 additions & 11 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class RawPtrRefLambdaCapturesChecker
: Bug(this, description, "WebKit coding guidelines") {}

virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
virtual bool isPtrType(const std::string &) const = 0;
virtual const char *ptrKind(QualType QT) const = 0;

void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
Expand Down Expand Up @@ -68,6 +69,15 @@ class RawPtrRefLambdaCapturesChecker
return DynamicRecursiveASTVisitor::TraverseCXXMethodDecl(CXXMD);
}

bool TraverseObjCMethodDecl(ObjCMethodDecl *OCMD) override {
llvm::SaveAndRestore SavedDecl(ClsType);
if (OCMD && OCMD->isInstanceMethod()) {
if (auto *ImplParamDecl = OCMD->getSelfDecl())
ClsType = ImplParamDecl->getType();
}
return DynamicRecursiveASTVisitor::TraverseObjCMethodDecl(OCMD);
}

bool VisitTypedefDecl(TypedefDecl *TD) override {
if (Checker->RTC)
Checker->RTC->visitTypedef(TD);
Expand Down Expand Up @@ -275,10 +285,10 @@ class RawPtrRefLambdaCapturesChecker
auto *VD = dyn_cast<VarDecl>(ValueDecl);
if (!VD)
return false;
auto *Init = VD->getInit()->IgnoreParenCasts();
auto *Init = VD->getInit();
if (!Init)
return false;
const Expr *Arg = Init;
const Expr *Arg = Init->IgnoreParenCasts();
do {
if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
Arg = BTE->getSubExpr()->IgnoreParenCasts();
Expand All @@ -287,7 +297,7 @@ class RawPtrRefLambdaCapturesChecker
if (!Ctor)
return false;
auto clsName = safeGetName(Ctor->getParent());
if (isRefType(clsName) && CE->getNumArgs()) {
if (Checker->isPtrType(clsName) && CE->getNumArgs()) {
Arg = CE->getArg(0)->IgnoreParenCasts();
continue;
}
Expand All @@ -307,6 +317,12 @@ class RawPtrRefLambdaCapturesChecker
Arg = CE->getArg(0)->IgnoreParenCasts();
continue;
}
if (auto *Callee = CE->getDirectCallee()) {
if (isCtorOfSafePtr(Callee) && CE->getNumArgs() == 1) {
Arg = CE->getArg(0)->IgnoreParenCasts();
continue;
}
}
}
if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
auto OpCode = OpCE->getOperator();
Expand All @@ -315,7 +331,7 @@ class RawPtrRefLambdaCapturesChecker
if (!Callee)
return false;
auto clsName = safeGetName(Callee->getParent());
if (!isRefType(clsName) || !OpCE->getNumArgs())
if (!Checker->isPtrType(clsName) || !OpCE->getNumArgs())
return false;
Arg = OpCE->getArg(0)->IgnoreParenCasts();
continue;
Expand All @@ -330,8 +346,15 @@ class RawPtrRefLambdaCapturesChecker
}
break;
} while (Arg);
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
return ProtectedThisDecls.contains(DRE->getDecl());
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
auto *Decl = DRE->getDecl();
if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(Decl)) {
auto kind = ImplicitParam->getParameterKind();
return kind == ImplicitParamKind::ObjCSelf ||
kind == ImplicitParamKind::CXXThis;
}
return ProtectedThisDecls.contains(Decl);
}
return isa<CXXThisExpr>(Arg);
}
};
Expand All @@ -351,10 +374,17 @@ class RawPtrRefLambdaCapturesChecker
ValueDecl *CapturedVar = C.getCapturedVar();
if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
continue;
if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(CapturedVar)) {
auto kind = ImplicitParam->getParameterKind();
if ((kind == ImplicitParamKind::ObjCSelf ||
kind == ImplicitParamKind::CXXThis) &&
!shouldCheckThis)
continue;
}
QualType CapturedVarQualType = CapturedVar->getType();
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
if (IsUncountedPtr && *IsUncountedPtr)
reportBug(C, CapturedVar, CapturedVarQualType);
reportBug(C, CapturedVar, CapturedVarQualType, L);
} else if (C.capturesThis() && shouldCheckThis) {
if (ignoreParamVarDecl) // this is always a parameter to this function.
continue;
Expand All @@ -364,11 +394,12 @@ class RawPtrRefLambdaCapturesChecker
}

void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
const QualType T) const {
const QualType T, LambdaExpr *L) const {
assert(CapturedVar);

if (isa<ImplicitParamDecl>(CapturedVar) && !Capture.getLocation().isValid())
return; // Ignore implicit captruing of self.
auto Location = Capture.getLocation();
if (isa<ImplicitParamDecl>(CapturedVar) && !Location.isValid())
Location = L->getBeginLoc();

SmallString<100> Buf;
llvm::raw_svector_ostream Os(Buf);
Expand All @@ -387,7 +418,7 @@ class RawPtrRefLambdaCapturesChecker
printQuotedQualifiedName(Os, CapturedVar);
Os << " to " << ptrKind(T) << " type is unsafe.";

PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
PathDiagnosticLocation BSLoc(Location, BR->getSourceManager());
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
BR->emitReport(std::move(Report));
}
Expand Down Expand Up @@ -429,6 +460,10 @@ class UncountedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
return result2;
}

virtual bool isPtrType(const std::string &Name) const final {
return isRefType(Name) || isCheckedPtr(Name);
}

const char *ptrKind(QualType QT) const final {
if (isUncounted(QT))
return "uncounted";
Expand All @@ -448,6 +483,10 @@ class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
return RTC->isUnretained(QT);
}

virtual bool isPtrType(const std::string &Name) const final {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any actual callers to this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, line 290 / 300 and line 318 / 334.

return isRetainPtr(Name);
}

const char *ptrKind(QualType QT) const final { return "unretained"; }
};

Expand Down
22 changes: 21 additions & 1 deletion clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures-arc.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ @interface ObjWithSelf : NSObject {
RetainPtr<id> delegate;
}
-(void)doWork;
-(void)doMoreWork;
-(void)run;
@end

Expand All @@ -265,9 +266,28 @@ -(void)doWork {
someFunction();
[delegate doWork];
};
auto doMoreWork = [=] {
someFunction();
[delegate doWork];
};
auto doExtraWork = [&, protectedSelf = retainPtr(self)] {
someFunction();
[delegate doWork];
};
callFunctionOpaque(doWork);
callFunctionOpaque(doMoreWork);
callFunctionOpaque(doExtraWork);
}

-(void)doMoreWork {
auto doWork = [self, protectedSelf = retainPtr(self)] {
someFunction();
[self doWork];
};
callFunctionOpaque(doWork);
}

-(void)run {
someFunction();
}
@end
@end
24 changes: 23 additions & 1 deletion clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,18 +279,40 @@ @interface ObjWithSelf : NSObject {
RetainPtr<id> delegate;
}
-(void)doWork;
-(void)doMoreWork;
-(void)run;
@end

@implementation ObjWithSelf
-(void)doWork {
auto doWork = [&] {
// expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also adda test for capture by value? auto doWork = [=]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

someFunction();
[delegate doWork];
};
auto doMoreWork = [=] {
// expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
someFunction();
[delegate doWork];
};
auto doExtraWork = [&, protectedSelf = retainPtr(self)] {
someFunction();
[delegate doWork];
};
callFunctionOpaque(doWork);
callFunctionOpaque(doMoreWork);
callFunctionOpaque(doExtraWork);
}

-(void)doMoreWork {
auto doWork = [self, protectedSelf = retainPtr(self)] {
someFunction();
[self doWork];
};
callFunctionOpaque(doWork);
}

-(void)run {
someFunction();
}
@end
@end