From f06ee44f248fd08ccd42817f97c54b711cb3dec1 Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Tue, 10 Dec 2024 05:45:14 -0800 Subject: [PATCH] PtraceMonitor: Use deadline manager instead of sigtimedwait As SIGCHLD is process directed in case where there are many sandboxees running it is not reliably delivered (any monitor can get it). Thus the whole relies on the monitors actually polling the status every 500ms. This causes bigger latency and unneccessary CPU load. PiperOrigin-RevId: 704670528 Change-Id: I1e7f8a116e271d61d357eab41f7f31ebbfe5417c --- sandboxed_api/sandbox2/CMakeLists.txt | 6 +- sandboxed_api/sandbox2/monitor_ptrace.cc | 68 +++++++++-------------- sandboxed_api/sandbox2/monitor_ptrace.h | 16 +++--- sandboxed_api/sandbox2/sandbox2_test.cc | 3 + sandboxed_api/sandbox2/util/pid_waiter.cc | 2 + 5 files changed, 41 insertions(+), 54 deletions(-) diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 36679126..655ef94e 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -470,8 +470,7 @@ add_library(sandbox2_monitor_ptrace ${SAPI_LIB_TYPE} ) add_library(sandbox2::monitor_ptrace ALIAS sandbox2_monitor_ptrace) target_link_libraries(sandbox2_monitor_ptrace - PRIVATE absl::core_headers - absl::cleanup + PRIVATE absl::cleanup absl::flat_hash_set absl::flags absl::log @@ -486,14 +485,15 @@ target_link_libraries(sandbox2_monitor_ptrace sapi::status sandbox2::client sandbox2::comms - sandbox2::pid_waiter sandbox2::result sandbox2::sanitizer sandbox2::util PUBLIC absl::check + absl::core_headers sandbox2::executor sandbox2::monitor_base sandbox2::notify + sandbox2::pid_waiter sandbox2::policy sandbox2::regs sandbox2::syscall diff --git a/sandboxed_api/sandbox2/monitor_ptrace.cc b/sandboxed_api/sandbox2/monitor_ptrace.cc index 3305d682..48068b02 100644 --- a/sandboxed_api/sandbox2/monitor_ptrace.cc +++ b/sandboxed_api/sandbox2/monitor_ptrace.cc @@ -200,14 +200,12 @@ bool PtraceMonitor::InterruptSandboxee() { #define __WPTRACEEVENT(x) ((x & 0xff0000) >> 16) void PtraceMonitor::NotifyMonitor() { - absl::ReaderMutexLock lock(¬ify_mutex_); - if (thread_.IsJoinable()) { - pthread_kill(thread_.handle(), SIGCHLD); - } + absl::MutexLock lock(¬ify_mutex_); + pid_waiter_.SetDeadline(absl::InfinitePast()); + notified_ = true; } void PtraceMonitor::Join() { - absl::MutexLock lock(¬ify_mutex_); if (thread_.IsJoinable()) { thread_.Join(); CHECK(IsDone()) << "Monitor did not terminate"; @@ -232,12 +230,6 @@ void PtraceMonitor::Run() { }; absl::Cleanup setup_notify = [this] { setup_notification_.Notify(); }; - // It'd be costly to initialize the sigset_t for each sigtimedwait() - // invocation, so do it once per Monitor. - if (!InitSetupSignals()) { - SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SIGNALS); - return; - } // This call should be the last in the init sequence, because it can cause the // sandboxee to enter ptrace-stopped state, in which it will not be able to // send any messages over the Comms channel. @@ -251,11 +243,15 @@ void PtraceMonitor::Run() { std::move(setup_notify).Invoke(); bool sandboxee_exited = false; - PidWaiter pid_waiter(process_.main_pid); + pid_waiter_.SetPriorityPid(process_.main_pid); int status; // All possible still running children of main process, will be killed due to // PTRACE_O_EXITKILL ptrace() flag. while (result().final_status() == Result::UNSET) { + { + absl::MutexLock lock(¬ify_mutex_); + notified_ = false; + } if (absl::Now() >= hard_deadline_) { LOG(WARNING) << "Hard deadline exceeded (timed_out=" << timed_out_ << ", external_kill=" << external_kill_ @@ -295,13 +291,18 @@ void PtraceMonitor::Run() { break; } } - - pid_t ret = pid_waiter.Wait(&status); + absl::Time effective_deadline = hard_deadline_; + if (deadline != 0 && hard_deadline_ == absl::InfiniteFuture()) { + effective_deadline = absl::FromUnixMillis(deadline); + } + { + absl::MutexLock lock(¬ify_mutex_); + if (!notified_) { + pid_waiter_.SetDeadline(effective_deadline); + } + } + pid_t ret = pid_waiter_.Wait(&status); if (ret == 0) { - constexpr timespec ts = {kWakeUpPeriodSec, kWakeUpPeriodNSec}; - int signo = sigtimedwait(&sset_, nullptr, &ts); - LOG_IF(ERROR, signo != -1 && signo != SIGCHLD) - << "Unknown signal received: " << signo; continue; } @@ -310,7 +311,7 @@ void PtraceMonitor::Run() { LOG(ERROR) << "PANIC(). The main process has not exited yet, " << "yet we haven't seen its exit event"; SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_CHILD); - } else { + } else if (errno != EINTR) { PLOG(ERROR) << "waitpid() failed"; } continue; @@ -374,14 +375,17 @@ void PtraceMonitor::Run() { absl::GetFlag(FLAGS_sandbox2_stack_traces_collection_timeout); } for (;;) { - auto left = deadline - absl::Now(); if (absl::Now() >= deadline) { LOG(WARNING) << "Waiting for sandboxee exit timed out. Sandboxee result: " << result_.ToString(); break; } - pid_t ret = pid_waiter.Wait(&status); + { + absl::MutexLock lock(¬ify_mutex_); + pid_waiter_.SetDeadline(deadline); + } + pid_t ret = pid_waiter_.Wait(&status); if (ret == -1) { if (!log_stack_traces || ret != ECHILD) { PLOG(ERROR) << "waitpid() failed"; @@ -397,8 +401,6 @@ void PtraceMonitor::Run() { } if (ret == 0) { - auto ts = absl::ToTimespec(left); - sigtimedwait(&sset_, nullptr, &ts); continue; } @@ -434,26 +436,6 @@ void PtraceMonitor::LogStackTraceOfPid(pid_t pid) { } } -bool PtraceMonitor::InitSetupSignals() { - if (sigemptyset(&sset_) == -1) { - PLOG(ERROR) << "sigemptyset()"; - return false; - } - - // sigtimedwait will react (wake-up) to arrival of this signal. - if (sigaddset(&sset_, SIGCHLD) == -1) { - PLOG(ERROR) << "sigaddset(SIGCHLD)"; - return false; - } - - if (pthread_sigmask(SIG_BLOCK, &sset_, nullptr) == -1) { - PLOG(ERROR) << "pthread_sigmask(SIG_BLOCK, SIGCHLD)"; - return false; - } - - return true; -} - absl::Status TryAttach(const absl::flat_hash_set& tasks, absl::Time deadline, absl::flat_hash_set& tasks_attached) { diff --git a/sandboxed_api/sandbox2/monitor_ptrace.h b/sandboxed_api/sandbox2/monitor_ptrace.h index 1790dbcc..d2918134 100644 --- a/sandboxed_api/sandbox2/monitor_ptrace.h +++ b/sandboxed_api/sandbox2/monitor_ptrace.h @@ -22,6 +22,7 @@ #include #include +#include "absl/base/thread_annotations.h" #include "absl/container/flat_hash_map.h" #include "absl/log/log.h" #include "absl/synchronization/mutex.h" @@ -34,6 +35,7 @@ #include "sandboxed_api/sandbox2/policy.h" #include "sandboxed_api/sandbox2/regs.h" #include "sandboxed_api/sandbox2/syscall.h" +#include "sandboxed_api/sandbox2/util/pid_waiter.h" #include "sandboxed_api/util/thread.h" namespace sandbox2 { @@ -62,14 +64,11 @@ class PtraceMonitor : public MonitorBase { absl::Time deadline = absl::Now() + limit; deadline_millis_.store(absl::ToUnixMillis(deadline), std::memory_order_relaxed); + NotifyMonitor(); } } private: - // Timeout used with sigtimedwait (0.5s). - static constexpr int kWakeUpPeriodSec = 0L; - static constexpr int kWakeUpPeriodNSec = (500L * 1000L * 1000L); - // Waits for events from monitored clients and signals from the main process. void RunInternal() override; void Join() override; @@ -127,9 +126,6 @@ class PtraceMonitor : public MonitorBase { // Returns false if an error occurred and process could not be interrupted. bool InterruptSandboxee(); - // Sets up required signal masks/handlers; prepare mask for sigtimedwait(). - bool InitSetupSignals(); - // ptrace(PTRACE_SEIZE) to the Client. // Returns success/failure status. bool InitPtraceAttach(); @@ -159,12 +155,16 @@ class PtraceMonitor : public MonitorBase { sigset_t sset_; // Deadline after which sandboxee get terminated via PTRACE_O_EXITKILL. absl::Time hard_deadline_ = absl::InfiniteFuture(); + // PidWaiter for waiting for sandboxee events. + PidWaiter pid_waiter_; // Monitor thread object. sapi::Thread thread_; - // Synchronizes monitor thread deletion and notifying the monitor. + // Synchronizes deadline setting and notifying the monitor. absl::Mutex notify_mutex_; + // True iff monitor thread is notified + bool notified_ ABSL_GUARDED_BY(notify_mutex_) = false; }; } // namespace sandbox2 diff --git a/sandboxed_api/sandbox2/sandbox2_test.cc b/sandboxed_api/sandbox2/sandbox2_test.cc index 47d704e6..9094b220 100644 --- a/sandboxed_api/sandbox2/sandbox2_test.cc +++ b/sandboxed_api/sandbox2/sandbox2_test.cc @@ -182,10 +182,13 @@ TEST_P(Sandbox2Test, SandboxeeTimeoutDisabledStacktraces) { .TryBuild()); Sandbox2 sandbox(std::move(executor), std::move(policy)); ASSERT_THAT(SetUpSandbox(&sandbox), IsOk()); + absl::Time start_time = absl::Now(); ASSERT_TRUE(sandbox.RunAsync()); sandbox.set_walltime_limit(absl::Seconds(1)); auto result = sandbox.AwaitResult(); EXPECT_EQ(result.final_status(), Result::TIMEOUT); + auto elapsed = absl::Now() - start_time; + EXPECT_THAT(elapsed, Lt(absl::Seconds(2))); EXPECT_THAT(result.stack_trace(), IsEmpty()); } diff --git a/sandboxed_api/sandbox2/util/pid_waiter.cc b/sandboxed_api/sandbox2/util/pid_waiter.cc index 2951d4ca..13b06041 100644 --- a/sandboxed_api/sandbox2/util/pid_waiter.cc +++ b/sandboxed_api/sandbox2/util/pid_waiter.cc @@ -14,6 +14,8 @@ #include "sandboxed_api/sandbox2/util/pid_waiter.h" +#include + #include #include