From eb62bae167aa1179afd79a4fa3154757023ba547 Mon Sep 17 00:00:00 2001 From: Christian Blichmann Date: Mon, 20 Jul 2020 00:24:12 -0700 Subject: [PATCH] Refactor stack trace handling - Drop `delim` argument from the `GetStackTrace()` family of functions. We only ever used plain spaces. - Use an `std::vector` 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 | 32 +++--- sandboxed_api/sandbox2/result.cc | 5 + sandboxed_api/sandbox2/result.h | 11 +- sandboxed_api/sandbox2/stack_trace.cc | 89 ++++++++++------- sandboxed_api/sandbox2/stack_trace.h | 21 +++- sandboxed_api/sandbox2/stack_trace_test.cc | 27 +++++ sandboxed_api/sandbox2/unwind/unwind.cc | 111 ++++++++++----------- sandboxed_api/sandbox2/unwind/unwind.h | 8 +- sandboxed_api/sandbox2/unwind/unwind.proto | 10 +- 9 files changed, 189 insertions(+), 125 deletions(-) diff --git a/sandboxed_api/sandbox2/monitor.cc b/sandboxed_api/sandbox2/monitor.cc index 38fa547..10e0710 100644 --- a/sandboxed_api/sandbox2/monitor.cc +++ b/sandboxed_api/sandbox2/monitor.cc @@ -314,15 +314,20 @@ void Monitor::SetAdditionalResultInfo(std::unique_ptr regs) { result_.SetRegs(std::move(regs)); result_.SetProgName(util::GetProgName(pid)); result_.SetProcMaps(ReadProcMaps(pid_)); - if (ShouldCollectStackTrace()) { - auto* ns = policy_->GetNamespace(); - const Mounts empty_mounts; - result_.SetStackTrace( - GetStackTrace(result_.GetRegs(), ns ? ns->mounts() : empty_mounts)); - LOG(INFO) << "Stack trace: " << result_.GetStackTrace(); - } else { + if (!ShouldCollectStackTrace()) { LOG(INFO) << "Stack traces have been disabled"; + return; } + auto* ns = policy_->GetNamespace(); + const Mounts empty_mounts; + result_.set_stack_trace( + GetStackTrace(result_.GetRegs(), ns ? ns->mounts() : empty_mounts)); + + LOG(INFO) << "Stack trace: ["; + 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 (status.ok()) { - VLOG(0) << "SANDBOX STACK : PID: " << pid << ", [" - << GetStackTrace(®s, policy_->GetNamespace()->mounts()) << "]"; - } else { + if (auto status = regs.Fetch(); !status.ok()) { 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(®s, policy_->GetNamespace()->mounts())) { + VLOG(0) << " " << frame; + } + VLOG(0) << "]"; } should_dump_stack_ = false; } diff --git a/sandboxed_api/sandbox2/result.cc b/sandboxed_api/sandbox2/result.cc index ae78775..4c16fc4 100644 --- a/sandboxed_api/sandbox2/result.cc +++ b/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: diff --git a/sandboxed_api/sandbox2/result.h b/sandboxed_api/sandbox2/result.h index 3d48a1e..f381ccb 100644 --- a/sandboxed_api/sandbox2/result.h +++ b/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) { - stack_trace_ = stack_trace; + void set_stack_trace(std::vector value) { + stack_trace_ = std::move(value); } void SetRegs(std::unique_ptr 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 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 stack_trace_; // Might contain the register values of the process, similar to the stack. // trace std::unique_ptr regs_; diff --git a/sandboxed_api/sandbox2/stack_trace.cc b/sandboxed_api/sandbox2/stack_trace.cc index adfec8c..7df7952 100644 --- a/sandboxed_api/sandbox2/stack_trace.cc +++ b/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, - const std::string& delim); + UnwindResult* result); }; std::unique_ptr StackTracePeer::GetPolicy(pid_t target_pid, @@ -106,9 +105,8 @@ std::unique_ptr StackTracePeer::GetPolicy(pid_t target_pid, .AddPolicyOnSyscall( __NR_process_vm_readv, { - // The pid technically is a 64bit int, however - // Linux usually uses max 16 bit, so we are fine - // with comparing only 32 bits here. + // The pid technically is a 64bit int, however Linux usually uses + // max 16 bit, so we are fine with comparing only 32 bits here. ARG_32(0), JEQ32(static_cast(target_pid), ALLOW), JEQ32(static_cast(1), ALLOW), @@ -139,25 +137,23 @@ std::unique_ptr StackTracePeer::GetPolicy(pid_t target_pid, } } - auto policy_or = builder.TryBuild(); - if (!policy_or.ok()) { + auto policy = builder.TryBuild(); + if (!policy.ok()) { LOG(ERROR) << "Creating stack unwinder sandbox policy failed"; return nullptr; } - std::unique_ptr policy = std::move(policy_or).value(); auto keep_capabilities = absl::make_unique>(); 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; - return policy; + (*policy)->GetNamespace()->clone_flags_ = 0; + return std::move(*policy); } bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs, const Mounts& mounts, - sandbox2::UnwindResult* result, - const std::string& delim) { + sandbox2::UnwindResult* result) { 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(); - Sandbox2 s2(std::move(executor), std::move(policy)); + auto* comms = executor->ipc()->comms(); + 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(®s->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, - const std::string& delim) { +std::vector GetStackTrace(const Regs* regs, const Mounts& mounts) { 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)) { - return ""; + UnwindResult res; + 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 UnsafeGetStackTrace(pid_t pid) { LOG(WARNING) << "Using non-sandboxed libunwind"; - std::string stack_trace; std::vector ips; - RunLibUnwindAndSymbolizer(pid, &stack_trace, &ips, kDefaultMaxFrames, delim); - return stack_trace; + return RunLibUnwindAndSymbolizer(pid, &ips, kDefaultMaxFrames); +} + +std::vector CompactStackTrace( + const std::vector& stack_trace) { + std::vector 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 diff --git a/sandboxed_api/sandbox2/stack_trace.h b/sandboxed_api/sandbox2/stack_trace.h index ddb5fee..de948d4 100644 --- a/sandboxed_api/sandbox2/stack_trace.h +++ b/sandboxed_api/sandbox2/stack_trace.h @@ -20,9 +20,11 @@ #define SANDBOXED_API_SANDBOX2_STACK_TRACE_H_ #include + #include #include #include +#include #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. -std::string GetStackTrace(const Regs* regs, const Mounts& mounts, - const std::string& delim = " "); +// Returns the stack-trace of the PID=pid, one line per frame. +std::vector GetStackTrace(const Regs* regs, const Mounts& mounts); // Similar to GetStackTrace() but without using the sandbox to isolate // libunwind. -std::string UnsafeGetStackTrace(pid_t pid, const std::string& delim = " "); +std::vector 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 CompactStackTrace( + const std::vector& stack_trace); } // namespace sandbox2 diff --git a/sandboxed_api/sandbox2/stack_trace_test.cc b/sandboxed_api/sandbox2/stack_trace_test.cc index bbc198a..0f0c600 100644 --- a/sandboxed_api/sandbox2/stack_trace_test.cc +++ b/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 diff --git a/sandboxed_api/sandbox2/unwind/unwind.cc b/sandboxed_api/sandbox2/unwind/unwind.cc index 7d57519..3da9b46 100644 --- a/sandboxed_api/sandbox2/unwind/unwind.cc +++ b/sandboxed_api/sandbox2/unwind/unwind.cc @@ -71,80 +71,76 @@ std::string GetSymbolAt(const std::map& addr_to_symbol, } // namespace -void GetIPList(pid_t pid, std::vector* ips, int max_frames) { - ips->clear(); - +std::vector GetIPList(pid_t pid, int max_frames) { 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(_UPT_create(pid)); + std::unique_ptr ui( + reinterpret_cast(_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 { - for (int i = 0; i < max_frames; i++) { - unw_word_t ip; - rc = unw_get_reg(&cursor, UNW_REG_IP, &ip); - if (rc < 0) { - // Could be UNW_EUNSPEC or UNW_EBADREG. - SAPI_RAW_LOG(WARNING, "unw_get_reg() failed with error %d", rc); - break; - } - ips->push_back(ip); - rc = unw_step(&cursor); - // Non-error condition: UNW_ESUCCESS (0). - if (rc < 0) { - // If anything but UNW_ESTOPUNWIND (-5), there has been an error. - // However since we can't do anything about it and it appears that - // this happens every time we don't log this. - break; - } - } + return {}; } - // This is only needed if _UPT_create() has been successful. - _UPT_destroy(ui); + std::vector ips; + for (int i = 0; i < max_frames; i++) { + unw_word_t ip; + rc = unw_get_reg(&cursor, UNW_REG_IP, &ip); + if (rc < 0) { + // Could be UNW_EUNSPEC or UNW_EBADREG. + SAPI_RAW_LOG(WARNING, "unw_get_reg() failed with error %d", rc); + break; + } + ips.push_back(ip); + rc = unw_step(&cursor); + // Non-error condition: UNW_ESUCCESS (0). + if (rc < 0) { + // If anything but UNW_ESTOPUNWIND (-5), there has been an error. + // However since we can't do anything about it and it appears that + // this happens every time we don't log this. + break; + } + } + return ips; } bool RunLibUnwindAndSymbolizer(Comms* comms) { - UnwindSetup pb_setup; - if (!comms->RecvProtoBuf(&pb_setup)) { + UnwindSetup 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 ips; + std::vector stack_trace = + RunLibUnwindAndSymbolizer(setup.pid(), &ips, setup.default_max_frames()); - RunLibUnwindAndSymbolizer(pb_setup.pid(), &stack_trace, &ips, - pb_setup.default_max_frames(), pb_setup.delim()); - for (const auto& i : ips) { - msg.add_ip(i); - } - msg.set_stacktrace(stack_trace.c_str(), stack_trace.size()); + UnwindResult msg; + *msg.mutable_stacktrace() = {stack_trace.begin(), stack_trace.end()}; + *msg.mutable_ip() = {ips.begin(), ips.end()}; return comms->SendProtoBuf(msg); } -void RunLibUnwindAndSymbolizer(pid_t pid, std::string* stack_trace_out, - std::vector* ips, int max_frames, - const std::string& delim) { +std::vector RunLibUnwindAndSymbolizer(pid_t pid, + std::vector* ips, + 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); - if (!maps_or.ok()) { + auto maps = ParseProcMaps(maps_content); + 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 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 stack_trace; + stack_trace.reserve(ips->size()); // Symbolize stacktrace. - for (auto i = ips->begin(); i != ips->end(); ++i) { - if (i != ips->begin()) { - stack_trace += delim; - } - std::string symbol = GetSymbolAt(addr_to_symbol, static_cast(*i)); - absl::StrAppend(&stack_trace, symbol, "(0x", absl::Hex(*i), ")"); + for (const auto& ip : *ips) { + const std::string symbol = + GetSymbolAt(addr_to_symbol, static_cast(ip)); + stack_trace.push_back(absl::StrCat(symbol, "(0x", absl::Hex(ip), ")")); } - - *stack_trace_out = stack_trace; + return stack_trace; } } // namespace sandbox2 diff --git a/sandboxed_api/sandbox2/unwind/unwind.h b/sandboxed_api/sandbox2/unwind/unwind.h index eae539f..883593c 100644 --- a/sandboxed_api/sandbox2/unwind/unwind.h +++ b/sandboxed_api/sandbox2/unwind/unwind.h @@ -25,12 +25,12 @@ namespace sandbox2 { -void GetIPList(pid_t pid, std::vector* 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* ips, int max_frames, - const std::string& delim); +std::vector RunLibUnwindAndSymbolizer(pid_t pid, + std::vector* ips, + int max_frames); } // namespace sandbox2 diff --git a/sandboxed_api/sandbox2/unwind/unwind.proto b/sandboxed_api/sandbox2/unwind/unwind.proto index d81a093..f50c201 100644 --- a/sandboxed_api/sandbox2/unwind/unwind.proto +++ b/sandboxed_api/sandbox2/unwind/unwind.proto @@ -18,22 +18,18 @@ syntax = "proto3"; package sandbox2; message UnwindSetup { - // Required - // Process ID of the process to unwind + // Process ID of the process to unwind. Required. 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 - // specified in the UnwindSetup message - bytes stacktrace = 1; + // Readable stacktrace, symbolized, one frame per line + repeated string stacktrace = 1; // Stack frames repeated uint64 ip = 2; }