From 3a41ed15743a1a6bd7d0bcc3ed627dcba692c05c Mon Sep 17 00:00:00 2001 From: Alexey Samsonov Date: Thu, 11 Dec 2014 18:30:25 +0000 Subject: [PATCH] [Sanitizer] Fix report_path functionality: Summary: - Make sure mmap() is never called inside RawWrite function. - Wrap a bunch of standalone globals in a ReportFile object. - Make sure accesses to these globals are thread-safe. - Fix report_path functionality on Windows, where __sanitizer_set_report_path() would break program. I've started this yak shaving in order to make "CommonFlags::mmap_limit_mb" immutable. Currently we drop this flag to zero before printing an error message. Test Plan: regression test suite Reviewers: kcc, glider Subscribers: llvm-commits Differential Revision: http://reviews.llvm.org/D6595 llvm-svn: 224031 --- .../lib/sanitizer_common/sanitizer_common.cc | 92 ++++++++++++++-------- .../lib/sanitizer_common/sanitizer_common.h | 35 ++++++-- .../sanitizer_common/sanitizer_common_libcdep.cc | 23 ++---- .../lib/sanitizer_common/sanitizer_posix.cc | 38 ++------- compiler-rt/lib/sanitizer_common/sanitizer_win.cc | 13 +-- 5 files changed, 103 insertions(+), 98 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common.cc b/compiler-rt/lib/sanitizer_common/sanitizer_common.cc index 737177c..eb5ed36 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_common.cc +++ b/compiler-rt/lib/sanitizer_common/sanitizer_common.cc @@ -26,19 +26,66 @@ uptr GetPageSizeCached() { return PageSize; } +StaticSpinMutex report_file_mu; +ReportFile report_file = {&report_file_mu, kStderrFd, "", "", 0}; -// By default, dump to stderr. If |log_to_file| is true and |report_fd_pid| -// isn't equal to the current PID, try to obtain file descriptor by opening -// file "report_path_prefix.". -fd_t report_fd = kStderrFd; +void RawWrite(const char *buffer) { + report_file.Write(buffer, internal_strlen(buffer)); +} -// Set via __sanitizer_set_report_path. -bool log_to_file = false; -char report_path_prefix[sizeof(report_path_prefix)]; +void ReportFile::ReopenIfNecessary() { + mu->CheckLocked(); + if (fd == kStdoutFd || fd == kStderrFd) return; + + uptr pid = internal_getpid(); + // If in tracer, use the parent's file. + if (pid == stoptheworld_tracer_pid) + pid = stoptheworld_tracer_ppid; + if (fd != kInvalidFd) { + // If the report file is already opened by the current process, + // do nothing. Otherwise the report file was opened by the parent + // process, close it now. + if (fd_pid == pid) + return; + else + internal_close(fd); + } -// PID of process that opened |report_fd|. If a fork() occurs, the PID of the -// child thread will be different from |report_fd_pid|. -uptr report_fd_pid = 0; + internal_snprintf(full_path, kMaxPathLength, "%s.%zu", path_prefix, pid); + uptr openrv = OpenFile(full_path, true); + if (internal_iserror(openrv)) { + const char *ErrorMsgPrefix = "ERROR: Can't open file: "; + internal_write(kStderrFd, ErrorMsgPrefix, internal_strlen(ErrorMsgPrefix)); + internal_write(kStderrFd, full_path, internal_strlen(full_path)); + Die(); + } + fd = openrv; + fd_pid = pid; +} + +void ReportFile::SetReportPath(const char *path) { + if (!path) + return; + uptr len = internal_strlen(path); + if (len > sizeof(path_prefix) - 100) { + Report("ERROR: Path is too long: %c%c%c%c%c%c%c%c...\n", + path[0], path[1], path[2], path[3], + path[4], path[5], path[6], path[7]); + Die(); + } + + SpinMutexLock l(mu); + if (fd != kStdoutFd && fd != kStderrFd && fd != kInvalidFd) + internal_close(fd); + fd = kInvalidFd; + if (internal_strcmp(path, "stdout") == 0) { + fd = kStdoutFd; + } else if (internal_strcmp(path, "stderr") == 0) { + fd = kStderrFd; + } else { + internal_snprintf(path_prefix, kMaxPathLength, "%s", path); + } +} // PID of the tracer task in StopTheWorld. It shares the address space with the // main process, but has a different PID and thus requires special handling. @@ -230,30 +277,7 @@ using namespace __sanitizer; // NOLINT extern "C" { void __sanitizer_set_report_path(const char *path) { - if (!path) - return; - uptr len = internal_strlen(path); - if (len > sizeof(report_path_prefix) - 100) { - Report("ERROR: Path is too long: %c%c%c%c%c%c%c%c...\n", - path[0], path[1], path[2], path[3], - path[4], path[5], path[6], path[7]); - Die(); - } - if (report_fd != kStdoutFd && - report_fd != kStderrFd && - report_fd != kInvalidFd) - internal_close(report_fd); - report_fd = kInvalidFd; - log_to_file = false; - if (internal_strcmp(path, "stdout") == 0) { - report_fd = kStdoutFd; - } else if (internal_strcmp(path, "stderr") == 0) { - report_fd = kStderrFd; - } else { - internal_strncpy(report_path_prefix, path, sizeof(report_path_prefix)); - report_path_prefix[len] = '\0'; - log_to_file = true; - } + report_file.SetReportPath(path); } void __sanitizer_report_error_summary(const char *error_summary) { diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common.h b/compiler-rt/lib/sanitizer_common/sanitizer_common.h index 6a07fde..c00dce6 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_common.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_common.h @@ -129,9 +129,6 @@ void SetLowLevelAllocateCallback(LowLevelAllocateCallback callback); // IO void RawWrite(const char *buffer); -bool PrintsToTty(); -// Caching version of PrintsToTty(). Not thread-safe. -bool PrintsToTtyCached(); bool ColorizeReports(); void Printf(const char *format, ...); void Report(const char *format, ...); @@ -147,11 +144,33 @@ void SetPrintfAndReportCallback(void (*callback)(const char *)); // Can be used to prevent mixing error reports from different sanitizers. extern StaticSpinMutex CommonSanitizerReportMutex; -void MaybeOpenReportFile(); -extern fd_t report_fd; -extern bool log_to_file; -extern char report_path_prefix[kMaxPathLength]; -extern uptr report_fd_pid; + +struct ReportFile { + void Write(const char *buffer, uptr length); + bool PrintsToTty(); + void SetReportPath(const char *path); + + // Don't use fields directly. They are only declared public to allow + // aggregate initialization. + + // Protects fields below. + StaticSpinMutex *mu; + // Opened file descriptor. Defaults to stderr. It may be equal to + // kInvalidFd, in which case new file will be opened when necessary. + fd_t fd; + // Path prefix of report file, set via __sanitizer_set_report_path. + char path_prefix[kMaxPathLength]; + // Full path to report, obtained as .PID + char full_path[kMaxPathLength]; + // PID of the process that opened fd. If a fork() occurs, + // the PID of child will be different from fd_pid. + uptr fd_pid; + + private: + void ReopenIfNecessary(); +}; +extern ReportFile report_file; + extern uptr stoptheworld_tracer_pid; extern uptr stoptheworld_tracer_ppid; diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common_libcdep.cc b/compiler-rt/lib/sanitizer_common/sanitizer_common_libcdep.cc index 328ea64..20c1d5a 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_common_libcdep.cc +++ b/compiler-rt/lib/sanitizer_common/sanitizer_common_libcdep.cc @@ -18,30 +18,21 @@ namespace __sanitizer { -bool PrintsToTty() { - MaybeOpenReportFile(); - return internal_isatty(report_fd) != 0; +bool ReportFile::PrintsToTty() { + SpinMutexLock l(mu); + ReopenIfNecessary(); + return internal_isatty(fd) != 0; } -bool PrintsToTtyCached() { +bool ColorizeReports() { // FIXME: Add proper Windows support to AnsiColorDecorator and re-enable color // printing on Windows. if (SANITIZER_WINDOWS) - return 0; + return false; - static int cached = 0; - static bool prints_to_tty; - if (!cached) { // Not thread-safe. - prints_to_tty = PrintsToTty(); - cached = 1; - } - return prints_to_tty; -} - -bool ColorizeReports() { const char *flag = common_flags()->color; return internal_strcmp(flag, "always") == 0 || - (internal_strcmp(flag, "auto") == 0 && PrintsToTtyCached()); + (internal_strcmp(flag, "auto") == 0 && report_file.PrintsToTty()); } static void (*sandboxing_callback)(); diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_posix.cc b/compiler-rt/lib/sanitizer_common/sanitizer_posix.cc index 23a92c5..4205c2b 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_posix.cc +++ b/compiler-rt/lib/sanitizer_common/sanitizer_posix.cc @@ -286,37 +286,13 @@ char *FindPathToBinary(const char *name) { return 0; } -void MaybeOpenReportFile() { - if (!log_to_file) return; - uptr pid = internal_getpid(); - // If in tracer, use the parent's file. - if (pid == stoptheworld_tracer_pid) - pid = stoptheworld_tracer_ppid; - if (report_fd_pid == pid) return; - InternalScopedString report_path_full(kMaxPathLength); - report_path_full.append("%s.%zu", report_path_prefix, pid); - uptr openrv = OpenFile(report_path_full.data(), true); - if (internal_iserror(openrv)) { - report_fd = kStderrFd; - log_to_file = false; - Report("ERROR: Can't open file: %s\n", report_path_full.data()); - Die(); - } - if (report_fd != kInvalidFd) { - // We're in the child. Close the parent's log. - internal_close(report_fd); - } - report_fd = openrv; - report_fd_pid = pid; -} - -void RawWrite(const char *buffer) { - static const char *kRawWriteError = - "RawWrite can't output requested buffer!\n"; - uptr length = (uptr)internal_strlen(buffer); - MaybeOpenReportFile(); - if (length != internal_write(report_fd, buffer, length)) { - internal_write(report_fd, kRawWriteError, internal_strlen(kRawWriteError)); +void ReportFile::Write(const char *buffer, uptr length) { + SpinMutexLock l(mu); + static const char *kWriteError = + "ReportFile::Write() can't output requested buffer!\n"; + ReopenIfNecessary(); + if (length != internal_write(fd, buffer, length)) { + internal_write(fd, kWriteError, internal_strlen(kWriteError)); Die(); } } diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_win.cc b/compiler-rt/lib/sanitizer_common/sanitizer_win.cc index d3d791b..3e90141 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_win.cc +++ b/compiler-rt/lib/sanitizer_common/sanitizer_win.cc @@ -492,15 +492,10 @@ void BufferedStackTrace::SlowUnwindStackWithContext(uptr pc, void *context, } #endif // #if !SANITIZER_GO -void MaybeOpenReportFile() { - // Windows doesn't have native fork, and we don't support Cygwin or other - // environments that try to fake it, so the initial report_fd will always be - // correct. -} - -void RawWrite(const char *buffer) { - uptr length = (uptr)internal_strlen(buffer); - if (length != internal_write(report_fd, buffer, length)) { +void ReportFile::Write(const char *buffer, uptr length) { + SpinMutexLock l(mu); + ReopenIfNecessary(); + if (length != internal_write(fd, buffer, length)) { // stderr may be closed, but we may be able to print to the debugger // instead. This is the case when launching a program from Visual Studio, // and the following routine should write to its console. -- 2.7.4