Refactor stack trace handling

- Drop `delim` argument from the `GetStackTrace()` family of functions.
  We only ever used plain spaces.
- Use an `std::vector<std::string>` for the symbolized stack frames and
  adjust the unwind proto accordingly.

This change now prints each stack frame on its own line while skipping
duplicate ones:

```
I20200717 11:47:16.811381 3636246 monitor.cc:326] Stack trace: [
I20200717 11:47:16.811415 3636246 monitor.cc:337]   map:/lib/x86_64-linux-gnu/libc-2.30.so+0xceee7(0x7fb871602ee7)
I20200717 11:47:16.811420 3636246 monitor.cc:337]   Rot13File+0x130(0x55ed24615995)
I20200717 11:47:16.811424 3636246 monitor.cc:337]   ffi_call_unix64+0x55(0x55ed2461f2dd)
I20200717 11:47:16.811429 3636246 monitor.cc:337]   map:[stack]+0x1ec80(0x7ffee4257c80)
I20200717 11:47:16.811455 3636246 monitor.cc:339]   (last frame repeated 196 times)
I20200717 11:47:16.811460 3636246 monitor.cc:347] ]
```

PiperOrigin-RevId: 322089140
Change-Id: I05b0de2f4118fed90fe920c06bbd70ea0d1119e2

sandboxed_api/sandbox2/monitor.cc

