diff --git a/sandboxed_api/sandbox2/monitor.cc b/sandboxed_api/sandbox2/monitor.cc index 045d7a4..f894ce2 100644 --- a/sandboxed_api/sandbox2/monitor.cc +++ b/sandboxed_api/sandbox2/monitor.cc @@ -68,6 +68,7 @@ #include "sandboxed_api/sandbox2/util.h" #include "sandboxed_api/util/file_helpers.h" #include "sandboxed_api/util/raw_logging.h" +#include "sandboxed_api/util/status_macros.h" #include "sandboxed_api/util/temp_file.h" using std::string; @@ -373,8 +374,15 @@ void Monitor::SetAdditionalResultInfo(std::unique_ptr regs) { } auto* ns = policy_->GetNamespace(); const Mounts empty_mounts; - result_.set_stack_trace( - GetStackTrace(result_.GetRegs(), ns ? ns->mounts() : empty_mounts)); + absl::StatusOr> stack_trace = + GetStackTrace(result_.GetRegs(), ns ? ns->mounts() : empty_mounts); + + if (!stack_trace.ok()) { + LOG(ERROR) << "Could not obtain stack trace: " << stack_trace.status(); + return; + } + + result_.set_stack_trace(*stack_trace); LOG(INFO) << "Stack trace: ["; for (const auto& frame : CompactStackTrace(result_.stack_trace())) { @@ -936,13 +944,18 @@ void Monitor::StateProcessStopped(pid_t pid, int status) { if (ABSL_PREDICT_FALSE(pid == pid_ && should_dump_stack_ && executor_->libunwind_sbox_for_pid_ == 0 && policy_->GetNamespace())) { - Regs regs(pid); - if (auto status = regs.Fetch(); !status.ok()) { - LOG(WARNING) << "FAILED TO GET SANDBOX STACK : " << status; + auto stack_trace = [this, + pid]() -> absl::StatusOr> { + Regs regs(pid); + SAPI_RETURN_IF_ERROR(regs.Fetch()); + return GetStackTrace(®s, policy_->GetNamespace()->mounts()); + }(); + + if (!stack_trace.ok()) { + LOG(WARNING) << "FAILED TO GET SANDBOX STACK : " << stack_trace.status(); } else if (SAPI_VLOG_IS_ON(0)) { VLOG(0) << "SANDBOX STACK: PID: " << pid << ", ["; - for (const auto& frame : - GetStackTrace(®s, policy_->GetNamespace()->mounts())) { + for (const auto& frame : *stack_trace) { VLOG(0) << " " << frame; } VLOG(0) << "]"; diff --git a/sandboxed_api/sandbox2/stack_trace.cc b/sandboxed_api/sandbox2/stack_trace.cc index c1eeb7b..e42b3d2 100644 --- a/sandboxed_api/sandbox2/stack_trace.cc +++ b/sandboxed_api/sandbox2/stack_trace.cc @@ -24,8 +24,10 @@ #include #include +#include "absl/cleanup/cleanup.h" #include "sandboxed_api/util/flag.h" #include "absl/memory/memory.h" +#include "absl/status/status.h" #include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" #include "absl/strings/strip.h" @@ -45,6 +47,7 @@ #include "sandboxed_api/sandbox2/util/bpf_helper.h" #include "sandboxed_api/util/fileops.h" #include "sandboxed_api/util/path.h" +#include "sandboxed_api/util/status_macros.h" ABSL_FLAG(bool, sandbox_disable_all_stack_traces, false, "Completely disable stack trace collection for sandboxees"); @@ -53,27 +56,34 @@ ABSL_FLAG(bool, sandbox_libunwind_crash_handler, true, "Sandbox libunwind when handling violations (preferred)"); namespace sandbox2 { +namespace { namespace file = ::sapi::file; namespace file_util = ::sapi::file_util; +// Similar to GetStackTrace() but without using the sandbox to isolate +// libunwind. +absl::StatusOr> UnsafeGetStackTrace(pid_t pid) { + LOG(WARNING) << "Using non-sandboxed libunwind"; + return RunLibUnwindAndSymbolizer(pid, kDefaultMaxFrames); +} + +} // namespace + class StackTracePeer { public: - static std::unique_ptr GetPolicy(pid_t target_pid, - const std::string& maps_file, - const std::string& app_path, - const std::string& exe_path, - const Mounts& mounts); + static absl::StatusOr> GetPolicy( + pid_t target_pid, const std::string& maps_file, + const std::string& app_path, const std::string& exe_path, + const Mounts& mounts); - static bool LaunchLibunwindSandbox(const Regs* regs, const Mounts& mounts, - UnwindResult* result); + static absl::StatusOr LaunchLibunwindSandbox( + const Regs* regs, const Mounts& mounts); }; -std::unique_ptr StackTracePeer::GetPolicy(pid_t target_pid, - const std::string& maps_file, - const std::string& app_path, - const std::string& exe_path, - const Mounts& mounts) { +absl::StatusOr> StackTracePeer::GetPolicy( + pid_t target_pid, const std::string& maps_file, const std::string& app_path, + const std::string& exe_path, const Mounts& mounts) { PolicyBuilder builder; builder // Use the mounttree of the original executable as starting point. @@ -147,23 +157,18 @@ std::unique_ptr StackTracePeer::GetPolicy(pid_t target_pid, } } - auto policy = builder.TryBuild(); - if (!policy.ok()) { - LOG(ERROR) << "Creating stack unwinder sandbox policy failed"; - return nullptr; - } + SAPI_ASSIGN_OR_RETURN(std::unique_ptr policy, builder.TryBuild()); 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 std::move(*policy); + policy->GetNamespace()->clone_flags_ = 0; + return std::move(policy); } -bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs, - const Mounts& mounts, - sandbox2::UnwindResult* result) { +absl::StatusOr StackTracePeer::LaunchLibunwindSandbox( + const Regs* regs, const Mounts& mounts) { const pid_t pid = regs->pid(); // Tell executor to use this special internal mode. @@ -183,8 +188,8 @@ bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs, char unwind_temp_directory_template[] = "/tmp/.sandbox2_unwind_XXXXXX"; char* unwind_temp_directory = mkdtemp(unwind_temp_directory_template); if (!unwind_temp_directory) { - LOG(WARNING) << "Could not create temporary directory for unwinding"; - return false; + return absl::InternalError( + "Could not create temporary directory for unwinding"); } struct UnwindTempDirectoryCleanup { ~UnwindTempDirectoryCleanup() { @@ -200,8 +205,7 @@ bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs, if (!file_util::fileops::CopyFile( file::JoinPath("/proc", absl::StrCat(pid), "maps"), unwind_temp_maps_path, 0400)) { - LOG(WARNING) << "Could not copy maps file"; - return false; + return absl::InternalError("Could not copy maps file"); } // Get path to the binary. @@ -211,8 +215,7 @@ bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs, std::string app_path; std::string proc_pid_exe = file::JoinPath("/proc", absl::StrCat(pid), "exe"); if (!file_util::fileops::ReadLinkAbsolute(proc_pid_exe, &app_path)) { - LOG(WARNING) << "Could not obtain absolute path to the binary"; - return false; + return absl::InternalError("Could not obtain absolute path to the binary"); } // The exe_path will have a mountable path of the application, even if it was @@ -227,8 +230,7 @@ bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs, // Create a copy of /proc/pid/exe, mount that one. exe_path = file::JoinPath(unwind_temp_directory, "exe"); if (!file_util::fileops::CopyFile(proc_pid_exe, exe_path, 0700)) { - LOG(WARNING) << "Could not copy /proc/pid/exe"; - return false; + return absl::InternalError("Could not copy /proc/pid/exe"); } } @@ -236,11 +238,9 @@ bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs, // Add mappings for the binary (as they might not have been added due to the // forkserver). - auto policy = StackTracePeer::GetPolicy(pid, unwind_temp_maps_path, app_path, - exe_path, mounts); - if (!policy) { - return false; - } + SAPI_ASSIGN_OR_RETURN(std::unique_ptr policy, + StackTracePeer::GetPolicy(pid, unwind_temp_maps_path, + app_path, exe_path, mounts)); Sandbox2 sandbox(std::move(executor), std::move(policy)); VLOG(1) << "Running libunwind sandbox"; @@ -253,37 +253,53 @@ bool StackTracePeer::LaunchLibunwindSandbox(const Regs* regs, sizeof(regs->user_regs_)); msg.set_default_max_frames(kDefaultMaxFrames); - bool success = true; - if (!comms->SendProtoBuf(msg)) { - LOG(ERROR) << "Sending libunwind setup message failed"; - success = false; - } - - if (success && !comms->RecvProtoBuf(result)) { - LOG(ERROR) << "Receiving libunwind result failed"; - success = false; - } - - if (!success) { + absl::Cleanup kill_sandbox = [&sandbox]() { sandbox.Kill(); + sandbox.AwaitResult().IgnoreResult(); + }; + + if (!comms->SendProtoBuf(msg)) { + return absl::InternalError("Sending libunwind setup message failed"); } + absl::Status status; + if (!comms->RecvStatus(&status)) { + return absl::InternalError( + "Receiving status from libunwind sandbox failed"); + } + if (!status.ok()) { + return status; + } + UnwindResult result; + if (!comms->RecvProtoBuf(&result)) { + return absl::InternalError("Receiving libunwind result failed"); + } + + std::move(kill_sandbox).Cancel(); + auto sandbox_result = sandbox.AwaitResult(); LOG(INFO) << "Libunwind execution status: " << sandbox_result.ToString(); - return success && sandbox_result.final_status() == Result::OK; + if (sandbox_result.final_status() != Result::OK) { + return absl::InternalError( + absl::StrCat("libunwind sandbox did not finish properly: ", + sandbox_result.ToString())); + } + + return result; } -std::vector GetStackTrace(const Regs* regs, const Mounts& mounts) { +absl::StatusOr> GetStackTrace(const Regs* regs, + const Mounts& mounts) { if constexpr (sapi::host_cpu::IsArm64()) { - return {"[Stack traces unavailable]"}; + return absl::UnavailableError("Stack traces unavailable on Aarch64"); } if (absl::GetFlag(FLAGS_sandbox_disable_all_stack_traces)) { - return {"[Stacktraces disabled]"}; + return absl::UnavailableError("Stacktraces disabled"); } if (!regs) { - LOG(WARNING) << "Could not obtain stacktrace, regs == nullptr"; - return {"[ERROR (noregs)]"}; + return absl::InvalidArgumentError( + "Could not obtain stacktrace, regs == nullptr"); } // Show a warning if sandboxed libunwind is requested but we're running in @@ -303,17 +319,10 @@ std::vector GetStackTrace(const Regs* regs, const Mounts& mounts) { return UnsafeGetStackTrace(regs->pid()); } - UnwindResult res; - if (!StackTracePeer::LaunchLibunwindSandbox(regs, mounts, &res)) { - return {}; - } - return {res.stacktrace().begin(), res.stacktrace().end()}; -} - -std::vector UnsafeGetStackTrace(pid_t pid) { - LOG(WARNING) << "Using non-sandboxed libunwind"; - std::vector ips; - return RunLibUnwindAndSymbolizer(pid, &ips, kDefaultMaxFrames); + SAPI_ASSIGN_OR_RETURN(UnwindResult res, + StackTracePeer::LaunchLibunwindSandbox(regs, mounts)); + return std::vector(res.stacktrace().begin(), + res.stacktrace().end()); } std::vector CompactStackTrace( diff --git a/sandboxed_api/sandbox2/stack_trace.h b/sandboxed_api/sandbox2/stack_trace.h index de948d4..00df829 100644 --- a/sandboxed_api/sandbox2/stack_trace.h +++ b/sandboxed_api/sandbox2/stack_trace.h @@ -19,15 +19,11 @@ #ifndef SANDBOXED_API_SANDBOX2_STACK_TRACE_H_ #define SANDBOXED_API_SANDBOX2_STACK_TRACE_H_ -#include - -#include -#include #include #include +#include "absl/status/statusor.h" #include "sandboxed_api/sandbox2/mounts.h" -#include "sandboxed_api/sandbox2/policy.h" #include "sandboxed_api/sandbox2/regs.h" namespace sandbox2 { @@ -36,11 +32,8 @@ namespace sandbox2 { constexpr size_t kDefaultMaxFrames = 200; // 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::vector UnsafeGetStackTrace(pid_t pid); +absl::StatusOr> GetStackTrace(const Regs* regs, + const Mounts& mounts); // Returns a stack trace that collapses duplicate stack frames and annotates // them with a repetition count. diff --git a/sandboxed_api/sandbox2/unwind/BUILD.bazel b/sandboxed_api/sandbox2/unwind/BUILD.bazel index f7e8834..e091a5b 100644 --- a/sandboxed_api/sandbox2/unwind/BUILD.bazel +++ b/sandboxed_api/sandbox2/unwind/BUILD.bazel @@ -59,7 +59,9 @@ cc_library( "//sandboxed_api/sandbox2/util:minielf", "//sandboxed_api/util:file_helpers", "//sandboxed_api/util:raw_logging", + "//sandboxed_api/util:status", "//sandboxed_api/util:strerror", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@org_gnu_libunwind//:unwind-ptrace-wrapped", ], diff --git a/sandboxed_api/sandbox2/unwind/CMakeLists.txt b/sandboxed_api/sandbox2/unwind/CMakeLists.txt index ec07ddd..d5b082c 100644 --- a/sandboxed_api/sandbox2/unwind/CMakeLists.txt +++ b/sandboxed_api/sandbox2/unwind/CMakeLists.txt @@ -30,6 +30,7 @@ add_library(sandbox2_unwind STATIC ) add_library(sandbox2::unwind ALIAS sandbox2_unwind) target_link_libraries(sandbox2_unwind PRIVATE + absl::statusor absl::strings sandbox2::comms sandbox2::maps_parser @@ -40,6 +41,7 @@ target_link_libraries(sandbox2_unwind PRIVATE sapi::base sapi::file_helpers sapi::raw_logging + sapi::status unwind::unwind_ptrace_wrapped ) diff --git a/sandboxed_api/sandbox2/unwind/unwind.cc b/sandboxed_api/sandbox2/unwind/unwind.cc index 8df0e42..0a12d2b 100644 --- a/sandboxed_api/sandbox2/unwind/unwind.cc +++ b/sandboxed_api/sandbox2/unwind/unwind.cc @@ -24,6 +24,7 @@ #include #include +#include "absl/status/statusor.h" #include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "libunwind-ptrace.h" @@ -34,6 +35,7 @@ #include "sandboxed_api/sandbox2/util/minielf.h" #include "sandboxed_api/util/file_helpers.h" #include "sandboxed_api/util/raw_logging.h" +#include "sandboxed_api/util/status_macros.h" #include "sandboxed_api/util/strerror.h" namespace sandbox2 { @@ -73,29 +75,77 @@ std::string GetSymbolAt(const std::map& addr_to_symbol, return ""; } -} // namespace +absl::StatusOr> LoadSymbolsMap(pid_t pid) { + const std::string maps_filename = absl::StrCat("/proc/", pid, "/maps"); + std::string maps_content; + SAPI_RETURN_IF_ERROR(sapi::file::GetContents(maps_filename, &maps_content, + sapi::file::Defaults())); -std::vector GetIPList(pid_t pid, int max_frames) { + SAPI_ASSIGN_OR_RETURN(std::vector maps, + ParseProcMaps(maps_content)); + + // 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 MapsEntry& entry : maps) { + if (!entry.is_executable || + entry.inode == 0 || // Only parse file-backed entries + entry.path.empty() || + absl::EndsWith(entry.path, " (deleted)") // Skip deleted files + ) { + continue; + } + + // Store details about start + end of this map. + // The maps entries are ordered and thus sorted with increasing adresses. + // This means if there is a symbol @ entry.end, it will be overwritten in + // the next iteration. + addr_to_symbol[entry.start] = absl::StrCat("map:", entry.path); + addr_to_symbol[entry.end] = ""; + + absl::StatusOr elf = + ElfFile::ParseFromFile(entry.path, ElfFile::kLoadSymbols); + if (!elf.ok()) { + SAPI_RAW_LOG(WARNING, "Could not load symbols for %s: %s", + entry.path.c_str(), + std::string(elf.status().message()).c_str()); + continue; + } + + for (const ElfFile::Symbol& symbol : elf->symbols()) { + if (elf->position_independent()) { + if (symbol.address < entry.end - entry.start) { + addr_to_symbol[symbol.address + entry.start] = symbol.name; + } + } else { + if (symbol.address >= entry.start && symbol.address < entry.end) { + addr_to_symbol[symbol.address] = symbol.name; + } + } + } + } + return addr_to_symbol; +} + +absl::StatusOr> RunLibUnwind(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 absl::InternalError("unw_create_addr_space() failed"); } std::unique_ptr ui( reinterpret_cast(_UPT_create(pid)), _UPT_destroy); if (ui == nullptr) { - SAPI_RAW_LOG(WARNING, "_UPT_create() failed"); - return {}; + return absl::InternalError("_UPT_create() failed"); } 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); - return {}; + return absl::InternalError( + absl::StrCat("unw_init_remote() failed with error ", rc)); } std::vector ips; @@ -120,6 +170,22 @@ std::vector GetIPList(pid_t pid, int max_frames) { return ips; } +absl::StatusOr> SymbolizeStacktrace( + pid_t pid, const std::vector& ips) { + SAPI_ASSIGN_OR_RETURN(auto addr_to_symbol, LoadSymbolsMap(pid)); + std::vector stack_trace; + stack_trace.reserve(ips.size()); + // Symbolize stacktrace + for (uintptr_t ip : ips) { + const std::string symbol = + GetSymbolAt(addr_to_symbol, static_cast(ip)); + stack_trace.push_back(absl::StrCat(symbol, "(0x", absl::Hex(ip), ")")); + } + return stack_trace; +} + +} // namespace + bool RunLibUnwindAndSymbolizer(Comms* comms) { UnwindSetup setup; if (!comms->RecvProtoBuf(&setup)) { @@ -128,87 +194,34 @@ bool RunLibUnwindAndSymbolizer(Comms* comms) { EnablePtraceEmulationWithUserRegs(setup.regs()); - std::vector ips; - std::vector stack_trace = - RunLibUnwindAndSymbolizer(setup.pid(), &ips, setup.default_max_frames()); + absl::StatusOr> ips = + RunLibUnwind(setup.pid(), setup.default_max_frames()); + absl::StatusOr> stack_trace; + if (ips.ok()) { + stack_trace = SymbolizeStacktrace(setup.pid(), *ips); + } else { + stack_trace = ips.status(); + } + + if (!comms->SendStatus(stack_trace.status())) { + return false; + } + + if (!stack_trace.ok()) { + return true; + } UnwindResult msg; - *msg.mutable_stacktrace() = {stack_trace.begin(), stack_trace.end()}; - *msg.mutable_ip() = {ips.begin(), ips.end()}; + *msg.mutable_stacktrace() = {stack_trace->begin(), stack_trace->end()}; + *msg.mutable_ip() = {ips->begin(), ips->end()}; return comms->SendProtoBuf(msg); } -std::vector RunLibUnwindAndSymbolizer(pid_t pid, - std::vector* ips, - int max_frames) { - *ips = GetIPList(pid, max_frames); // Uses libunwind - - const std::string maps_filename = absl::StrCat("/proc/", pid, "/maps"); - std::string maps_content; - if (auto status = sapi::file::GetContents(maps_filename, &maps_content, - sapi::file::Defaults()); - !status.ok()) { - SAPI_RAW_LOG(ERROR, "%s", status.ToString().c_str()); - return {}; - } - - auto maps = ParseProcMaps(maps_content); - if (!maps.ok()) { - SAPI_RAW_LOG(ERROR, "Could not parse file: %s", maps_filename.c_str()); - return {}; - } - - // 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) { - if (!entry.path.empty()) { - // Store details about start + end of this map. - // The maps entries are ordered and thus sorted with increasing adresses. - // This means if there is a symbol @ entry.end, it will be overwritten in - // the next iteration. - addr_to_symbol[entry.start] = absl::StrCat("map:", entry.path); - addr_to_symbol[entry.end] = ""; - } - - if (!entry.is_executable || - entry.inode == 0 || // Only parse file-backed entries - entry.path.empty() || - absl::EndsWith(entry.path, " (deleted)") // Skip deleted files - ) { - continue; - } - - auto elf = ElfFile::ParseFromFile(entry.path, ElfFile::kLoadSymbols); - if (!elf.ok()) { - SAPI_RAW_LOG(WARNING, "Could not load symbols for %s: %s", - entry.path.c_str(), - std::string(elf.status().message()).c_str()); - continue; - } - - for (const auto& symbol : elf->symbols()) { - if (elf->position_independent()) { - if (symbol.address < entry.end - entry.start) { - addr_to_symbol[symbol.address + entry.start] = symbol.name; - } - } else { - if (symbol.address >= entry.start && symbol.address < entry.end) { - addr_to_symbol[symbol.address] = symbol.name; - } - } - } - } - - std::vector stack_trace; - stack_trace.reserve(ips->size()); - // Symbolize stacktrace - 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), ")")); - } - return stack_trace; +absl::StatusOr> RunLibUnwindAndSymbolizer( + pid_t pid, int max_frames) { + SAPI_ASSIGN_OR_RETURN(std::vector ips, + RunLibUnwind(pid, max_frames)); + return SymbolizeStacktrace(pid, ips); } } // namespace sandbox2 diff --git a/sandboxed_api/sandbox2/unwind/unwind.h b/sandboxed_api/sandbox2/unwind/unwind.h index 883593c..b9c4638 100644 --- a/sandboxed_api/sandbox2/unwind/unwind.h +++ b/sandboxed_api/sandbox2/unwind/unwind.h @@ -21,6 +21,7 @@ #include #include +#include "absl/status/statusor.h" #include "sandboxed_api/sandbox2/comms.h" namespace sandbox2 { @@ -28,9 +29,8 @@ namespace sandbox2 { // Runs libunwind and the symbolizer and sends the results via comms. bool RunLibUnwindAndSymbolizer(Comms* comms); -std::vector RunLibUnwindAndSymbolizer(pid_t pid, - std::vector* ips, - int max_frames); +absl::StatusOr> RunLibUnwindAndSymbolizer( + pid_t pid, int max_frames); } // namespace sandbox2