Skip to content

[alpha.webkit.UncheckedCallArgsChecker] Don't emit a warning for passing a temporary object as an argument. #155033

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 1 commit into from
Sep 8, 2025
Merged

[alpha.webkit.UncheckedCallArgsChecker] Don't emit a warning for passing a temporary object as an argument. #155033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 0 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,8 @@ bool isASafeCallArg(const Expr *E) {
return true;
}
}
if (isa<CXXTemporaryObjectExpr>(E))
return true; // A temporary lives until the end of this statement.
Copy link
Collaborator

@haoNoQ haoNoQ Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually no, it could be lifetime extended. Moreover, since C++17 due to mandatory copy elision it can end up absolutely anywhere. Stack, heap, global, an unknown location into which the function 12 levels above you in the call stack returns, you name it. For example: https://godbolt.org/z/EnvdsWK8G - notice how there are no copy/move-constructors anywhere in that AST. The CXXTemporaryObjectExpr (a sub-class of CXXConstructExpr btw) constructs the object directly at its final destination, no matter how far apart they are in the AST.

The annoying part is that you can't tell that by looking at the construct-expression itself. Normally for a method call the this argument is inside the call expression but for constructors it's somewhere in the parent expression and it could be one of like 30+ different rigid AST variations that you need to take into account, often with an indeterminate amount of irrelevant parent statement layers you need to peel one by one. (Eg. there could be an arbitrary amount of nested ?: operators.) And I'm afraid you can't really tell how long the object lives until you pattern-match the specific AST pattern to figure out the construction target.

Frontend solves it by simply being recursive - so it always has the construction target somewhere up its visitor stack. Maybe that approach would work for you too? You could use the parent map too but that could lead you to some really weird places. For our less recursive analysis machines I've tried to learn how to pattern-match some of these cases in https://clang.llvm.org/doxygen/classclang_1_1ConstructionContext.html but I wasn't wise enough to make it available over pure AST. And it's a very incomplete list anyway.

So the remaining question is, is there a specific way to quickly notice the temporaries that aren't lifetime-extended and ignore the other cases? Basically what you're looking for is the nearest parent MaterializeTemporaryExpr - and then you can ask it whether its getStorageDuration() is SD_FullExpression. The annoying part is that MaterializeTemporaryExpr doesn't have to be there at all - consider C(14, 15) and C(16, 17) in the godbolt example. In this case if you keep using the parent map it may lead you to weird places. So I think your best bet may be to either find an somewhat immediate MaterializeTemporaryExpr or give up and treat the constructor as unsafe. That'd probably handle your test case but it'd fail if doWorkWithObject() accepted it by value. For that you'd need to handle the "passed directly into a CallExpr" case too. But if you don't find either of those almost immediately (either directly above you, or additionally bypassing CXXBindTemporaryExpr which shows up iff the class has a non-trivial destructor IIRC) then it might be better to give up and treat the constructor is unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no, it could be lifetime extended. Moreover, since C++17 due to mandatory copy elision it can end up absolutely anywhere. Stack, heap, global, an unknown location into which the function 12 levels above you in the call stack returns, you name it.

Oh sure, but here what we need is the temporary to at least out-live the end of current statement since what we want to guarantee is that each function argument outlives the function call. It's okay if the temporary's lifetime is extended beyond that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we can revise the comment to say it "lives at least until the end of this statement"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oooooooooh. Right. You already know it's an argument. Yeah, case closed, the codegen-style recursive approach wins again.

And it looks like you've correctly skipped the MaterializeTemporaryExpr which is probably there in your test because the argument is accepted by reference. There may also be a CXXBindTemporaryExpr if the class has a destructor but it's probably peeled correctly for the same reason.

You folks are really good at this 😅

if (isConstOwnerPtrMemberExpr(E))
return true;

Expand Down
5 changes: 5 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/unchecked-call-arg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,8 @@ void foo() {
provide()->doWork();
// expected-warning@-1{{Call argument for 'this' parameter is unchecked and unsafe}}
}

void doWorkWithObject(const CheckedObject&);
void bar() {
doWorkWithObject(CheckedObject());
}