Rework stack_trace_test
PiperOrigin-RevId: 513467290
Change-Id: Iab630412052fa5e7333514f3864ebdfb7f10e1ef
diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel
index b0c960c..b387ec2 100644
--- a/sandboxed_api/sandbox2/BUILD.bazel
+++ b/sandboxed_api/sandbox2/BUILD.bazel
@@ -970,15 +970,13 @@
":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",
],
)
diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt
index 416ece2..02446a2 100644
--- a/sandboxed_api/sandbox2/CMakeLists.txt
+++ b/sandboxed_api/sandbox2/CMakeLists.txt
@@ -1063,19 +1063,17 @@
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
diff --git a/sandboxed_api/sandbox2/sandbox2_test.cc b/sandboxed_api/sandbox2/sandbox2_test.cc
index 5a3af66..3177c3d 100644
--- a/sandboxed_api/sandbox2/sandbox2_test.cc
+++ b/sandboxed_api/sandbox2/sandbox2_test.cc
@@ -53,7 +53,6 @@
using ::sapi::GetTestSourcePath;
using ::testing::Eq;
-using ::testing::HasSubstr;
using ::testing::IsEmpty;
using ::testing::IsTrue;
using ::testing::Lt;
@@ -118,28 +117,6 @@
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 @@
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 @@
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 @@
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 @@
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(); });
diff --git a/sandboxed_api/sandbox2/stack_trace_test.cc b/sandboxed_api/sandbox2/stack_trace_test.cc
index 1e7defb..48b56eb 100644
--- a/sandboxed_api/sandbox2/stack_trace_test.cc
+++ b/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 {
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 @@
absl::FlagSaver fs;
absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true);
- SymbolizationWorksCommon(
+ SymbolizationWorksWithModifiedPolicy(
[](PolicyBuilder* builder) { builder->AddDirectory("/proc"); });
}
@@ -123,7 +126,7 @@
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 @@
absl::FlagSaver fs;
absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true);
- SymbolizationWorksCommon(
+ SymbolizationWorksWithModifiedPolicy(
[](PolicyBuilder* builder) { builder->AddDirectory("/sys"); });
}
@@ -142,12 +145,12 @@
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 @@
// 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 @@
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 @@
"(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
diff --git a/sandboxed_api/sandbox2/testcases/BUILD.bazel b/sandboxed_api/sandbox2/testcases/BUILD.bazel
index 8b552c4..017a6ba 100644
--- a/sandboxed_api/sandbox2/testcases/BUILD.bazel
+++ b/sandboxed_api/sandbox2/testcases/BUILD.bazel
@@ -168,9 +168,9 @@
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",
],
diff --git a/sandboxed_api/sandbox2/testcases/CMakeLists.txt b/sandboxed_api/sandbox2/testcases/CMakeLists.txt
index b9bcbe7..ca6f610 100644
--- a/sandboxed_api/sandbox2/testcases/CMakeLists.txt
+++ b/sandboxed_api/sandbox2/testcases/CMakeLists.txt
@@ -208,10 +208,9 @@
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
)
diff --git a/sandboxed_api/sandbox2/testcases/symbolize.cc b/sandboxed_api/sandbox2/testcases/symbolize.cc
index 55cb1d2..9a5d998 100644
--- a/sandboxed_api/sandbox2/testcases/symbolize.cc
+++ b/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"
-ABSL_ATTRIBUTE_NOINLINE
-void CrashMe() {
- volatile char* null = nullptr;
- *null = 0;
+// 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();
}
-void RunWritable() {
- int exe_fd = open("/proc/self/exe", O_RDONLY);
- SAPI_RAW_PCHECK(exe_fd >= 0, "Opening /proc/self/exe");
+ABSL_ATTRIBUTE_NOINLINE
+void CrashMe(char x = 0) {
+ volatile char* null = nullptr;
+ *null = x;
+}
- 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");
+ABSL_ATTRIBUTE_NOINLINE
+ABSL_ATTRIBUTE_NO_TAIL_CALL
+void ViolatePolicy(int x = 0) {
+ IndirectLibcCall([x]() { syscall(__NR_ptrace, x); });
+}
- 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;
+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);
}
- SAPI_RAW_PCHECK(write(tmp_fd, buf, read_cnt) == read_cnt,
- "Writing temporary file");
- }
+ });
+}
- 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");
-
- 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 @@
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;
}
