This is an archive of the discontinued LLVM Phabricator instance.

[asan] Fix nested error detection
ClosedPublic

Authored by vitalybuka on Sep 18 2017, 6:54 PM.

Event Timeline

vitalybuka created this revision.Sep 18 2017, 6:54 PM

Spin on reporting_thread_tid_

Reordered code.

Test comment and fixed typo in the test name

dvyukov added inline comments.Sep 19 2017, 12:32 AM
compiler-rt/lib/asan/asan_report.cc
126–127

This looks like a data race.
While one thread executes ScopedInErrorReport, another thread can do something like:

halt_on_error_ = fatal;
if (flags()->halt_on_error) halt_on_error_ = true;

And then the first thread can fail to halt and the program will hang for 100 seconds.

Or, there can be 2 thread: one with fatal=true and another with fatal=false, and asan will halt after a non-fatal error which can cause user confusion.

129–153

This looks somewhat fishy. Is there any known problem with making this an infinite loop as it is now?
Pretty sure SleepForMillis can undersleep if SIGPOF profiling is enabled, and then we can terminate the process while another thread still reports an error.

vitalybuka marked 2 inline comments as done.

infinit loop and not crash for fatal errors

vitalybuka added inline comments.Sep 19 2017, 11:15 AM
compiler-rt/lib/asan/asan_report.cc
126–127

This looks like a data race.
While one thread executes ScopedInErrorReport, another thread can do something like:
halt_on_error_ = fatal;
if (flags()->halt_on_error) halt_on_error_ = true;

Not sure I understand you.
halt_on_error_ is not static, and only one thread suppose to access particular ScopedInErrorReport instance.

Or, there can be 2 thread: one with fatal=true and another with fatal=false, and asan will halt after a non-fatal error which can cause user confusion.

Correct. I noticed that this is weird too, but I believe it's preexisted behavior and not related to the bug I am fixing. So I was not trying to improve that.
If I understand correctly, to avoid such confusion, I can throw away "if (halt_on_error_) break;" And code after the loop.

129–153

Probably nothing known.
I'll switch back to the infinit one.

dvyukov added inline comments.Sep 20 2017, 1:34 AM
compiler-rt/lib/asan/asan_report.cc
126–127

Right. It's not static. Withdrawn.

vitalybuka added 1 blocking reviewer(s): eugenis.Sep 20 2017, 2:37 PM
eugenis added inline comments.Sep 20 2017, 3:06 PM
compiler-rt/lib/asan/asan_report.cc
127–160

What if GetCurrentTidOrInvalid return kInvalidTid? We will start reporting w/o claiming reporting_thread_tid_. Use a different default value?

Different unclaimed sentinel

vitalybuka marked an inline comment as done.Sep 20 2017, 3:28 PM
eugenis accepted this revision.Sep 20 2017, 3:39 PM
eugenis added inline comments.
compiler-rt/lib/asan/asan_report.cc
125

0xfffffe

This revision is now accepted and ready to land.Sep 20 2017, 3:39 PM
This revision was automatically updated to reflect the committed changes.
vitalybuka marked an inline comment as done.