From 227daf4a4240a9e6fe22c9f3e1a8aeefe739b92e Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Tue, 19 Sep 2023 06:49:17 -0700 Subject: [PATCH] Do 1 level of recursion on libunwind crashes PiperOrigin-RevId: 566617450 Change-Id: If5e3ce2e9763360c6cbd50145c432dfb62621136 --- cmake/abseil-cpp.cmake | 3 + sandboxed_api/sandbox2/BUILD.bazel | 2 + sandboxed_api/sandbox2/CMakeLists.txt | 2 + sandboxed_api/sandbox2/executor.h | 6 +- sandboxed_api/sandbox2/monitor_base.cc | 5 +- sandboxed_api/sandbox2/stack_trace.cc | 18 +++--- sandboxed_api/sandbox2/stack_trace.h | 5 +- sandboxed_api/sandbox2/stack_trace_test.cc | 71 ++++++++++++++++++++++ 8 files changed, 101 insertions(+), 11 deletions(-) diff --git a/cmake/abseil-cpp.cmake b/cmake/abseil-cpp.cmake index 9b7e73a..e89df42 100644 --- a/cmake/abseil-cpp.cmake +++ b/cmake/abseil-cpp.cmake @@ -19,6 +19,9 @@ FetchContent_Declare(absl set(ABSL_CXX_STANDARD ${SAPI_CXX_STANDARD} CACHE STRING "" FORCE) set(ABSL_PROPAGATE_CXX_STD ON CACHE BOOL "" FORCE) set(ABSL_RUN_TESTS OFF CACHE BOOL "" FORCE) +set(ABSL_BUILD_TEST_HELPERS ON CACHE BOOL "" FORCE) +set(ABSL_USE_EXTERNAL_GOOGLETEST ON) +set(ABSL_FIND_GOOGLETEST OFF) set(ABSL_USE_GOOGLETEST_HEAD OFF CACHE BOOL "" FORCE) FetchContent_MakeAvailable(absl) diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index 04d797c..9e0ade3 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/sandboxed_api/sandbox2/BUILD.bazel @@ -1032,9 +1032,11 @@ cc_test( "//sandboxed_api:testing", "//sandboxed_api/util:fileops", "//sandboxed_api/util:status_matchers", + "@com_google_absl//absl/base:log_severity", "@com_google_absl//absl/flags:flag", "@com_google_absl//absl/flags:reflection", "@com_google_absl//absl/log:check", + "@com_google_absl//absl/log:scoped_mock_log", "@com_google_absl//absl/strings", "@com_google_absl//absl/time", "@com_google_googletest//:gtest_main", diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 4966d1d..116f609 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -1118,6 +1118,8 @@ if(BUILD_TESTING AND SAPI_BUILD_TESTING) target_link_libraries(sandbox2_stack_trace_test PRIVATE absl::check absl::flags + absl::log_severity + absl::scoped_mock_log absl::status absl::strings absl::time diff --git a/sandboxed_api/sandbox2/executor.h b/sandboxed_api/sandbox2/executor.h index 5c2fb84..6f0ba0e 100644 --- a/sandboxed_api/sandbox2/executor.h +++ b/sandboxed_api/sandbox2/executor.h @@ -97,6 +97,8 @@ class Executor final { return *this; } + int libunwind_recursion_depth() { return libunwind_recursion_depth_; } + private: friend class MonitorBase; friend class PtraceMonitor; @@ -104,8 +106,9 @@ class Executor final { // Internal constructor for executing libunwind on the given pid // enable_sandboxing_pre_execve=false as we are not going to execve. - explicit Executor(pid_t libunwind_sbox_for_pid) + explicit Executor(pid_t libunwind_sbox_for_pid, int libunwind_recursion_depth) : libunwind_sbox_for_pid_(libunwind_sbox_for_pid), + libunwind_recursion_depth_(libunwind_recursion_depth), enable_sandboxing_pre_execve_(false) { CHECK_GE(libunwind_sbox_for_pid_, 0); SetUpServerSideCommsFd(); @@ -131,6 +134,7 @@ class Executor final { // If this executor is running the libunwind sandbox for a process, // this variable will hold the PID of the process. Otherwise it is zero. pid_t libunwind_sbox_for_pid_ = 0; + int libunwind_recursion_depth_ = 0; // Should the sandboxing be enabled before execve() occurs, or the binary will // do it by itself, using the Client object's methods diff --git a/sandboxed_api/sandbox2/monitor_base.cc b/sandboxed_api/sandbox2/monitor_base.cc index 4ab5faa..8423415 100644 --- a/sandboxed_api/sandbox2/monitor_base.cc +++ b/sandboxed_api/sandbox2/monitor_base.cc @@ -401,7 +401,7 @@ bool MonitorBase::StackTraceCollectionPossible() const { // recursion). if ((policy_->GetNamespace() || absl::GetFlag(FLAGS_sandbox_libunwind_crash_handler) == false) && - executor_->libunwind_sbox_for_pid_ == 0) { + executor_->libunwind_recursion_depth() <= 1) { return true; } LOG(ERROR) << "Cannot collect stack trace. Unwind pid " @@ -443,7 +443,8 @@ bool MonitorBase::ShouldCollectStackTrace(Result::StatusEnum status) const { absl::StatusOr> MonitorBase::GetStackTrace( const Regs* regs) { return sandbox2::GetStackTrace(regs, policy_->GetNamespaceOrNull(), - uses_custom_forkserver_); + uses_custom_forkserver_, + executor_->libunwind_recursion_depth() + 1); } absl::StatusOr> MonitorBase::GetAndLogStackTrace( diff --git a/sandboxed_api/sandbox2/stack_trace.cc b/sandboxed_api/sandbox2/stack_trace.cc index ec41539..2b970ad 100644 --- a/sandboxed_api/sandbox2/stack_trace.cc +++ b/sandboxed_api/sandbox2/stack_trace.cc @@ -97,7 +97,8 @@ class StackTracePeer { const Namespace* ns, bool uses_custom_forkserver); static absl::StatusOr> LaunchLibunwindSandbox( - const Regs* regs, const Namespace* ns, bool uses_custom_forkserver); + const Regs* regs, const Namespace* ns, bool uses_custom_forkserver, + int recursion_depth); }; absl::StatusOr> StackTracePeer::GetPolicy( @@ -185,7 +186,8 @@ SandboxPeer::SpawnFn SandboxPeer::spawn_fn_ = nullptr; } // namespace internal absl::StatusOr> StackTracePeer::LaunchLibunwindSandbox( - const Regs* regs, const Namespace* ns, bool uses_custom_forkserver) { + const Regs* regs, const Namespace* ns, bool uses_custom_forkserver, + int recursion_depth) { const pid_t pid = regs->pid(); sapi::file_util::fileops::FDCloser memory_fd( @@ -195,7 +197,7 @@ absl::StatusOr> StackTracePeer::LaunchLibunwindSandbox( } // Tell executor to use this special internal mode. Using `new` to access a // non-public constructor. - auto executor = absl::WrapUnique(new Executor(pid)); + auto executor = absl::WrapUnique(new Executor(pid, recursion_depth)); executor->limits()->set_rlimit_cpu(10).set_walltime_limit(absl::Seconds(5)); @@ -275,7 +277,8 @@ absl::StatusOr> StackTracePeer::LaunchLibunwindSandbox( absl::Cleanup kill_sandbox = [&sandbox]() { sandbox->Kill(); - sandbox->AwaitResult().IgnoreResult(); + sandbox2::Result result = sandbox->AwaitResult(); + LOG(INFO) << "Libunwind execution status: " << result.ToString(); }; if (!comms->SendProtoBuf(msg)) { @@ -313,7 +316,8 @@ absl::StatusOr> StackTracePeer::LaunchLibunwindSandbox( } absl::StatusOr> GetStackTrace( - const Regs* regs, const Namespace* ns, bool uses_custom_forkserver) { + const Regs* regs, const Namespace* ns, bool uses_custom_forkserver, + int recursion_depth) { if (absl::GetFlag(FLAGS_sandbox_disable_all_stack_traces)) { return absl::UnavailableError("Stacktraces disabled"); } @@ -333,8 +337,8 @@ absl::StatusOr> GetStackTrace( return UnsafeGetStackTrace(regs->pid()); } - return StackTracePeer::LaunchLibunwindSandbox(regs, ns, - uses_custom_forkserver); + return StackTracePeer::LaunchLibunwindSandbox( + regs, ns, uses_custom_forkserver, recursion_depth); } std::vector CompactStackTrace( diff --git a/sandboxed_api/sandbox2/stack_trace.h b/sandboxed_api/sandbox2/stack_trace.h index 42a40df..a37af00 100644 --- a/sandboxed_api/sandbox2/stack_trace.h +++ b/sandboxed_api/sandbox2/stack_trace.h @@ -37,6 +37,7 @@ namespace sandbox2 { class Sandbox2; +class StackTraceTestPeer; namespace internal { @@ -56,6 +57,7 @@ class SandboxPeer { private: friend class ::sandbox2::Sandbox2; + friend class ::sandbox2::StackTraceTestPeer; using SpawnFn = std::unique_ptr (*)(std::unique_ptr, std::unique_ptr); static SpawnFn spawn_fn_; @@ -68,7 +70,8 @@ constexpr size_t kDefaultMaxFrames = 200; // Returns the stack-trace of the PID=pid, one line per frame. absl::StatusOr> GetStackTrace( - const Regs* regs, const Namespace* ns, bool uses_custom_forkserver); + const Regs* regs, const Namespace* ns, bool uses_custom_forkserver, + int recursion_depth); // Returns a stack trace that collapses duplicate stack frames and annotates // them with a repetition count. diff --git a/sandboxed_api/sandbox2/stack_trace_test.cc b/sandboxed_api/sandbox2/stack_trace_test.cc index 433b7bf..6940953 100644 --- a/sandboxed_api/sandbox2/stack_trace_test.cc +++ b/sandboxed_api/sandbox2/stack_trace_test.cc @@ -25,10 +25,12 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "absl/base/log_severity.h" #include "absl/flags/declare.h" #include "absl/flags/flag.h" #include "absl/flags/reflection.h" #include "absl/log/check.h" +#include "absl/log/scoped_mock_log.h" #include "absl/strings/str_cat.h" #include "absl/time/time.h" #include "sandboxed_api/sandbox2/executor.h" @@ -44,12 +46,57 @@ ABSL_DECLARE_FLAG(bool, sandbox_libunwind_crash_handler); namespace sandbox2 { + +class StackTraceTestPeer { + public: + static StackTraceTestPeer& GetInstance() { + static auto* peer = new StackTraceTestPeer(); + return *peer; + } + std::unique_ptr SpawnFn( + std::unique_ptr executor, std::unique_ptr policy) { + if (crash_unwind_) { + policy = PolicyBuilder().BuildOrDie(); + crash_unwind_ = false; + } + return old_spawn_fn_(std::move(executor), std::move(policy)); + } + void ReplaceSpawnFn() { + old_spawn_fn_ = internal::SandboxPeer::spawn_fn_; + internal::SandboxPeer::spawn_fn_ = +[](std::unique_ptr executor, + std::unique_ptr policy) { + return GetInstance().SpawnFn(std::move(executor), std::move(policy)); + }; + } + void RestoreSpawnFn() { internal::SandboxPeer::spawn_fn_ = old_spawn_fn_; } + void CrashNextUnwind() { crash_unwind_ = true; } + + private: + internal::SandboxPeer::SpawnFn old_spawn_fn_; + bool crash_unwind_ = false; +}; + +struct ScopedSpawnOverride { + ScopedSpawnOverride() { StackTraceTestPeer::GetInstance().ReplaceSpawnFn(); } + ~ScopedSpawnOverride() { StackTraceTestPeer::GetInstance().RestoreSpawnFn(); } + ScopedSpawnOverride(ScopedSpawnOverride&&) = delete; + ScopedSpawnOverride& operator=(ScopedSpawnOverride&&) = delete; + ScopedSpawnOverride(const ScopedSpawnOverride&) = delete; + ScopedSpawnOverride& operator=(const ScopedSpawnOverride&) = delete; + + void CrashNextUnwind() { + StackTraceTestPeer::GetInstance().CrashNextUnwind(); + } +}; + namespace { namespace file_util = ::sapi::file_util; using ::sapi::CreateDefaultPermissiveTestPolicy; using ::sapi::GetTestSourcePath; +using ::testing::_; using ::testing::Contains; +using ::testing::ContainsRegex; using ::testing::ElementsAre; using ::testing::Eq; using ::testing::IsEmpty; @@ -213,6 +260,30 @@ TEST(StackTraceTest, CompactStackTrace) { "(previous frame repeated 3 times)")); } +TEST(StackTraceTest, RecursiveStackTrace) { + // Very first sandbox run will initialize spawn_fn_ + SKIP_SANITIZERS; + ScopedSpawnOverride spawn_override; + SymbolizationWorksCommon({}); + absl::ScopedMockLog log; + EXPECT_CALL( + log, + Log(absl::LogSeverity::kInfo, _, + ContainsRegex( + "Libunwind execution status: SYSCALL VIOLATION.*Stack: \\w+"))); + spawn_override.CrashNextUnwind(); + const std::string path = GetTestSourcePath("sandbox2/testcases/symbolize"); + std::vector args = {path, absl::StrCat(1), absl::StrCat(1)}; + PolicyBuilder builder = CreateDefaultPermissiveTestPolicy(path); + SAPI_ASSERT_OK_AND_ASSIGN(auto policy, builder.TryBuild()); + + Sandbox2 s2(std::make_unique(path, args), std::move(policy)); + log.StartCapturingLogs(); + ASSERT_TRUE(s2.RunAsync()); + auto result = s2.AwaitResult(); + EXPECT_THAT(result.final_status(), Eq(Result::SIGNALED)); +} + INSTANTIATE_TEST_SUITE_P( Instantiation, StackTraceTest, ::testing::Values(