Page MenuHomePhabricator

[ASan] Fix issue where system log buffer was not cleared after reporting an issue.
ClosedPublic

Authored by delcypher on Mar 24 2020, 7:49 PM.

Details

Summary

When ASan reports an issue the contents of the system log buffer
(error_message_buffer) get flushed to the system log (via
LogFullErrorReport()). After this happens the buffer is not cleared
but this is usually fine because the process usually exits soon after
reporting the issue.

However, when ASan runs in halt_on_error=0 mode execution continues
without clearing the buffer. This leads to problems if more ASan
issues are found and reported.

  1. Duplicate ASan reports in the system log. The Nth (start counting from 1)

ASan report will be duplicated (M - N) times in the system log if M is the
number of ASan issues reported.

  1. Lost ASan reports. Given a sufficient

number of reports the buffer will fill up and consequently cannot be appended
to. This means reports can be lost.

The fix here is to reset error_message_buffer_pos to 0 which
effectively clears the system log buffer.

A test case is included but unfortunately it is Darwin specific because
querying the system log is an OS specific activity.

rdar://problem/55986279

Diff Detail

Event Timeline

delcypher created this revision.Mar 24 2020, 7:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2020, 7:49 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Mar 24 2020, 10:16 PM
compiler-rt/lib/asan/asan_report.cpp
58

static?
or maybe just avoid a function

162–165

I guess it should be reset here just after internal_memcpy under the same lock

delcypher marked 2 inline comments as done.Mar 25 2020, 3:42 PM
delcypher added inline comments.
compiler-rt/lib/asan/asan_report.cpp
58

The reason I made this a function is because I intend to refactor the logging support out of ASan into sanitizer_common so that UBSan has syslog support. However putting the logic inside a function at this stage is probably premature. I'll remove it.

162–165

That sounds reasonable. I placed my code later because there was already a block conditional on halt_on_error_. However what you suggested sounds better because it avoids any races where something writes to the buffer after it gets released here and where I added my code to clear the buffer.

delcypher updated this revision to Diff 252701.Mar 25 2020, 4:10 PM

Address review feedback

@vitalybuka Thanks for the initial review. I've tried to address your comments. Please let me know if there's anything else you'd like to change.

vitalybuka added inline comments.Mar 25 2020, 8:27 PM
compiler-rt/lib/asan/asan_report.cpp
163

why do we need to check halt_on_error_?
we copied all info line above, we need nothing there in any case

delcypher marked an inline comment as done.Mar 25 2020, 8:47 PM
delcypher added inline comments.
compiler-rt/lib/asan/asan_report.cpp
163

Clearing the buffer is only needed when halt_on_error_ is false. However the process will soon die when halt_on_error_ is true so there probably isn't any harm is clearing the buffer unconditionally. I'll update the patch.

delcypher updated this revision to Diff 252736.Mar 25 2020, 8:50 PM

Clear error_message_buffer unconditionally.

vitalybuka accepted this revision.Mar 25 2020, 9:56 PM
This revision is now accepted and ready to land.Mar 25 2020, 9:56 PM
This revision was automatically updated to reflect the committed changes.