Skip to content
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

[clang] Fix null dereference on return in lambda attribute statement expr #66643

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

pfusik
Copy link
Contributor

@pfusik pfusik commented Sep 18, 2023

Fixes #48527

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 18, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 18, 2023

@llvm/pr-subscribers-clang

Changes

Fixes #48527

Full diff: https://github.com/llvm/llvm-project/pull/66643.diff

2 Files Affected:

  • (modified) clang/lib/Sema/SemaStmt.cpp (+2)
  • (added) clang/test/SemaCXX/gh48527.cpp (+10)
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 7cc509542d5381d..10adfbc406dfbb5 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -3577,6 +3577,8 @@ StmtResult Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc,
   CapturingScopeInfo *CurCap = cast<CapturingScopeInfo>(getCurFunction());
   QualType FnRetType = CurCap->ReturnType;
   LambdaScopeInfo *CurLambda = dyn_cast<LambdaScopeInfo>(CurCap);
+  if (CurLambda && CurLambda->CallOperator->getType().isNull())
+    return StmtError();
   bool HasDeducedReturnType =
       CurLambda && hasDeducedReturnType(CurLambda->CallOperator);
 
diff --git a/clang/test/SemaCXX/gh48527.cpp b/clang/test/SemaCXX/gh48527.cpp
new file mode 100644
index 000000000000000..420c35be37f5191
--- /dev/null
+++ b/clang/test/SemaCXX/gh48527.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int main() { // expected-note {{to match this '{'}}
+    auto a = [](void)__attribute__((b(({ // expected-note {{to match this '('}}
+    return 0;
+} // expected-error 3 {{expected ')'}} \
+  // expected-error {{expected ';' at end of declaration}}
+// expected-error@+2 {{expected ')'}}
+// expected-error@+1 {{expected body of lambda expression}}
+// expected-error {{expected '}'}}

Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

The change needs a release note, otherwise looks good.
Right, @cor3ntin ?

@cor3ntin
Copy link
Contributor

@Fznamznon I think so too, although I'd be inclined to move the test to clang/test/Parser/ because even though the fix is in Sema, it's testing a wrong parse.

The commit message also could do with a longer description.

Otherwise LGTM

@pfusik
Copy link
Contributor Author

pfusik commented Sep 18, 2023

I'd be inclined to move the test to clang/test/Parser/ because even though the fix is in Sema, it's testing a wrong parse.

It's an ICE in Sema. The test is identical to the bug report, but it doesn't mean the issue is limited to syntax errors.

int main() {
    []()__attribute__((b(({ return 0; })))){};
}

g++ accepts the above while Clang crashes. I'll add this as a second test.

@cor3ntin
Copy link
Contributor

I would still move the original test, things in sema usually at least parse (ie, this has unbalanced parens). I won't insist though

@pfusik
Copy link
Contributor Author

pfusik commented Sep 18, 2023

@cor3ntin to test/CXX/what or test/Parser ?

@cor3ntin
Copy link
Contributor

test/Parser. CXX is for tests that map very directly to a paragraph in the standard

int main() {
auto a = []()__attribute__((b(({ return 0; })))){}; // expected-warning {{unknown attribute 'b' ignored}}
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can move to clang/test/SemaCXX/lambda-expressions.cpp
(the name of the function can have the number of the GH issue)

We try to limit the number of test files

@pfusik pfusik force-pushed the gh48527 branch 2 times, most recently from 81281e5 to b84e2a8 Compare September 18, 2023 17:56
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@pfusik
Copy link
Contributor Author

pfusik commented Sep 18, 2023

Rebased resolving the conflict in release notes.

@shafik
Copy link
Collaborator

shafik commented Sep 18, 2023

LGTM

…expr

clang was crashing on a lambda attribute with a statement expression
that contained a `return`.
It attempted to access the lambda type which was unknown at that point.

Fixes llvm#48527
@pfusik pfusik merged commit c724ac9 into llvm:main Sep 19, 2023
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
…expr (llvm#66643)

clang was crashing on a lambda attribute with a statement expression
that contained a `return`.
It attempted to access the lambda type which was unknown at that point.

Fixes llvm#48527
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang++ segment fault in clang::Sema::ActOnCapScopeReturnStmt
5 participants