From 47dcb516c397c904be38f4bc2b4ce6221cef88c3 Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Thu, 12 Dec 2024 01:39:01 -0800 Subject: [PATCH] Automated rollback of commit f06ee44f248fd08ccd42817f97c54b711cb3dec1. PiperOrigin-RevId: 705414332 Change-Id: I420eccf475bde17f28d1f54d9106407a7f5f0c24 --- sandboxed_api/sandbox2/CMakeLists.txt | 6 +- sandboxed_api/sandbox2/monitor_ptrace.cc | 74 ++++++++++++++---------- sandboxed_api/sandbox2/monitor_ptrace.h | 20 +++---- 3 files changed, 56 insertions(+), 44 deletions(-) diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index b3cbb3c4..c0af50a3 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -470,7 +470,8 @@ 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::cleanup + PRIVATE absl::core_headers + absl::cleanup absl::flat_hash_set absl::flags absl::log @@ -485,15 +486,14 @@ 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 cc8533b2..3305d682 100644 --- a/sandboxed_api/sandbox2/monitor_ptrace.cc +++ b/sandboxed_api/sandbox2/monitor_ptrace.cc @@ -200,13 +200,14 @@ bool PtraceMonitor::InterruptSandboxee() { #define __WPTRACEEVENT(x) ((x & 0xff0000) >> 16) void PtraceMonitor::NotifyMonitor() { - absl::MutexLock lock(¬ify_mutex_); - pid_waiter_.SetDeadline(absl::InfinitePast()); - notified_ = true; + absl::ReaderMutexLock lock(¬ify_mutex_); + if (thread_.IsJoinable()) { + pthread_kill(thread_.handle(), SIGCHLD); + } } void PtraceMonitor::Join() { - absl::MutexLock lock(&thread_mutex_); + absl::MutexLock lock(¬ify_mutex_); if (thread_.IsJoinable()) { thread_.Join(); CHECK(IsDone()) << "Monitor did not terminate"; @@ -216,10 +217,7 @@ void PtraceMonitor::Join() { } void PtraceMonitor::RunInternal() { - { - absl::MutexLock lock(&thread_mutex_); - thread_ = sapi::Thread(this, &PtraceMonitor::Run, "sandbox2-Monitor"); - } + thread_ = sapi::Thread(this, &PtraceMonitor::Run, "sandbox2-Monitor"); // Wait for the Monitor to set-up the sandboxee correctly (or fail while // doing that). From here on, it is safe to use the IPC object for @@ -234,6 +232,12 @@ 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. @@ -247,15 +251,11 @@ void PtraceMonitor::Run() { std::move(setup_notify).Invoke(); bool sandboxee_exited = false; - pid_waiter_.SetPriorityPid(process_.main_pid); + PidWaiter pid_waiter(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,18 +295,13 @@ void PtraceMonitor::Run() { break; } } - 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); + + 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; } @@ -315,7 +310,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 if (errno != EINTR) { + } else { PLOG(ERROR) << "waitpid() failed"; } continue; @@ -379,17 +374,14 @@ 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; } - { - absl::MutexLock lock(¬ify_mutex_); - pid_waiter_.SetDeadline(deadline); - } - pid_t ret = pid_waiter_.Wait(&status); + pid_t ret = pid_waiter.Wait(&status); if (ret == -1) { if (!log_stack_traces || ret != ECHILD) { PLOG(ERROR) << "waitpid() failed"; @@ -405,6 +397,8 @@ void PtraceMonitor::Run() { } if (ret == 0) { + auto ts = absl::ToTimespec(left); + sigtimedwait(&sset_, nullptr, &ts); continue; } @@ -440,6 +434,26 @@ 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 3e9b263d..1790dbcc 100644 --- a/sandboxed_api/sandbox2/monitor_ptrace.h +++ b/sandboxed_api/sandbox2/monitor_ptrace.h @@ -22,7 +22,6 @@ #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" @@ -35,7 +34,6 @@ #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 { @@ -64,11 +62,14 @@ 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; @@ -126,6 +127,9 @@ 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(); @@ -155,18 +159,12 @@ 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_; - // Synchronizes joining the monitor thread. - absl::Mutex thread_mutex_; // Monitor thread object. - sapi::Thread ABSL_GUARDED_BY(thread_mutex_) thread_; + sapi::Thread thread_; - // Synchronizes deadline setting and notifying the monitor. + // Synchronizes monitor thread deletion and notifying the monitor. absl::Mutex notify_mutex_; - // True iff monitor thread is notified - bool notified_ ABSL_GUARDED_BY(notify_mutex_) = false; }; } // namespace sandbox2