Skip to content

[webkit.UncountedLambdaCapturesChecker] Recognize nested protectedThis pattern #126443

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 6 commits into from
Feb 15, 2025
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
Original file line number Diff line number Diff line change
@@ -41,7 +41,9 @@ class UncountedLambdaCapturesChecker
const UncountedLambdaCapturesChecker *Checker;
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
llvm::DenseSet<const LambdaExpr *> LambdasToIgnore;
llvm::DenseSet<const ValueDecl *> ProtectedThisDecls;
llvm::DenseSet<const CXXConstructExpr *> ConstructToIgnore;

QualType ClsType;

explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
@@ -66,7 +68,7 @@ class UncountedLambdaCapturesChecker
bool VisitLambdaExpr(LambdaExpr *L) override {
if (LambdasToIgnore.contains(L))
return true;
Checker->visitLambdaExpr(L, shouldCheckThis());
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
return true;
}

@@ -94,7 +96,7 @@ class UncountedLambdaCapturesChecker
if (!L)
return true;
LambdasToIgnore.insert(L);
Checker->visitLambdaExpr(L, shouldCheckThis());
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
return true;
}

@@ -119,7 +121,8 @@ class UncountedLambdaCapturesChecker
if (auto *L = findLambdaInArg(Arg)) {
LambdasToIgnore.insert(L);
if (!Param->hasAttr<NoEscapeAttr>())
Checker->visitLambdaExpr(L, shouldCheckThis());
Checker->visitLambdaExpr(L, shouldCheckThis() &&
!hasProtectedThis(L));
}
++ArgIndex;
}
@@ -139,7 +142,8 @@ class UncountedLambdaCapturesChecker
if (auto *L = findLambdaInArg(Arg)) {
LambdasToIgnore.insert(L);
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
Checker->visitLambdaExpr(L, shouldCheckThis());
Checker->visitLambdaExpr(L, shouldCheckThis() &&
!hasProtectedThis(L));
}
++ArgIndex;
}
@@ -168,6 +172,13 @@ class UncountedLambdaCapturesChecker
ConstructToIgnore.insert(CE);
return Lambda;
}
if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
E = TempExpr->getSubExpr()->IgnoreParenCasts();
if (auto *Lambda = dyn_cast<LambdaExpr>(E)) {
ConstructToIgnore.insert(CE);
return Lambda;
}
}
auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
if (!DRE)
return nullptr;
@@ -213,9 +224,68 @@ class UncountedLambdaCapturesChecker
return;
DeclRefExprsToIgnore.insert(ArgRef);
LambdasToIgnore.insert(L);
Checker->visitLambdaExpr(L, shouldCheckThis(),
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
/* ignoreParamVarDecl */ true);
}

bool hasProtectedThis(LambdaExpr *L) {
for (const LambdaCapture &OtherCapture : L->captures()) {
if (!OtherCapture.capturesVariable())
continue;
if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
if (declProtectsThis(ValueDecl)) {
ProtectedThisDecls.insert(ValueDecl);
return true;
}
}
}
return false;
}

bool declProtectsThis(const ValueDecl *ValueDecl) const {
auto *VD = dyn_cast<VarDecl>(ValueDecl);
if (!VD)
return false;
auto *Init = VD->getInit()->IgnoreParenCasts();
if (!Init)
return false;
const Expr *Arg = Init;
do {
if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
Arg = BTE->getSubExpr()->IgnoreParenCasts();
if (auto *CE = dyn_cast_or_null<CXXConstructExpr>(Arg)) {
auto *Ctor = CE->getConstructor();
if (!Ctor)
return false;
auto clsName = safeGetName(Ctor->getParent());
if (!isRefType(clsName) || !CE->getNumArgs())
return false;
Arg = CE->getArg(0)->IgnoreParenCasts();
continue;
}
if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
auto OpCode = OpCE->getOperator();
if (OpCode == OO_Star || OpCode == OO_Amp) {
auto *Callee = OpCE->getDirectCallee();
auto clsName = safeGetName(Callee->getParent());
if (!isRefType(clsName) || !OpCE->getNumArgs())
return false;
Arg = OpCE->getArg(0)->IgnoreParenCasts();
continue;
}
}
if (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
auto OpCode = UO->getOpcode();
if (OpCode == UO_Deref || OpCode == UO_AddrOf)
Arg = UO->getSubExpr()->IgnoreParenCasts();
continue;
}
break;
} while (Arg);
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
return ProtectedThisDecls.contains(DRE->getDecl());
return isa<CXXThisExpr>(Arg);
}
};

LocalVisitor visitor(this);
@@ -238,53 +308,11 @@ class UncountedLambdaCapturesChecker
} else if (C.capturesThis() && shouldCheckThis) {
if (ignoreParamVarDecl) // this is always a parameter to this function.
continue;
bool hasProtectThis = false;
for (const LambdaCapture &OtherCapture : L->captures()) {
if (!OtherCapture.capturesVariable())
continue;
if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
if (protectThis(ValueDecl)) {
hasProtectThis = true;
break;
}
}
}
if (!hasProtectThis)
reportBugOnThisPtr(C);
reportBugOnThisPtr(C);
}
}
}

bool protectThis(const ValueDecl *ValueDecl) const {
auto *VD = dyn_cast<VarDecl>(ValueDecl);
if (!VD)
return false;
auto *Init = VD->getInit()->IgnoreParenCasts();
if (!Init)
return false;
auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Init);
if (!BTE)
return false;
auto *CE = dyn_cast_or_null<CXXConstructExpr>(BTE->getSubExpr());
if (!CE)
return false;
auto *Ctor = CE->getConstructor();
if (!Ctor)
return false;
auto clsName = safeGetName(Ctor->getParent());
if (!isRefType(clsName) || !CE->getNumArgs())
return false;
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
while (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
auto OpCode = UO->getOpcode();
if (OpCode == UO_Deref || OpCode == UO_AddrOf)
Arg = UO->getSubExpr();
else
break;
}
return isa<CXXThisExpr>(Arg);
}

void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
const QualType T) const {
assert(CapturedVar);
32 changes: 32 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
Original file line number Diff line number Diff line change
@@ -89,6 +89,7 @@ template <typename Callback> void call(Callback callback) {
someFunction();
callback();
}
void callAsync(const WTF::Function<void()>&);

void raw_ptr() {
RefCountable* ref_countable = make_obj();
@@ -303,6 +304,37 @@ struct RefCountableWithLambdaCapturingThis {
});
}

void callAsyncNoescape([[clang::noescape]] WTF::Function<bool(RefCountable&)>&&);
void method_temp_lambda(RefCountable* obj) {
callAsyncNoescape([this, otherObj = RefPtr { obj }](auto& obj) {
return otherObj == &obj;
});
}

void method_nested_lambda() {
callAsync([this, protectedThis = Ref { *this }] {
callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(protectedThis)] {
nonTrivial();
});
});
}

void method_nested_lambda2() {
callAsync([this, protectedThis = RefPtr { this }] {
callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(*protectedThis)] {
nonTrivial();
});
});
}
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 need a test for a lambda capture with a this and an unprotected this?

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, we have such a test case. See method_captures_this_unsafe around line 217 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can add a test case for nested lambdas though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test case.


void method_nested_lambda3() {
callAsync([this, protectedThis = RefPtr { this }] {
callAsync([this] {
// expected-warning@-1{{Captured raw-pointer 'this' to ref-counted type or CheckedPtr-capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
nonTrivial();
});
});
}
};

struct NonRefCountableWithLambdaCapturingThis {