[libclang]: check validity before visiting Stmt node

[milianw]

Summary

Fixes crash when visiting a partial AST after encountering
a parse error in the input code:

#5  clang::Stmt::getStmtClass (this=<optimized out>) at llvm-project/clang/include/clang/AST/Stmt.h:1125
#6  clang::cxcursor::MakeCXCursor (S=S@entry=0x0, Parent=0x0, TU=0x7f8c6c5186b0, RegionOfInterest=...) at llvm-project/clang/tools/libclang/CXCursor.cpp:132
#7  0x00007f8caddc96e5 in clang::cxcursor::CursorVisitor::EnqueueWorkList (this=this@entry=0x7f8c9bffc330, WL=..., S=S@entry=0x0) at llvm-project/clang/tools/libclang/CIndex.cpp:3028
#8  0x00007f8cadde4e2c in clang::cxcursor::CursorVisitor::Visit (this=this@entry=0x7f8c9bffc330, S=0x0) at llvm-project/clang/tools/libclang/CIndex.cpp:3256
#9  0x00007f8cadde4604 in clang::cxcursor::CursorVisitor::RunVisitorWorkList (this=this@entry=0x7f8c9bffc330, WL=...) at llvm-project/clang/tools/libclang/CIndex.cpp:3214
#10 0x00007f8cadde4e37 in clang::cxcursor::CursorVisitor::Visit (this=this@entry=0x7f8c9bffc330, S=0x7f8c6c17dc30) at llvm-project/clang/tools/libclang/CIndex.cpp:3257
#11 0x00007f8cadddeccb in clang::cxcursor::CursorVisitor::VisitChildren (this=this@entry=0x7f8c9bffc330, Cursor=...) at llvm-project/clang/tools/libclang/CIndex.cpp:515
#12 0x00007f8cadde688f in clang_visitChildren (parent=..., visitor=0x7f8caef6ddda <(anonymous namespace)::visitCursor(CXCursor, CXCursor, CXClientData)>, client_data=0x7f8c9bffe3b0) at llvm-project/clang/tools/libclang/CIndex.cpp:4450

[milianw]

I'm not sure how to write a unit test for this, but I ran into a reproducible crash with a complex C++ file which got fixed by this patch

[jkorous]

@milianw could you try to reduce the reproducer you have? Maybe take lldb and see where does the nullptr come from?

[jkorous]

Your patch definitely fixes the symptoms of the bug. I just want to make sure that we aren't covering some logic error with a bandaid as it would be harder to find out the root cause once we land this.

[milianw]

@jkorous how would you use a debugger (would be GDB for me) to find the source - I would have to use RR or something like that to see why and where the invalid node is added, no?

I also don't have the breaking code at hand anymore, I can try to come up with a way to reproduce it, but as I said it was very non-intuitive and in a large file. I think it was related to parse errors in a statement for a variable that was then captured in a lambda - but that's just a hunch I got from the backtrace so far.

Generally, it's not very straight forward to build a reduced crash test for clang-c. Maybe one could use c-reduce with a minimal visitor... And preprocess the file to have it standalone. I'll see if I get a chance to look into this any time soon, I doubt it.

[milianw]

Also see https://reviews.llvm.org/D82629 which I believe is also a fix for this crash. But I didn't use a VLA in my code, but maybe the parse error just triggered something similar like that.

[milianw]

@jkorous ping? can you chime in on either of the two patches? I'm fine with either, and D82629 comes with a test too. So maybe let's just go with that one? If so, could you integrate that one please?

[jkorous]

@milianw I just approved https://reviews.llvm.org/D82629 - I feel like that patch is addressing the core issue.

[milianw]

great, thanks for the help - please land the other change then

clang/tools/libclang/CIndex.cpp

switch (LI.getKind()) {
           return true;
         break;
     }
   }
   return false;
 }
 
 bool CursorVisitor::Visit(const Stmt *S) {
+  if (!S)
+    return false;
   VisitorWorkList *WL = nullptr;
   if (!WorkListFreeList.empty()) {
     WL = WorkListFreeList.back();
     WL->clear();
     WorkListFreeList.pop_back();
   }
   else {
     WL = new VisitorWorkList();