[clang][AST] Fix crash in MatchChildASTVisitor::TraverseLambdaExpr

[PiotrZSL]

When a lambda expression captures a VLA array by reference, the 'capture_init' array contains one element, which is 'nullptr'. While traversing the AST with the 'IgnoreUnlessSpelledInSource' flag, there is a dereference of this 'nullptr'.

This change introduces a verification step to check if 'capture_init' is 'nullptr' before attempting to dereference it. Additionally, it includes tests for matchers and for the clang-tidy check that originally revealed the issue.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-tidy

Author: Piotr Zegar (PiotrZSL)

Changes

When a lambda expression captures a VLA array by reference, the 'capture_init' array contains one element, which is 'nullptr'. While traversing the AST with the 'IgnoreUnlessSpelledInSource' flag, there is a dereference of this 'nullptr'.

This change introduces a verification step to check if 'capture_init' is 'nullptr' before attempting to dereference it. Additionally, it includes tests for matchers and for the clang-tidy check that originally revealed the issue.

---

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

3 Files Affected:

• (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp (+10)
• (modified) clang/lib/ASTMatchers/ASTMatchFinder.cpp (+2-1)
• (modified) clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp (+13)

diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp
index eb8ad1b8b879250..7ab5ecb9cfe3e2b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp
@@ -395,3 +395,13 @@ namespace PR63994 {
     // CHECK-NOTES: [[@LINE-1]]:5: warning: returning a newly created resource of type 'A *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'
   }
 }
+
+namespace PR70460 {
+  template<typename T>
+  void h(T&& x) { x(); }
+
+  void f(int N) {
+    int a[N];
+    h([&a]() {});
+  }
+}
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 0bac2ed63a927ef..08739b7079c1a82 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -290,7 +290,8 @@ class MatchChildASTVisitor
         continue;
       if (Node->isInitCapture(C) && !match(*C->getCapturedVar()))
         return false;
-      if (!match(*Node->capture_init_begin()[I]))
+      const Expr *CI = Node->capture_init_begin()[I];
+      if (CI && !match(*CI))
         return false;
     }
 
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index d4a695b974bf0e5..87fdc3da480f076 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -606,6 +606,19 @@ void foo()
                                   varDecl(hasName("lPtrDecay"))))))));
 }
 
+TEST(Matcher, NotCrashesOnLambdaThatCapturesVla) {
+  StringRef Code = R"cpp(
+void f(int N) {
+    int a[N];
+    [&a]() {};
+}
+)cpp";
+  EXPECT_FALSE(
+      matches(Code, traverse(TK_AsIs, lambdaExpr(hasDescendant(expr())))));
+  EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource,
+                                      lambdaExpr(hasDescendant(expr())))));
+}
+
 TEST(Matcher, MatchesCoroutine) {
   FileContentMappings M;
   M.push_back(std::make_pair("/coro_header", R"cpp(

[cor3ntin]

• @AaronBallman Having a nullptr init capture seems fishy, do we actually mean to allow capturing vla by reference here?

[AaronBallman]

• @AaronBallman Having a nullptr init capture seems fishy, do we actually mean to allow capturing vla by reference here?

That does seem fishy to me -- I would expect VLA to behave the same as a constant-size array in terms of what we capture. So I think we do want to capture the VLA by reference (that's how we behave for constant-sized arrays).

[PiotrZSL]

Would be nice to simply not crash in such case, whatever someone will fix origin issue later is up to debate.
I would like to see this change on 17.0.6 (that means 2 weeks left to merge this).
Unless someone fixes this somewere in lower layers.

[AaronBallman]

Would be nice to simply not crash in such case, whatever someone will fix origin issue later is up to debate. I would like to see this change on 17.0.6 (that means 2 weeks left to merge this). Unless someone fixes this somewere in lower layers.

The worry with pushing this as a fix is that it seems to be papering over a real issue; if we "fix" it this way, 1) less incentive to go fix the actual issue, and 2) easy to forget to come tear this code back out when we do fix it and that makes future maintenance harder.

I don't think this should change for 17.0.6 unless we have a correct fix that we agree is definitely safe and correct.

Unfortunately, I think we need to spend some time looking into the root cause more (not certain when I'll have the time to poke at that myself, but it would likely be in the next year timeframe for me at this point due to holidays coming up).

You can definitely see the NULL entry in the AST in https://godbolt.org/z/bax6Ghj3n but the code also seems to behave correctly and the assembly generated seems reasonable. I think the issue is likely near here:

llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h

Line 2657 in 108380d

for (unsigned I = 0, N = S->capture_size(); I != N; ++I) {