Rework stack_trace_test

PiperOrigin-RevId: 513467290
Change-Id: Iab630412052fa5e7333514f3864ebdfb7f10e1ef

sandboxed_api/sandbox2/BUILD.bazel

@@ -970,15 +970,13 @@ cc_test(
         ":stack_trace",
         ":testonly_allow_all_syscalls",
         "//sandboxed_api:testing",
-        "//sandboxed_api/sandbox2/util:bpf_helper",
         "//sandboxed_api/util:fileops",
         "//sandboxed_api/util:status_matchers",
-        "//sandboxed_api/util:temp_file",
-        "@com_google_absl//absl/cleanup",
         "@com_google_absl//absl/flags:flag",
         "@com_google_absl//absl/flags:reflection",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/time",
         "@com_google_googletest//:gtest_main",
     ],
 )

sandboxed_api/sandbox2/CMakeLists.txt

@@ -1063,19 +1063,17 @@ if(BUILD_TESTING AND SAPI_BUILD_TESTING)
     sandbox2::testcase_symbolize
   )
   target_link_libraries(sandbox2_stack_trace_test PRIVATE
-    absl::cleanup
     absl::flags
     absl::status
     absl::strings
+    absl::time
     sandbox2::allow_all_syscalls
-    sandbox2::bpf_helper
     sandbox2::global_forkserver
     sandbox2::namespace
     sandbox2::sandbox2
     sandbox2::stack_trace
     sandbox2::util
     sapi::fileops
-    sapi::temp_file
     sapi::testing
     sapi::status_matchers
     sapi::test_main

sandboxed_api/sandbox2/sandbox2_test.cc

@@ -53,7 +53,6 @@ PolicyBuilder CreateDefaultPolicyBuilder(absl::string_view path) {
 
 using ::sapi::GetTestSourcePath;
 using ::testing::Eq;
-using ::testing::HasSubstr;
 using ::testing::IsEmpty;
 using ::testing::IsTrue;
 using ::testing::Lt;
@@ -118,28 +117,6 @@ TEST(ExecutorTest, ExecutorFdConstructor) {
   ASSERT_EQ(result.final_status(), Result::OK);
 }
 
-// Tests that we return the correct state when the sandboxee timed out.
-TEST(StackTraceTest, StackTraceOnTimeoutWorks) {
-  SKIP_ANDROID;
-  const std::string path = GetTestSourcePath("sandbox2/testcases/sleep");
-
-  std::vector<std::string> args = {path};
-  std::vector<std::string> envs;
-  auto executor = std::make_unique<Executor>(path, args, envs);
-
-  SAPI_ASSERT_OK_AND_ASSIGN(auto policy,
-                            PolicyBuilder()
-                                // Don't restrict the syscalls at all.
-                                .DangerDefaultAllowAll()
-                                .TryBuild());
-  Sandbox2 sandbox(std::move(executor), std::move(policy));
-  ASSERT_TRUE(sandbox.RunAsync());
-  sandbox.set_walltime_limit(absl::Seconds(1));
-  auto result = sandbox.AwaitResult();
-  EXPECT_EQ(result.final_status(), Result::TIMEOUT);
-  EXPECT_THAT(result.GetStackTrace(), HasSubstr("sleep"));
-}
-
 // Tests that we return the correct state when the sandboxee was killed by an
 // external signal. Also make sure that we do not have the stack trace.
 TEST(RunAsyncTest, SandboxeeExternalKill) {
@@ -157,7 +134,7 @@ TEST(RunAsyncTest, SandboxeeExternalKill) {
   sandbox.Kill();
   auto result = sandbox.AwaitResult();
   EXPECT_EQ(result.final_status(), Result::EXTERNAL_KILL);
-  EXPECT_THAT(result.GetStackTrace(), IsEmpty());
+  EXPECT_THAT(result.stack_trace(), IsEmpty());
 }
 
 // Tests that we do not collect stack traces if it was disabled (signaled).
@@ -176,7 +153,7 @@ TEST(RunAsyncTest, SandboxeeTimeoutDisabledStacktraces) {
   sandbox.set_walltime_limit(absl::Seconds(1));
   auto result = sandbox.AwaitResult();
   EXPECT_EQ(result.final_status(), Result::TIMEOUT);
-  EXPECT_THAT(result.GetStackTrace(), IsEmpty());
+  EXPECT_THAT(result.stack_trace(), IsEmpty());
 }
 
 // Tests that we do not collect stack traces if it was disabled (violation).
@@ -196,7 +173,7 @@ TEST(RunAsyncTest, SandboxeeViolationDisabledStacktraces) {
   ASSERT_TRUE(sandbox.RunAsync());
   auto result = sandbox.AwaitResult();
   EXPECT_EQ(result.final_status(), Result::VIOLATION);
-  EXPECT_THAT(result.GetStackTrace(), IsEmpty());
+  EXPECT_THAT(result.stack_trace(), IsEmpty());
 }
 
 TEST(RunAsyncTest, SandboxeeNotKilledWhenStartingThreadFinishes) {
@@ -205,7 +182,6 @@ TEST(RunAsyncTest, SandboxeeNotKilledWhenStartingThreadFinishes) {
   auto executor = std::make_unique<Executor>(path, args);
 
   SAPI_ASSERT_OK_AND_ASSIGN(auto policy, CreateDefaultPolicyBuilder(path)
-                                             .CollectStacktracesOnExit(true)
                                              .TryBuild());
   Sandbox2 sandbox(std::move(executor), std::move(policy));
   std::thread sandbox_start_thread([&sandbox]() { sandbox.RunAsync(); });

sandboxed_api/sandbox2/stack_trace_test.cc

@@ -24,12 +24,11 @@
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
-#include "absl/cleanup/cleanup.h"
 #include "absl/flags/declare.h"
 #include "absl/flags/flag.h"
 #include "absl/flags/reflection.h"
-#include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
+#include "absl/time/time.h"
 #include "sandboxed_api/sandbox2/allow_all_syscalls.h"
 #include "sandboxed_api/sandbox2/executor.h"
 #include "sandboxed_api/sandbox2/global_forkclient.h"
@@ -37,11 +36,9 @@
 #include "sandboxed_api/sandbox2/policybuilder.h"
 #include "sandboxed_api/sandbox2/result.h"
 #include "sandboxed_api/sandbox2/sandbox2.h"
-#include "sandboxed_api/sandbox2/util/bpf_helper.h"
 #include "sandboxed_api/testing.h"
 #include "sandboxed_api/util/fileops.h"
 #include "sandboxed_api/util/status_matchers.h"
-#include "sandboxed_api/util/temp_file.h"
 
 ABSL_DECLARE_FLAG(bool, sandbox_libunwind_crash_handler);
 
@@ -49,64 +46,70 @@ namespace sandbox2 {
 namespace {
 
 namespace file_util = ::sapi::file_util;
-using ::sapi::CreateNamedTempFileAndClose;
 using ::sapi::GetTestSourcePath;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Eq;
-using ::testing::HasSubstr;
 using ::testing::IsEmpty;
-using ::testing::IsTrue;
-using ::testing::Not;
+using ::testing::StartsWith;
+
+struct TestCase {
+  std::string arg = "1";
+  int final_status = Result::SIGNALED;
+  std::string function_name = "CrashMe";
+  std::string full_function_description = "CrashMe(char)";
+  std::function<void(PolicyBuilder*)> modify_policy;
+  absl::Duration wall_time_limit = absl::ZeroDuration();
+};
+
+class StackTraceTest : public ::testing::TestWithParam<TestCase> {};
 
 // Test that symbolization of stack traces works.
-void SymbolizationWorksCommon(
-    std::function<void(PolicyBuilder*)> modify_policy = {}) {
+void SymbolizationWorksCommon(TestCase param) {
   const std::string path = GetTestSourcePath("sandbox2/testcases/symbolize");
-  std::vector<std::string> args = {path, "1"};
-
-  SAPI_ASSERT_OK_AND_ASSIGN(std::string temp_filename,
-                            CreateNamedTempFileAndClose("/tmp/"));
-  absl::Cleanup temp_cleanup = [&temp_filename] {
-    remove(temp_filename.c_str());
-  };
-  ASSERT_THAT(
-      file_util::fileops::CopyFile("/proc/cpuinfo", temp_filename, 0444),
-      IsTrue());
+  std::vector<std::string> args = {path, param.arg};
 
   auto policybuilder = PolicyBuilder()
                            // Don't restrict the syscalls at all.
-                           .DefaultAction(AllowAllSyscalls())
-                           .AddFile(path)
-                           .AddLibrariesForBinary(path)
-                           .AddFileAt(temp_filename, "/proc/cpuinfo");
-  if (modify_policy) {
-    modify_policy(&policybuilder);
+                           .DefaultAction(AllowAllSyscalls());
+  if (param.modify_policy) {
+    param.modify_policy(&policybuilder);
   }
   SAPI_ASSERT_OK_AND_ASSIGN(auto policy, policybuilder.TryBuild());
 
   Sandbox2 s2(std::make_unique<Executor>(path, args), std::move(policy));
-  auto result = s2.Run();
+  ASSERT_TRUE(s2.RunAsync());
+  s2.set_walltime_limit(param.wall_time_limit);
+  auto result = s2.AwaitResult();
 
-  ASSERT_THAT(result.final_status(), Eq(Result::SIGNALED));
-  ASSERT_THAT(result.GetStackTrace(), HasSubstr("CrashMe"));
+  EXPECT_THAT(result.final_status(), Eq(param.final_status));
+  EXPECT_THAT(result.stack_trace(), Contains(StartsWith(param.function_name)));
   // Check that demangling works as well.
-  ASSERT_THAT(result.GetStackTrace(), HasSubstr("CrashMe()"));
+  EXPECT_THAT(result.stack_trace(),
+              Contains(StartsWith(param.full_function_description)));
 }
 
-TEST(StackTraceTest, SymbolizationWorksNonSandboxedLibunwind) {
+void SymbolizationWorksWithModifiedPolicy(
+    std::function<void(PolicyBuilder*)> modify_policy) {
+  TestCase test_case;
+  test_case.modify_policy = std::move(modify_policy);
+  SymbolizationWorksCommon(test_case);
+}
+
+TEST_P(StackTraceTest, SymbolizationWorksNonSandboxedLibunwind) {
   SKIP_SANITIZERS_AND_COVERAGE;
   absl::FlagSaver fs;
   absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, false);
 
-  SymbolizationWorksCommon();
+  SymbolizationWorksCommon(GetParam());
 }
 
-TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwind) {
+TEST_P(StackTraceTest, SymbolizationWorksSandboxedLibunwind) {
   SKIP_SANITIZERS_AND_COVERAGE;
   absl::FlagSaver fs;
   absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true);
 
-  SymbolizationWorksCommon();
+  SymbolizationWorksCommon(GetParam());
 }
 
 TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindProcDirMounted) {
@@ -114,7 +117,7 @@ TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindProcDirMounted) {
   absl::FlagSaver fs;
   absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true);
 
-  SymbolizationWorksCommon(
+  SymbolizationWorksWithModifiedPolicy(
       [](PolicyBuilder* builder) { builder->AddDirectory("/proc"); });
 }
 
@@ -123,7 +126,7 @@ TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindProcFileMounted) {
   absl::FlagSaver fs;
   absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true);
 
-  SymbolizationWorksCommon([](PolicyBuilder* builder) {
+  SymbolizationWorksWithModifiedPolicy([](PolicyBuilder* builder) {
     builder->AddFile("/proc/sys/vm/overcommit_memory");
   });
 }
@@ -133,7 +136,7 @@ TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindSysDirMounted) {
   absl::FlagSaver fs;
   absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true);
 
-  SymbolizationWorksCommon(
+  SymbolizationWorksWithModifiedPolicy(
       [](PolicyBuilder* builder) { builder->AddDirectory("/sys"); });
 }
 
@@ -142,12 +145,12 @@ TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindSysFileMounted) {
   absl::FlagSaver fs;
   absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true);
 
-  SymbolizationWorksCommon([](PolicyBuilder* builder) {
+  SymbolizationWorksWithModifiedPolicy([](PolicyBuilder* builder) {
     builder->AddFile("/sys/devices/system/cpu/online");
   });
 }
 
-static size_t FileCountInDirectory(const std::string& path) {
+size_t FileCountInDirectory(const std::string& path) {
   std::vector<std::string> fds;
   std::string error;
   CHECK(file_util::fileops::ListDirectoryEntries(path, &fds, &error));
@@ -161,9 +164,7 @@ TEST(StackTraceTest, ForkEnterNsLibunwindDoesNotLeakFDs) {
 
   // Very first sanitization might create some fds (e.g. for initial
   // namespaces).
-  SymbolizationWorksCommon([](PolicyBuilder* builder) {
-    builder->AddFile("/sys/devices/system/cpu/online");
-  });
+  SymbolizationWorksCommon({});
 
   // Get list of open FDs in the global forkserver.
   pid_t forkserver_pid = GlobalForkClient::GetPid();
@@ -171,33 +172,11 @@ TEST(StackTraceTest, ForkEnterNsLibunwindDoesNotLeakFDs) {
       absl::StrCat("/proc/", forkserver_pid, "/fd");
   size_t filecount_before = FileCountInDirectory(forkserver_fd_path);
 
-  SymbolizationWorksCommon([](PolicyBuilder* builder) {
-    builder->AddFile("/sys/devices/system/cpu/online");
-  });
+  SymbolizationWorksCommon({});
 
   EXPECT_THAT(filecount_before, Eq(FileCountInDirectory(forkserver_fd_path)));
 }
 
-// Test that symbolization skips writeable files (attack vector).
-TEST(StackTraceTest, SymbolizationTrustedFilesOnly) {
-  SKIP_SANITIZERS_AND_COVERAGE;
-  const std::string path = GetTestSourcePath("sandbox2/testcases/symbolize");
-  std::vector<std::string> args = {path, "2"};
-
-  SAPI_ASSERT_OK_AND_ASSIGN(auto policy,
-                            PolicyBuilder()
-                                // Don't restrict the syscalls at all.
-                                .DefaultAction(AllowAllSyscalls())
-                                .AddFile(path)
-                                .AddLibrariesForBinary(path)
-                                .TryBuild());
-  Sandbox2 s2(std::make_unique<Executor>(path, args), std::move(policy));
-  auto result = s2.Run();
-
-  ASSERT_THAT(result.final_status(), Eq(Result::SIGNALED));
-  ASSERT_THAT(result.GetStackTrace(), Not(HasSubstr("CrashMe")));
-}
-
 TEST(StackTraceTest, CompactStackTrace) {
   EXPECT_THAT(CompactStackTrace({}), IsEmpty());
   EXPECT_THAT(CompactStackTrace({"_start"}), ElementsAre("_start"));
@@ -223,5 +202,41 @@ TEST(StackTraceTest, CompactStackTrace) {
                           "(previous frame repeated 3 times)"));
 }
 
+INSTANTIATE_TEST_SUITE_P(
+    Instantiation, StackTraceTest,
+    ::testing::Values(
+        TestCase{
+            .arg = "1",
+            .final_status = Result::SIGNALED,
+            .function_name = "CrashMe",
+            .full_function_description = "CrashMe(char)",
+        },
+        TestCase{
+            .arg = "2",
+            .final_status = Result::VIOLATION,
+            .function_name = "ViolatePolicy",
+            .full_function_description = "ViolatePolicy(int)",
+        },
+        TestCase{
+            .arg = "3",
+            .final_status = Result::OK,
+            .function_name = "ExitNormally",
+            .full_function_description = "ExitNormally(int)",
+            .modify_policy =
+                [](PolicyBuilder* builder) {
+                  builder->CollectStacktracesOnExit(true);
+                },
+        },
+        TestCase{
+            .arg = "4",
+            .final_status = Result::TIMEOUT,
+            .function_name = "SleepForXSeconds",
+            .full_function_description = "SleepForXSeconds(int)",
+            .wall_time_limit = absl::Seconds(1),
+        }),
+    [](const ::testing::TestParamInfo<TestCase>& info) {
+      return info.param.function_name;
+    });
+
 }  // namespace
 }  // namespace sandbox2

sandboxed_api/sandbox2/testcases/BUILD.bazel

@@ -168,9 +168,9 @@ cc_binary(
     testonly = True,
     srcs = ["symbolize.cc"],
     copts = sapi_platform_copts(),
+    features = ["fully_static_link"],
     deps = [
         "//sandboxed_api/util:raw_logging",
-        "//sandboxed_api/util:temp_file",
         "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/strings",
     ],

sandboxed_api/sandbox2/testcases/CMakeLists.txt

@@ -208,10 +208,9 @@ set_target_properties(sandbox2_testcase_symbolize PROPERTIES
   OUTPUT_NAME symbolize
 )
 target_link_libraries(sandbox2_testcase_symbolize PRIVATE
+  -static
   absl::core_headers
   absl::strings
-  sapi::temp_file
-  sapi::strerror
   sapi::base
   sapi::raw_logging
 )

sandboxed_api/sandbox2/testcases/symbolize.cc

@@ -12,65 +12,59 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// A binary that crashes, either directly or by copying and re-executing,
-// to test the stack tracing symbolizer.
+// A binary that exits via different modes: crashes, causes violation, exits
+// normally or times out, to test the stack tracing symbolizer.
 
-#include <fcntl.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <sys/stat.h>
+#include <sys/syscall.h>
 #include <unistd.h>
 
-#include <string>
+#include <cstdlib>
 
 #include "absl/base/attributes.h"
 #include "absl/strings/numbers.h"
 #include "sandboxed_api/util/raw_logging.h"
-#include "sandboxed_api/util/temp_file.h"
+
+// Sometimes we don't have debug info to properly unwind through libc (a frame
+// is skipped).
+// Workaround by putting another frame on the call stack.
+template <typename F>
+ABSL_ATTRIBUTE_NOINLINE ABSL_ATTRIBUTE_NO_TAIL_CALL void IndirectLibcCall(
+    F func) {
+  func();
+}
 
 ABSL_ATTRIBUTE_NOINLINE
-void CrashMe() {
+void CrashMe(char x = 0) {
   volatile char* null = nullptr;
-  *null = 0;
+  *null = x;
 }
 
-void RunWritable() {
-  int exe_fd = open("/proc/self/exe", O_RDONLY);
-  SAPI_RAW_PCHECK(exe_fd >= 0, "Opening /proc/self/exe");
-
-  std::string tmpname;
-  int tmp_fd;
-  std::tie(tmpname, tmp_fd) = sapi::CreateNamedTempFile("tmp").value();
-  SAPI_RAW_PCHECK(fchmod(tmp_fd, S_IRWXU) == 0, "Fchmod on temporary file");
-
-  char buf[4096];
-  while (true) {
-    ssize_t read_cnt = read(exe_fd, buf, sizeof(buf));
-    SAPI_RAW_PCHECK(read_cnt >= 0, "Reading /proc/self/exe");
-    if (read_cnt == 0) {
-      break;
-    }
-    SAPI_RAW_PCHECK(write(tmp_fd, buf, read_cnt) == read_cnt,
-                    "Writing temporary file");
+ABSL_ATTRIBUTE_NOINLINE
+ABSL_ATTRIBUTE_NO_TAIL_CALL
+void ViolatePolicy(int x = 0) {
+  IndirectLibcCall([x]() { syscall(__NR_ptrace, x); });
 }
 
-  SAPI_RAW_PCHECK(close(tmp_fd) == 0, "Closing temporary file");
-  tmp_fd = open(tmpname.c_str(), O_RDONLY);
-  SAPI_RAW_PCHECK(tmp_fd >= 0, "Reopening temporary file");
+ABSL_ATTRIBUTE_NOINLINE
+ABSL_ATTRIBUTE_NO_TAIL_CALL
+void ExitNormally(int x = 0) {
+  IndirectLibcCall([x]() {
+    // _exit is marked noreturn, which makes stack traces a bit trickier -
+    // work around by using a volatile read
+    if (volatile int y = 1) {
+      _exit(x);
+    }
+  });
+}
 
-  char prog_name[] = "crashme";
-  char testno[] = "1";
-  char* argv[] = {prog_name, testno, nullptr};
-
-  SAPI_RAW_PCHECK(execv(tmpname.c_str(), argv) == 0, "Executing copied binary");
+ABSL_ATTRIBUTE_NOINLINE
+ABSL_ATTRIBUTE_NO_TAIL_CALL
+void SleepForXSeconds(int x = 0) {
+  IndirectLibcCall([x]() { sleep(x); });
 }
 
 int main(int argc, char* argv[]) {
-  if (argc < 2) {
-    printf("argc < 3\n");
-    return EXIT_FAILURE;
-  }
-
+  SAPI_RAW_CHECK(argc >= 2, "Not enough arguments");
   int testno;
   SAPI_RAW_CHECK(absl::SimpleAtoi(argv[1], &testno), "testno not a number");
   switch (testno) {
@@ -78,13 +72,16 @@ int main(int argc, char* argv[]) {
       CrashMe();
       break;
     case 2:
-      RunWritable();
+      ViolatePolicy();
+      break;
+    case 3:
+      ExitNormally();
+      break;
+    case 4:
+      SleepForXSeconds(10);
       break;
     default:
-      printf("Unknown test: %d\n", testno);
-      return EXIT_FAILURE;
+      SAPI_RAW_LOG(FATAL, "Unknown test case: %d", testno);
   }
-
-  printf("OK: All tests went OK\n");
   return EXIT_SUCCESS;
 }