Page MenuHomePhabricator

[libclang]: check validity before visiting Stmt node
AbandonedPublic

Authored by milianw on Jun 29 2020, 1:02 AM.

Details

Reviewers
bkramer
yvvan
nik
akyrtzi
benlangmuir
arphaman
jkorous
Group Reviewers
Restricted Project
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

Diff Detail

Unit TestsFailed

TimeTest
10,450 mslinux > libomp.env::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && env KMP_DISP_NUM_BUFFERS=0 /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp
1,230 mslinux > libomp.worksharing/for::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

Event Timeline

milianw created this revision.Jun 29 2020, 1:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 1:02 AM
milianw added a reviewer: Restricted Project.Jun 29 2020, 1:02 AM

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

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

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.

@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.

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.

@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?

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

milianw abandoned this revision.Jul 7 2020, 1:18 AM

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