[Sanitizer] Fix report_path functionality:

[samsonov]

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.

[glider]

Looks generally good, and thanks for the cleanup.

lib/sanitizer_common/sanitizer_common.h

133 ↗

(On Diff #17129)

Spare newline?

158 ↗

(On Diff #17129)

Are you planning to access these fields from outside ReportFile?
Looks like they can be made private now.

lib/sanitizer_common/sanitizer_win.cc

495 ↗

(On Diff #17129)

Isn't this function supposed to die?

[samsonov]

Address review comments.

[samsonov]

lib/sanitizer_common/sanitizer_common.h

133 ↗

(On Diff #17129)

Fixed

158 ↗

(On Diff #17129)

I can't make them private, because I need aggregate initialization for global ReportFile object (we're not allowed to have global constructors in the runtime). I'll leave a comment about that.

lib/sanitizer_common/sanitizer_win.cc

495 ↗

(On Diff #17129)

Alright, seems that closed fd is a normal situation on the Windows. Renamed this function.

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h

 };
 typedef void (*LowLevelAllocateCallback)(uptr ptr, uptr size);
 // Allows to register tool-specific callbacks for LowLevelAllocator.
 // Passing NULL removes the callback.
 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, ...);
 void SetPrintfAndReportCallback(void (*callback)(const char *));
 #define VReport(level, ...)                                              \
   do {                                                                   \
     if ((uptr)common_flags()->verbosity >= (level)) Report(__VA_ARGS__); \
   } while (0)
 #define VPrintf(level, ...)                                              \
   do {                                                                   \
     if ((uptr)common_flags()->verbosity >= (level)) Printf(__VA_ARGS__); \
   } while (0)
 
 // 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 <path_prefix>.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;
 
 uptr OpenFile(const char *filename, bool write);
 // Opens the file 'file_name" and reads up to 'max_len' bytes.
 // The resulting buffer is mmaped and stored in '*buff'.
 // The size of the mmaped region is stored in '*buff_size',
 // Returns the number of read bytes or 0 if file can not be opened.

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.cc

 
 uptr GetPageSizeCached() {
   static uptr PageSize;
   if (!PageSize)
     PageSize = GetPageSize();
   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.<PID>".
-fd_t report_fd = kStderrFd;
-
-// Set via __sanitizer_set_report_path.
-bool log_to_file = false;
-char report_path_prefix[sizeof(report_path_prefix)];
-
-// 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;
+void RawWrite(const char *buffer) {
+  report_file.Write(buffer, internal_strlen(buffer));
+}
+
+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);
+  }
+
+  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.
 uptr stoptheworld_tracer_pid = 0;
 // Cached pid of parent process - if the parent process dies, we want to keep
 // writing to the same log file.
 uptr stoptheworld_tracer_ppid = 0;
 

 }
 
 }  // namespace __sanitizer
 
 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) {
   Printf("%s\n", error_summary);
 }
 }  // extern "C"

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_libcdep.cc

 
 #include "sanitizer_common.h"
 #include "sanitizer_flags.h"
 #include "sanitizer_stacktrace.h"
 #include "sanitizer_symbolizer.h"
 
 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)();
 void SetSandboxingCallback(void (*f)()) {
   sandboxing_callback = f;
 }
 
 void ReportErrorSummary(const char *error_type, StackTrace *stack) {

compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.cc

if (prefix_len + name_len + 2 <= kMaxPathLength) {
         return internal_strdup(buffer.data());
     }
     if (*end == '\0') break;
     beg = end + 1;
   }
   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();
   }
 }
 
 bool GetCodeRangeForFile(const char *module, uptr *start, uptr *end) {
   uptr s, e, off, prot;
   InternalScopedString buff(kMaxPathLength);
   MemoryMappingLayout proc_maps(/*cache_enabled*/false);

compiler-rt/trunk/lib/sanitizer_common/sanitizer_win.cc

while (StackWalk64(machine_type, GetCurrentProcess(), GetCurrentThread(),
                      &stack_frame, &ctx, NULL, &SymFunctionTableAccess64,
                      &SymGetModuleBase64, NULL) &&
          size < Min(max_depth, kStackTraceMax)) {
     trace_buffer[size++] = (uptr)stack_frame.AddrPC.Offset;
   }
 }
 #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.
     OutputDebugStringA(buffer);
   }
 }
 
 void SetAlternateSignalStack() {