@@ -314,15 +314,20 @@ void Monitor::SetAdditionalResultInfo(std::unique_ptr<Regs> regs) {
   result_.SetRegs(std::move(regs));
   result_.SetProgName(util::GetProgName(pid));
   result_.SetProcMaps(ReadProcMaps(pid_));
-  if (ShouldCollectStackTrace()) {
+  if (!ShouldCollectStackTrace()) {
+    LOG(INFO) << "Stack traces have been disabled";
+    return;
+  }
   auto* ns = policy_->GetNamespace();
   const Mounts empty_mounts;
-    result_.SetStackTrace(
+  result_.set_stack_trace(
       GetStackTrace(result_.GetRegs(), ns ? ns->mounts() : empty_mounts));
-    LOG(INFO) << "Stack trace: " << result_.GetStackTrace();
+
-  } else {
+  LOG(INFO) << "Stack trace: [";
-    LOG(INFO) << "Stack traces have been disabled";
+  for (const auto& frame : CompactStackTrace(result_.stack_trace())) {
+    LOG(INFO) << "  " << frame;
   }
+  LOG(INFO) << "]";
 }
 
 void Monitor::KillSandboxee() {
@@ -873,12 +878,15 @@ void Monitor::StateProcessStopped(pid_t pid, int status) {
                          executor_->libunwind_sbox_for_pid_ == 0 &&
                          policy_->GetNamespace())) {
     Regs regs(pid);
-    auto status = regs.Fetch();
+    if (auto status = regs.Fetch(); !status.ok()) {
-    if (status.ok()) {
-      VLOG(0) << "SANDBOX STACK : PID: " << pid << ", ["
-              << GetStackTrace(&regs, policy_->GetNamespace()->mounts()) << "]";
-    } else {
       LOG(WARNING) << "FAILED TO GET SANDBOX STACK : " << status;
+    } else if (SAPI_VLOG_IS_ON(0)) {
+      VLOG(0) << "SANDBOX STACK: PID: " << pid << ", [";
+      for (const auto& frame :
+           GetStackTrace(&regs, policy_->GetNamespace()->mounts())) {
+        VLOG(0) << "  " << frame;
+      }
+      VLOG(0) << "]";
     }
     should_dump_stack_ = false;
   }

sandboxed_api/sandbox2/result.cc

@@ -17,6 +17,7 @@
 #include "sandboxed_api/sandbox2/result.h"
 
 #include "absl/strings/str_cat.h"
+#include "absl/strings/str_join.h"
 #include "sandboxed_api/sandbox2/syscall.h"
 #include "sandboxed_api/sandbox2/util.h"
 
@@ -42,6 +43,10 @@ Result& Result::operator=(const Result& other) {
   return *this;
 }
 
+std::string Result::GetStackTrace() const {
+  return absl::StrJoin(stack_trace_, " ");
+}
+
 absl::Status Result::ToStatus() const {
   switch (final_status()) {
     case OK:

sandboxed_api/sandbox2/result.h

@@ -111,8 +111,8 @@ class Result {
   // The stacktrace must be sometimes fetched before SetExitStatusCode is
   // called, because after WIFEXITED() or WIFSIGNALED() the process is just a
   // zombie.
-  void SetStackTrace(const std::string& stack_trace) {
+  void set_stack_trace(std::vector<std::string> value) {
-    stack_trace_ = stack_trace;
+    stack_trace_ = std::move(value);
   }
 
   void SetRegs(std::unique_ptr<Regs> regs) { regs_ = std::move(regs); }
@@ -135,7 +135,10 @@ class Result {
     return syscall_ ? syscall_->arch() : Syscall::kUnknown;
   }
 
-  const std::string& GetStackTrace() const { return stack_trace_; }
+  const std::vector<std::string> stack_trace() { return stack_trace_; }
+
+  // Returns the stack trace as a space-delimited string.
+  std::string GetStackTrace() const;
 
   const Regs* GetRegs() const { return regs_.get(); }
 
@@ -177,7 +180,7 @@ class Result {
   uintptr_t reason_code_ = 0;
   // Might contain stack-trace of the process, especially if it failed with
   // syscall violation, or was terminated by a signal.
-  std::string stack_trace_;
+  std::vector<std::string> stack_trace_;
   // Might contain the register values of the process, similar to the stack.
   // trace
   std::unique_ptr<Regs> regs_;

sandboxed_api/sandbox2/stack_trace.cc

@@ -62,8 +62,7 @@ class StackTracePeer {
                                            const Mounts& mounts);
 
   static bool LaunchLibunwindSandbox(const Regs* regs, const Mounts& mounts,
-                                     UnwindResult* result,
+                                     UnwindResult* result);
-                                     const std::string& delim);
 };
 
 std::unique_ptr<Policy> StackTracePeer::GetPolicy(pid_t target_pid,
@@ -106,9 +105,8 @@ std::unique_ptr<Policy> StackTracePeer::GetPolicy(pid_t target_pid,
       .AddPolicyOnSyscall(
           __NR_process_vm_readv,
           {
-              // The pid technically is a 64bit int, however
+              // The pid technically is a 64bit int, however Linux usually uses
-              // Linux usually uses max 16 bit, so we are fine
+              // max 16 bit, so we are fine with comparing only 32 bits here.
-              // with comparing only 32 bits here.
               ARG_32(0),
               JEQ32(static_cast<unsigned int>(target_pid), ALLOW),
               JEQ32(static_cast<unsigned int>(1), ALLOW),
@@ -139,25 +137,23 @@ std::unique_ptr<Policy> StackTracePeer::GetPolicy(pid_t target_pid,
     }
   }
 
-  auto policy_or = builder.TryBuild();
+  auto policy = builder.TryBuild();
-  if (!policy_or.ok()) {
+  if (!policy.ok()) {
     LOG(ERROR) << "Creating stack unwinder sandbox policy failed";
     return nullptr;
   }
-  std::unique_ptr<Policy> policy = std::move(policy_or).value();
   auto keep_capabilities = absl::make_unique<std::vector<int>>();
   keep_capabilities->push_back(CAP_SYS_PTRACE);
-  policy->AllowUnsafeKeepCapabilities(std::move(keep_capabilities));
+  (*policy)->AllowUnsafeKeepCapabilities(std::move(keep_capabilities));
   // Use no special namespace flags when cloning. We will join an existing
   // user namespace and will unshare() afterwards (See forkserver.cc).
-  policy->GetNamespace()->clone_flags_ = 0;
+  (*policy)->GetNamespace()->clone_flags_ = 0;
-  return policy;
+  return std::move(*policy);
 }
 
 bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs,
                                             const Mounts& mounts,
-                                            sandbox2::UnwindResult* result,
+                                            sandbox2::UnwindResult* result) {
-                                            const std::string& delim) {
   const pid_t pid = regs->pid();
 
   // Tell executor to use this special internal mode.
@@ -240,17 +236,17 @@ bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs,
   if (!policy) {
     return false;
   }
-  auto comms = executor->ipc()->comms();
+  auto* comms = executor->ipc()->comms();
-  Sandbox2 s2(std::move(executor), std::move(policy));
+  Sandbox2 sandbox(std::move(executor), std::move(policy));
 
   VLOG(1) << "Running libunwind sandbox";
-  s2.RunAsync();
+  sandbox.RunAsync();
+
   UnwindSetup msg;
   msg.set_pid(pid);
   msg.set_regs(reinterpret_cast<const char*>(&regs->user_regs_),
                sizeof(regs->user_regs_));
   msg.set_default_max_frames(kDefaultMaxFrames);
-  msg.set_delim(delim.c_str(), delim.size());
 
   bool success = true;
   if (!comms->SendProtoBuf(msg)) {
@@ -264,23 +260,22 @@ bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs,
   }
 
   if (!success) {
-    s2.Kill();
+    sandbox.Kill();
   }
-  auto s2_result = s2.AwaitResult();
+  auto sandbox_result = sandbox.AwaitResult();
 
-  LOG(INFO) << "Libunwind execution status: " << s2_result.ToString();
+  LOG(INFO) << "Libunwind execution status: " << sandbox_result.ToString();
 
-  return success && s2_result.final_status() == Result::OK;
+  return success && sandbox_result.final_status() == Result::OK;
 }
 
-std::string GetStackTrace(const Regs* regs, const Mounts& mounts,
+std::vector<std::string> GetStackTrace(const Regs* regs, const Mounts& mounts) {
-                          const std::string& delim) {
   if (absl::GetFlag(FLAGS_sandbox_disable_all_stack_traces)) {
-    return "[Stacktraces disabled]";
+    return {"[Stacktraces disabled]"};
   }
   if (!regs) {
     LOG(WARNING) << "Could not obtain stacktrace, regs == nullptr";
-    return "[ERROR (noregs)]";
+    return {"[ERROR (noregs)]"};
   }
 
 #if defined(THREAD_SANITIZER) || defined(ADDRESS_SANITIZER) || \
@@ -301,26 +296,50 @@ std::string GetStackTrace(const Regs* regs, const Mounts& mounts,
         << "Sanitizer build, using non-sandboxed libunwind";
     LOG_IF(WARNING, coverage_enabled)
         << "Coverage build, using non-sandboxed libunwind";
-    return UnsafeGetStackTrace(regs->pid(), delim);
+    return UnsafeGetStackTrace(regs->pid());
   }
 
   if (!absl::GetFlag(FLAGS_sandbox_libunwind_crash_handler)) {
-    return UnsafeGetStackTrace(regs->pid(), delim);
+    return UnsafeGetStackTrace(regs->pid());
   }
-  UnwindResult res;
 
-  if (!StackTracePeer::LaunchLibunwindSandbox(regs, mounts, &res, delim)) {
+  UnwindResult res;
-    return "";
+  if (!StackTracePeer::LaunchLibunwindSandbox(regs, mounts, &res)) {
+    return {};
   }
-  return res.stacktrace();
+  return {res.stacktrace().begin(), res.stacktrace().end()};
 }
 
-std::string UnsafeGetStackTrace(pid_t pid, const std::string& delim) {
+std::vector<std::string> UnsafeGetStackTrace(pid_t pid) {
   LOG(WARNING) << "Using non-sandboxed libunwind";
-  std::string stack_trace;
   std::vector<uintptr_t> ips;
-  RunLibUnwindAndSymbolizer(pid, &stack_trace, &ips, kDefaultMaxFrames, delim);
+  return RunLibUnwindAndSymbolizer(pid, &ips, kDefaultMaxFrames);
-  return stack_trace;
+}
+
+std::vector<std::string> CompactStackTrace(
+    const std::vector<std::string>& stack_trace) {
+  std::vector<std::string> compact_trace;
+  compact_trace.reserve(stack_trace.size() / 2);
+  const std::string* prev = nullptr;
+  int seen = 0;
+  auto add_repeats = [&compact_trace](int seen) {
+    if (seen != 0) {
+      compact_trace.push_back(
+          absl::StrCat("(previous frame repeated ", seen, " times)"));
+    }
+  };
+  for (const auto& frame : stack_trace) {
+    if (prev && frame == *prev) {
+      ++seen;
+    } else {
+      prev = &frame;
+      add_repeats(seen);
+      seen = 0;
+      compact_trace.push_back(frame);
+    }
+  }
+  add_repeats(seen);
+  return compact_trace;
 }
 
 }  // namespace sandbox2

sandboxed_api/sandbox2/stack_trace.h

@@ -20,9 +20,11 @@
 #define SANDBOXED_API_SANDBOX2_STACK_TRACE_H_
 
 #include <sys/types.h>
+
 #include <cstddef>
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "sandboxed_api/sandbox2/mounts.h"
 #include "sandboxed_api/sandbox2/policy.h"
@@ -33,13 +35,24 @@ namespace sandbox2 {
 // Maximum depth of analyzed call stack.
 constexpr size_t kDefaultMaxFrames = 200;
 
-// Returns the stack-trace of the PID=pid, delimited by the delim argument.
+// Returns the stack-trace of the PID=pid, one line per frame.
-std::string GetStackTrace(const Regs* regs, const Mounts& mounts,
+std::vector<std::string> GetStackTrace(const Regs* regs, const Mounts& mounts);
-                          const std::string& delim = " ");
 
 // Similar to GetStackTrace() but without using the sandbox to isolate
 // libunwind.
-std::string UnsafeGetStackTrace(pid_t pid, const std::string& delim = " ");
+std::vector<std::string> UnsafeGetStackTrace(pid_t pid);
+
+// Returns a stack trace that collapses duplicate stack frames and annotates
+// them with a repetition count.
+// Example:
+//   _start              _start
+//   main                main
+//   recursive_call      recursive_call
+//   recursive_call      (previous frame repeated 2 times)
+//   recursive_call      tail_call
+//   tail_call
+std::vector<std::string> CompactStackTrace(
+    const std::vector<std::string>& stack_trace);
 
 }  // namespace sandbox2
 

sandboxed_api/sandbox2/stack_trace_test.cc

@@ -42,8 +42,10 @@ ABSL_DECLARE_FLAG(bool, sandbox_libunwind_crash_handler);
 namespace sandbox2 {
 namespace {
 
+using ::testing::ElementsAre;
 using ::testing::Eq;
 using ::testing::HasSubstr;
+using ::testing::IsEmpty;
 using ::testing::Not;
 
 // Temporarily overrides a flag, restores the original flag value when it goes
@@ -190,5 +192,30 @@ TEST(StackTraceTest, SymbolizationTrustedFilesOnly) {
   ASSERT_THAT(result.GetStackTrace(), Not(HasSubstr("CrashMe")));
 }
 
+TEST(StackTraceTest, CompactStackTrace) {
+  EXPECT_THAT(CompactStackTrace({}), IsEmpty());
+  EXPECT_THAT(CompactStackTrace({"_start"}), ElementsAre("_start"));
+  EXPECT_THAT(CompactStackTrace({
+                  "_start",
+                  "main",
+                  "recursive_call",
+                  "recursive_call",
+                  "recursive_call",
+                  "tail_call",
+              }),
+              ElementsAre("_start", "main", "recursive_call",
+                          "(previous frame repeated 2 times)", "tail_call"));
+  EXPECT_THAT(CompactStackTrace({
+                  "_start",
+                  "main",
+                  "recursive_call",
+                  "recursive_call",
+                  "recursive_call",
+                  "recursive_call",
+              }),
+              ElementsAre("_start", "main", "recursive_call",
+                          "(previous frame repeated 3 times)"));
+}
+
 }  // namespace
 }  // namespace sandbox2

sandboxed_api/sandbox2/unwind/unwind.cc

@@ -71,28 +71,30 @@ std::string GetSymbolAt(const std::map<uint64_t, std::string>& addr_to_symbol,
 
 }  // namespace
 
-void GetIPList(pid_t pid, std::vector<uintptr_t>* ips, int max_frames) {
+std::vector<uintptr_t> GetIPList(pid_t pid, int max_frames) {
-  ips->clear();
-
   unw_cursor_t cursor;
   static unw_addr_space_t as =
       unw_create_addr_space(&_UPT_accessors, 0 /* byte order */);
   if (as == nullptr) {
     SAPI_RAW_LOG(WARNING, "unw_create_addr_space() failed");
-    return;
+    return {};
   }
 
-  auto* ui = reinterpret_cast<struct UPT_info*>(_UPT_create(pid));
+  std::unique_ptr<struct UPT_info, void (*)(void*)> ui(
+      reinterpret_cast<struct UPT_info*>(_UPT_create(pid)), _UPT_destroy);
   if (ui == nullptr) {
     SAPI_RAW_LOG(WARNING, "_UPT_create() failed");
-    return;
+    return {};
   }
 
-  int rc = unw_init_remote(&cursor, as, ui);
+  int rc = unw_init_remote(&cursor, as, ui.get());
   if (rc < 0) {
     // Could be UNW_EINVAL (8), UNW_EUNSPEC (1) or UNW_EBADREG (3).
     SAPI_RAW_LOG(WARNING, "unw_init_remote() failed with error %d", rc);
-  } else {
+    return {};
+  }
+
+  std::vector<uintptr_t> ips;
   for (int i = 0; i < max_frames; i++) {
     unw_word_t ip;
     rc = unw_get_reg(&cursor, UNW_REG_IP, &ip);
@@ -101,7 +103,7 @@ void GetIPList(pid_t pid, std::vector<uintptr_t>* ips, int max_frames) {
       SAPI_RAW_LOG(WARNING, "unw_get_reg() failed with error %d", rc);
       break;
     }
-      ips->push_back(ip);
+    ips.push_back(ip);
     rc = unw_step(&cursor);
     // Non-error condition: UNW_ESUCCESS (0).
     if (rc < 0) {
@@ -111,40 +113,34 @@ void GetIPList(pid_t pid, std::vector<uintptr_t>* ips, int max_frames) {
       break;
     }
   }
-  }
+  return ips;
-
-  // This is only needed if _UPT_create() has been successful.
-  _UPT_destroy(ui);
 }
 
 bool RunLibUnwindAndSymbolizer(Comms* comms) {
-  UnwindSetup pb_setup;
+  UnwindSetup setup;
-  if (!comms->RecvProtoBuf(&pb_setup)) {
+  if (!comms->RecvProtoBuf(&setup)) {
     return false;
   }
 
-  std::string data = pb_setup.regs();
+  const std::string& data = setup.regs();
   InstallUserRegs(data.c_str(), data.length());
   ArmPtraceEmulation();
 
-  UnwindResult msg;
-  std::string stack_trace;
   std::vector<uintptr_t> ips;
+  std::vector<std::string> stack_trace =
+      RunLibUnwindAndSymbolizer(setup.pid(), &ips, setup.default_max_frames());
 
-  RunLibUnwindAndSymbolizer(pb_setup.pid(), &stack_trace, &ips,
+  UnwindResult msg;
-                            pb_setup.default_max_frames(), pb_setup.delim());
+  *msg.mutable_stacktrace() = {stack_trace.begin(), stack_trace.end()};
-  for (const auto& i : ips) {
+  *msg.mutable_ip() = {ips.begin(), ips.end()};
-    msg.add_ip(i);
-  }
-  msg.set_stacktrace(stack_trace.c_str(), stack_trace.size());
   return comms->SendProtoBuf(msg);
 }
 
-void RunLibUnwindAndSymbolizer(pid_t pid, std::string* stack_trace_out,
+std::vector<std::string> RunLibUnwindAndSymbolizer(pid_t pid,
-                               std::vector<uintptr_t>* ips, int max_frames,
+                                                   std::vector<uintptr_t>* ips,
-                               const std::string& delim) {
+                                                   int max_frames) {
   // Run libunwind.
-  GetIPList(pid, ips, max_frames);
+  *ips = GetIPList(pid, max_frames);
 
   // Open /proc/pid/maps.
   std::string path_maps = absl::StrCat("/proc/", pid, "/maps");
@@ -157,7 +153,7 @@ void RunLibUnwindAndSymbolizer(pid_t pid, std::string* stack_trace_out,
   if (!f) {
     // Could not open maps file.
     SAPI_RAW_LOG(ERROR, "Could not open %s", path_maps);
-    return;
+    return {};
   }
 
   constexpr static size_t kBufferSize = 10 * 1024 * 1024;
@@ -166,21 +162,20 @@ void RunLibUnwindAndSymbolizer(pid_t pid, std::string* stack_trace_out,
   if (bytes_read == 0) {
     // Could not read the whole maps file.
     SAPI_RAW_PLOG(ERROR, "Could not read maps file");
-    return;
+    return {};
   }
   maps_content.resize(bytes_read);
 
-  auto maps_or = ParseProcMaps(maps_content);
+  auto maps = ParseProcMaps(maps_content);
-  if (!maps_or.ok()) {
+  if (!maps.ok()) {
     SAPI_RAW_LOG(ERROR, "Could not parse /proc/%d/maps", pid);
-    return;
+    return {};
   }
-  auto maps = std::move(maps_or).value();
 
   // Get symbols for each file entry in the maps entry.
   // This is not a very efficient way, so we might want to optimize it.
   std::map<uint64_t, std::string> addr_to_symbol;
-  for (const auto& entry : maps) {
+  for (const auto& entry : *maps) {
     if (!entry.path.empty()) {
       // Store details about start + end of this map.
       // The maps entries are ordered and thus sorted with increasing adresses.
@@ -216,17 +211,15 @@ void RunLibUnwindAndSymbolizer(pid_t pid, std::string* stack_trace_out,
     }
   }
 
-  std::string stack_trace;
+  std::vector<std::string> stack_trace;
+  stack_trace.reserve(ips->size());
   // Symbolize stacktrace.
-  for (auto i = ips->begin(); i != ips->end(); ++i) {
+  for (const auto& ip : *ips) {
-    if (i != ips->begin()) {
+    const std::string symbol =
-      stack_trace += delim;
+        GetSymbolAt(addr_to_symbol, static_cast<uint64_t>(ip));
+    stack_trace.push_back(absl::StrCat(symbol, "(0x", absl::Hex(ip), ")"));
   }
-    std::string symbol = GetSymbolAt(addr_to_symbol, static_cast<uint64_t>(*i));
+  return stack_trace;
-    absl::StrAppend(&stack_trace, symbol, "(0x", absl::Hex(*i), ")");
-  }
-
-  *stack_trace_out = stack_trace;
 }
 
 }  // namespace sandbox2

sandboxed_api/sandbox2/unwind/unwind.h

@@ -25,12 +25,12 @@
 
 namespace sandbox2 {
 
-void GetIPList(pid_t pid, std::vector<uintptr_t>* ips, int max_frames);
+// Runs libunwind and the symbolizer and sends the results via comms.
 bool RunLibUnwindAndSymbolizer(Comms* comms);
 
-void RunLibUnwindAndSymbolizer(pid_t pid, std::string* stack_trace_out,
+std::vector<std::string> RunLibUnwindAndSymbolizer(pid_t pid,
-                               std::vector<uintptr_t>* ips, int max_frames,
+                                                   std::vector<uintptr_t>* ips,
-                               const std::string& delim);
+                                                   int max_frames);
 
 }  // namespace sandbox2
 

sandboxed_api/sandbox2/unwind/unwind.proto

@@ -18,22 +18,18 @@ syntax = "proto3";
 package sandbox2;
 
 message UnwindSetup {
-  // Required
+  // Process ID of the process to unwind. Required.
-  // Process ID of the process to unwind
   uint64 pid = 1;
   // Register content for the process to unwind
   bytes regs = 2;
   // Optional
   // Maximum number of stack frames to unwind
   uint64 default_max_frames = 3;
-  // Delimiter used in the result to separate the stack frames
-  bytes delim = 4;
 }
 
 message UnwindResult {
-  // Readable stacktrace, symbolized, each framed separated by the delim
+  // Readable stacktrace, symbolized, one frame per line
-  // specified in the UnwindSetup message
+  repeated string stacktrace = 1;
-  bytes stacktrace = 1;
   // Stack frames
   repeated uint64 ip = 2;
 }