From 2430bc8ae80c3e74f041fefceca7ad0ae0cd485d Mon Sep 17 00:00:00 2001
From: Wiktor Garbacz <wiktorg@google.com>
Date: Tue, 27 Feb 2024 04:35:44 -0800
Subject: [PATCH] Use sandboxed libunwind also with sanitizers

PiperOrigin-RevId: 610710893
Change-Id: Iea2c103e88a848b40c5c5cbf3c9f6b9d7bf166db
---
 sandboxed_api/sandbox2/stack_trace.cc         | 34 +++++++++----------
 sandboxed_api/sandbox2/stack_trace_test.cc    |  7 ++++
 sandboxed_api/sandbox2/testcases/symbolize.cc | 13 +++++++
 3 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/sandboxed_api/sandbox2/stack_trace.cc b/sandboxed_api/sandbox2/stack_trace.cc
index 11f9b6f..e84b2f8 100644
--- a/sandboxed_api/sandbox2/stack_trace.cc
+++ b/sandboxed_api/sandbox2/stack_trace.cc
@@ -66,6 +66,10 @@ namespace {
 namespace file = ::sapi::file;
 namespace file_util = ::sapi::file_util;
 
+// Use a fake pid so that /proc/{pid}/maps etc. also exist in the new pid
+// namespace
+constexpr int kFakePid = 1;
+
 // Similar to GetStackTrace() but without using the sandbox to isolate
 // libunwind.
 absl::StatusOr<std::vector<std::string>> UnsafeGetStackTrace(pid_t pid) {
@@ -92,9 +96,9 @@ bool IsSameFile(const std::string& path, const std::string& other) {
 class StackTracePeer {
  public:
   static absl::StatusOr<std::unique_ptr<Policy>> GetPolicy(
-      pid_t target_pid, const std::string& maps_file,
-      const std::string& app_path, const std::string& exe_path,
-      const Namespace* ns, bool uses_custom_forkserver);
+      const std::string& maps_file, const std::string& app_path,
+      const std::string& exe_path, const Namespace* ns,
+      bool uses_custom_forkserver);
 
   static absl::StatusOr<std::vector<std::string>> LaunchLibunwindSandbox(
       const Regs* regs, const Namespace* ns, bool uses_custom_forkserver,
@@ -102,7 +106,7 @@ class StackTracePeer {
 };
 
 absl::StatusOr<std::unique_ptr<Policy>> StackTracePeer::GetPolicy(
-    pid_t target_pid, const std::string& maps_file, const std::string& app_path,
+    const std::string& maps_file, const std::string& app_path,
     const std::string& exe_path, const Namespace* ns,
     bool uses_custom_forkserver) {
   PolicyBuilder builder;
@@ -169,14 +173,15 @@ absl::StatusOr<std::unique_ptr<Policy>> StackTracePeer::GetPolicy(
 
       // Add proc maps.
       .AddFileAt(maps_file,
-                 file::JoinPath("/proc", absl::StrCat(target_pid), "maps"))
+                 file::JoinPath("/proc", absl::StrCat(kFakePid), "maps"))
       .AddFileAt(maps_file,
-                 file::JoinPath("/proc", absl::StrCat(target_pid), "task",
-                                absl::StrCat(target_pid), "maps"))
+                 file::JoinPath("/proc", absl::StrCat(kFakePid), "task",
+                                absl::StrCat(kFakePid), "maps"))
 
       // Add the binary itself.
       .AddFileAt(exe_path, app_path)
-      .AllowLlvmCoverage();
+      .AllowLlvmCoverage()
+      .AllowLlvmSanitizers();
 
   return builder.TryBuild();
 }
@@ -261,8 +266,8 @@ absl::StatusOr<std::vector<std::string>> StackTracePeer::LaunchLibunwindSandbox(
   // forkserver).
   SAPI_ASSIGN_OR_RETURN(
       std::unique_ptr<Policy> policy,
-      StackTracePeer::GetPolicy(pid, unwind_temp_maps_path, app_path, exe_path,
-                                ns, uses_custom_forkserver));
+      StackTracePeer::GetPolicy(unwind_temp_maps_path, app_path, exe_path, ns,
+                                uses_custom_forkserver));
 
   VLOG(1) << "Running libunwind sandbox";
   auto sandbox =
@@ -270,7 +275,7 @@ absl::StatusOr<std::vector<std::string>> StackTracePeer::LaunchLibunwindSandbox(
   Comms* comms = sandbox->comms();
 
   UnwindSetup msg;
-  msg.set_pid(pid);
+  msg.set_pid(kFakePid);
   msg.set_regs(reinterpret_cast<const char*>(&regs->user_regs_),
                sizeof(regs->user_regs_));
   msg.set_default_max_frames(kDefaultMaxFrames);
@@ -330,13 +335,6 @@ absl::StatusOr<std::vector<std::string>> GetStackTrace(
     return UnsafeGetStackTrace(regs->pid());
   }
 
-  // Show a warning if sandboxed libunwind is requested but we're running in
-  // a sanitizer build (= we can't use sandboxed libunwind).
-  if (sapi::sanitizers::IsAny()) {
-    LOG(WARNING) << "Sanitizer build, using non-sandboxed libunwind";
-    return UnsafeGetStackTrace(regs->pid());
-  }
-
   return StackTracePeer::LaunchLibunwindSandbox(
       regs, ns, uses_custom_forkserver, recursion_depth);
 }
diff --git a/sandboxed_api/sandbox2/stack_trace_test.cc b/sandboxed_api/sandbox2/stack_trace_test.cc
index 6940953..94db86f 100644
--- a/sandboxed_api/sandbox2/stack_trace_test.cc
+++ b/sandboxed_api/sandbox2/stack_trace_test.cc
@@ -331,6 +331,13 @@ INSTANTIATE_TEST_SUITE_P(
             .final_status = Result::VIOLATION,
             .function_name = "ViolatePolicy",
             .full_function_description = "ViolatePolicy(int)",
+        },
+        TestCase{
+            .testname = "ViolatePolicyForked",
+            .testno = 5,
+            .final_status = Result::VIOLATION,
+            .function_name = "ViolatePolicy",
+            .full_function_description = "ViolatePolicy(int)",
         }),
     [](const ::testing::TestParamInfo<TestCase>& info) {
       return info.param.testname;
diff --git a/sandboxed_api/sandbox2/testcases/symbolize.cc b/sandboxed_api/sandbox2/testcases/symbolize.cc
index 5022947..9ae85f9 100644
--- a/sandboxed_api/sandbox2/testcases/symbolize.cc
+++ b/sandboxed_api/sandbox2/testcases/symbolize.cc
@@ -80,6 +80,19 @@ void RunTest(int testno) {
     case 4:
       SleepForXSeconds(10);
       break;
+    case 5: {
+      constexpr int kMaxForks = 16;
+      for (int i = 0; i < kMaxForks; ++i) {
+        if (fork() == 0) {
+          if (i == kMaxForks - 1) {
+            ViolatePolicy();
+          }
+          break;
+        }
+      }
+      SleepForXSeconds(10);
+      break;
+    }
     default:
       SAPI_RAW_LOG(FATAL, "Unknown test case: %d", testno);
   }