From 9c09db654e3304ce57a2757b33c87e28df7153dc Mon Sep 17 00:00:00 2001
From: Jamie Liu <jamieliu@google.com>
Date: Mon, 12 Jul 2021 12:47:08 -0700
Subject: Fix async-signal-unsafety in chroot test.
PiperOrigin-RevId: 384295543
---
test/syscalls/linux/BUILD | 1 +
test/syscalls/linux/chroot.cc | 226 +++++++++++++++++++++++++++---------------
2 files changed, 145 insertions(+), 82 deletions(-)
(limited to 'test')
diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD
index 2bf685524..5ca655803 100644
--- a/test/syscalls/linux/BUILD
+++ b/test/syscalls/linux/BUILD
@@ -479,6 +479,7 @@ cc_binary(
"//test/util:cleanup",
"//test/util:file_descriptor",
"//test/util:fs_util",
+ "@com_google_absl//absl/cleanup",
"@com_google_absl//absl/strings",
gtest,
"//test/util:logging",
diff --git a/test/syscalls/linux/chroot.cc b/test/syscalls/linux/chroot.cc
index fab79d300..7e4626f03 100644
--- a/test/syscalls/linux/chroot.cc
+++ b/test/syscalls/linux/chroot.cc
@@ -20,16 +20,17 @@
#include <syscall.h>
#include <unistd.h>
+#include <algorithm>
#include <string>
#include <vector>
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include "absl/cleanup/cleanup.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/str_split.h"
#include "absl/strings/string_view.h"
#include "test/util/capability_util.h"
-#include "test/util/cleanup.h"
#include "test/util/file_descriptor.h"
#include "test/util/fs_util.h"
#include "test/util/logging.h"
@@ -46,13 +47,52 @@ namespace testing {
namespace {
+// Async-signal-safe conversion from integer to string, appending the string
+// (including a terminating NUL) to buf, which is a buffer of size len bytes.
+// Returns the number of bytes written, or 0 if the buffer is too small.
+//
+// Preconditions: 2 <= radix <= 16.
+template <typename T>
+size_t SafeItoa(T val, char* buf, size_t len, int radix) {
+ size_t n = 0;
+#define _WRITE_OR_FAIL(c) \
+ do { \
+ if (len == 0) { \
+ return 0; \
+ } \
+ buf[n] = (c); \
+ n++; \
+ len--; \
+ } while (false)
+ if (val == 0) {
+ _WRITE_OR_FAIL('0');
+ } else {
+ // Write digits in reverse order, then reverse them at the end.
+ bool neg = val < 0;
+ while (val != 0) {
+ // C/C++ define modulo such that the result is negative if exactly one of
+ // the dividend or divisor is negative, so this handles both positive and
+ // negative values.
+ char c = "fedcba9876543210123456789abcdef"[val % radix + 15];
+ _WRITE_OR_FAIL(c);
+ val /= 10;
+ }
+ if (neg) {
+ _WRITE_OR_FAIL('-');
+ }
+ std::reverse(buf, buf + n);
+ }
+ _WRITE_OR_FAIL('\0');
+ return n;
+#undef _WRITE_OR_FAIL
+}
+
TEST(ChrootTest, Success) {
SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_CHROOT)));
+ auto temp_dir = TempPath::CreateDir().ValueOrDie();
+ const std::string temp_dir_path = temp_dir.path();
- const auto rest = [] {
- auto temp_dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
- TEST_CHECK_SUCCESS(chroot(temp_dir.path().c_str()));
- };
+ const auto rest = [&] { TEST_CHECK_SUCCESS(chroot(temp_dir_path.c_str())); };
EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0));
}
@@ -101,28 +141,34 @@ TEST(ChrootTest, CreatesNewRoot) {
SyscallSucceeds());
auto new_root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+ const std::string new_root_path = new_root.path();
auto file_in_new_root =
ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFileIn(new_root.path()));
+ const std::string file_in_new_root_path = file_in_new_root.path();
const auto rest = [&] {
// chroot into new_root.
- TEST_CHECK_SUCCESS(chroot(new_root.path().c_str()));
+ TEST_CHECK_SUCCESS(chroot(new_root_path.c_str()));
// getcwd should return "(unreachable)" followed by the initial_cwd.
- char cwd[1024];
- TEST_CHECK_SUCCESS(syscall(__NR_getcwd, cwd, sizeof(cwd)));
- std::string expected_cwd = "(unreachable)";
- expected_cwd += initial_cwd;
- TEST_CHECK(strcmp(cwd, expected_cwd.c_str()) == 0);
+ char buf[1024];
+ TEST_CHECK_SUCCESS(syscall(__NR_getcwd, buf, sizeof(buf)));
+ constexpr char kUnreachablePrefix[] = "(unreachable)";
+ TEST_CHECK(
+ strncmp(buf, kUnreachablePrefix, sizeof(kUnreachablePrefix) - 1) == 0);
+ TEST_CHECK(strcmp(buf + sizeof(kUnreachablePrefix) - 1, initial_cwd) == 0);
// Should not be able to stat file by its full path.
struct stat statbuf;
- TEST_CHECK_ERRNO(stat(file_in_new_root.path().c_str(), &statbuf), ENOENT);
+ TEST_CHECK_ERRNO(stat(file_in_new_root_path.c_str(), &statbuf), ENOENT);
// Should be able to stat file at new rooted path.
- auto basename = std::string(Basename(file_in_new_root.path()));
- auto rootedFile = "/" + basename;
- TEST_CHECK_SUCCESS(stat(rootedFile.c_str(), &statbuf));
+ buf[0] = '/';
+ absl::string_view basename = Basename(file_in_new_root_path);
+ TEST_CHECK(basename.length() < (sizeof(buf) - 2));
+ memcpy(buf + 1, basename.data(), basename.length());
+ buf[basename.length() + 1] = '\0';
+ TEST_CHECK_SUCCESS(stat(buf, &statbuf));
// Should be able to stat cwd at '.' even though it's outside root.
TEST_CHECK_SUCCESS(stat(".", &statbuf));
@@ -131,8 +177,8 @@ TEST(ChrootTest, CreatesNewRoot) {
TEST_CHECK_SUCCESS(chdir("/"));
// getcwd should return "/".
- TEST_CHECK_SUCCESS(syscall(__NR_getcwd, cwd, sizeof(cwd)));
- TEST_CHECK_SUCCESS(strcmp(cwd, "/") == 0);
+ TEST_CHECK_SUCCESS(syscall(__NR_getcwd, buf, sizeof(buf)));
+ TEST_CHECK_SUCCESS(strcmp(buf, "/") == 0);
// Statting '.', '..', '/', and '/..' all return the same dev and inode.
struct stat statbuf_dot;
@@ -160,10 +206,11 @@ TEST(ChrootTest, DotDotFromOpenFD) {
auto fd = ASSERT_NO_ERRNO_AND_VALUE(
Open(dir_outside_root.path(), O_RDONLY | O_DIRECTORY));
auto new_root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+ const std::string new_root_path = new_root.path();
const auto rest = [&] {
// chroot into new_root.
- TEST_CHECK_SUCCESS(chroot(new_root.path().c_str()));
+ TEST_CHECK_SUCCESS(chroot(new_root_path.c_str()));
// openat on fd with path .. will succeed.
int other_fd;
@@ -184,15 +231,18 @@ TEST(ChrootTest, ProcFdLinkResolutionInChroot) {
const TempPath file_outside_chroot =
ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+ const std::string file_outside_chroot_path = file_outside_chroot.path();
const FileDescriptor fd =
ASSERT_NO_ERRNO_AND_VALUE(Open(file_outside_chroot.path(), O_RDONLY));
const FileDescriptor proc_fd = ASSERT_NO_ERRNO_AND_VALUE(
Open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC));
+ auto temp_dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+ const std::string temp_dir_path = temp_dir.path();
+
const auto rest = [&] {
- auto temp_dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
- TEST_CHECK_SUCCESS(chroot(temp_dir.path().c_str()));
+ TEST_CHECK_SUCCESS(chroot(temp_dir_path.c_str()));
// Opening relative to an already open fd to a node outside the chroot
// works.
@@ -201,9 +251,10 @@ TEST(ChrootTest, ProcFdLinkResolutionInChroot) {
// Proc fd symlinks can escape the chroot if the fd the symlink refers to
// refers to an object outside the chroot.
+ char fd_buf[11];
+ TEST_CHECK(SafeItoa(fd.get(), fd_buf, sizeof(fd_buf), 10));
struct stat s = {};
- TEST_CHECK_SUCCESS(
- fstatat(proc_self_fd.get(), absl::StrCat(fd.get()).c_str(), &s, 0));
+ TEST_CHECK_SUCCESS(fstatat(proc_self_fd.get(), fd_buf, &s, 0));
// Try to stat the stdin fd. Internally, this is handled differently from a
// proc fd entry pointing to a file, since stdin is backed by a host fd, and
@@ -223,10 +274,12 @@ TEST(ChrootTest, ProcMemSelfFdsNoEscapeProcOpen) {
const FileDescriptor proc =
ASSERT_NO_ERRNO_AND_VALUE(Open("/proc", O_RDONLY));
+ const auto temp_dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+ const std::string temp_dir_path = temp_dir.path();
+
const auto rest = [&] {
- // Create and enter a chroot directory.
- const auto temp_dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
- TEST_CHECK_SUCCESS(chroot(temp_dir.path().c_str()));
+ // Enter the chroot directory.
+ TEST_CHECK_SUCCESS(chroot(temp_dir_path.c_str()));
// Open a file inside the chroot at /foo.
const FileDescriptor foo =
@@ -234,11 +287,15 @@ TEST(ChrootTest, ProcMemSelfFdsNoEscapeProcOpen) {
// Examine /proc/self/fd/{foo_fd} to see if it exposes the fact that we're
// inside a chroot, the path should be /foo and NOT {chroot_dir}/foo.
- const std::string fd_path = absl::StrCat("self/fd/", foo.get());
+ constexpr char kSelfFdRelpath[] = "self/fd/";
+ char path_buf[20];
+ strcpy(path_buf, kSelfFdRelpath); // NOLINT: need async-signal-safety
+ TEST_CHECK(SafeItoa(foo.get(), path_buf + sizeof(kSelfFdRelpath) - 1,
+ sizeof(path_buf) - (sizeof(kSelfFdRelpath) - 1), 10));
char buf[1024] = {};
size_t bytes_read = 0;
- TEST_CHECK_SUCCESS(bytes_read = readlinkat(proc.get(), fd_path.c_str(), buf,
- sizeof(buf) - 1));
+ TEST_CHECK_SUCCESS(
+ bytes_read = readlinkat(proc.get(), path_buf, buf, sizeof(buf) - 1));
// The link should resolve to something.
TEST_CHECK(bytes_read > 0);
@@ -258,10 +315,12 @@ TEST(ChrootTest, ProcMemSelfMapsNoEscapeProcOpen) {
const FileDescriptor proc =
ASSERT_NO_ERRNO_AND_VALUE(Open("/proc", O_RDONLY));
+ const auto temp_dir = TEST_CHECK_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+ const std::string temp_dir_path = temp_dir.path();
+
const auto rest = [&] {
- // Create and enter a chroot directory.
- const auto temp_dir = TEST_CHECK_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
- TEST_CHECK_SUCCESS(chroot(temp_dir.path().c_str()));
+ // Enter the chroot directory.
+ TEST_CHECK_SUCCESS(chroot(temp_dir_path.c_str()));
// Open a file inside the chroot at /foo.
const FileDescriptor foo =
@@ -272,9 +331,12 @@ TEST(ChrootTest, ProcMemSelfMapsNoEscapeProcOpen) {
MAP_PRIVATE, foo.get(), 0);
TEST_CHECK_SUCCESS(reinterpret_cast<int64_t>(foo_map));
- // Always unmap.
- auto cleanup_map =
- Cleanup([&] { TEST_CHECK_SUCCESS(munmap(foo_map, kPageSize)); });
+ // Always unmap. Since this function is called between fork() and execve(),
+ // we can't use gvisor::testing::Cleanup, which uses std::function
+ // and thus may heap-allocate (which is async-signal-unsafe); instead, use
+ // absl::Cleanup, which is templated on the callback type.
+ auto cleanup_map = absl::MakeCleanup(
+ [&] { TEST_CHECK_SUCCESS(munmap(foo_map, kPageSize)); });
// Examine /proc/self/maps to be sure that /foo doesn't appear to be
// mapped with the full chroot path.
@@ -289,8 +351,8 @@ TEST(ChrootTest, ProcMemSelfMapsNoEscapeProcOpen) {
TEST_CHECK(bytes_read > 0);
// Finally we want to make sure the maps don't contain the chroot path
- TEST_CHECK(std::string(buf, bytes_read).find(temp_dir.path()) ==
- std::string::npos);
+ TEST_CHECK(
+ !absl::StrContains(absl::string_view(buf, bytes_read), temp_dir_path));
};
EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0));
}
@@ -302,72 +364,72 @@ TEST(ChrootTest, ProcMountsMountinfoNoEscape) {
SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_CHROOT)));
// Create nested tmpfs mounts.
- auto const outer_dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
- auto const outer_mount = ASSERT_NO_ERRNO_AND_VALUE(
- Mount("none", outer_dir.path(), "tmpfs", 0, "mode=0700", 0));
-
- auto const inner_dir =
- ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(outer_dir.path()));
- auto const inner_mount = ASSERT_NO_ERRNO_AND_VALUE(
- Mount("none", inner_dir.path(), "tmpfs", 0, "mode=0700", 0));
+ const auto outer_dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+ const std::string outer_dir_path = outer_dir.path();
+ const auto outer_mount = ASSERT_NO_ERRNO_AND_VALUE(
+ Mount("none", outer_dir_path, "tmpfs", 0, "mode=0700", 0));
+
+ const auto inner_dir =
+ ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(outer_dir_path));
+ const std::string inner_dir_path = inner_dir.path();
+ const auto inner_mount = ASSERT_NO_ERRNO_AND_VALUE(
+ Mount("none", inner_dir_path, "tmpfs", 0, "mode=0700", 0));
+ const std::string inner_dir_in_outer_chroot_path =
+ absl::StrCat("/", Basename(inner_dir_path));
+
+ // Filenames that will be checked for mounts, all relative to /proc dir.
+ std::string paths[3] = {"mounts", "self/mounts", "self/mountinfo"};
+
+ for (const std::string& path : paths) {
+ // We should have both inner and outer mounts.
+ const std::string contents =
+ ASSERT_NO_ERRNO_AND_VALUE(GetContents(JoinPath("/proc", path)));
+ EXPECT_THAT(contents,
+ AllOf(HasSubstr(outer_dir_path), HasSubstr(inner_dir_path)));
+ // We better have at least two mounts: the mounts we created plus the
+ // root.
+ std::vector<absl::string_view> submounts =
+ absl::StrSplit(contents, '\n', absl::SkipWhitespace());
+ ASSERT_GT(submounts.size(), 2);
+ }
- const auto rest = [&outer_dir, &inner_dir] {
- // Filenames that will be checked for mounts, all relative to /proc dir.
- std::string paths[3] = {"mounts", "self/mounts", "self/mountinfo"};
-
- for (const std::string& path : paths) {
- // We should have both inner and outer mounts.
- const std::string contents =
- TEST_CHECK_NO_ERRNO_AND_VALUE(GetContents(JoinPath("/proc", path)));
- EXPECT_THAT(contents, AllOf(HasSubstr(outer_dir.path()),
- HasSubstr(inner_dir.path())));
- // We better have at least two mounts: the mounts we created plus the
- // root.
- std::vector<absl::string_view> submounts =
- absl::StrSplit(contents, '\n', absl::SkipWhitespace());
- TEST_CHECK(submounts.size() > 2);
- }
-
- // Get a FD to /proc before we enter the chroot.
- const FileDescriptor proc =
- TEST_CHECK_NO_ERRNO_AND_VALUE(Open("/proc", O_RDONLY));
+ // Get a FD to /proc before we enter the chroot.
+ const FileDescriptor proc =
+ ASSERT_NO_ERRNO_AND_VALUE(Open("/proc", O_RDONLY));
+ const auto rest = [&] {
// Chroot to outer mount.
- TEST_CHECK_SUCCESS(chroot(outer_dir.path().c_str()));
+ TEST_CHECK_SUCCESS(chroot(outer_dir_path.c_str()));
+ char buf[8 * 1024];
for (const std::string& path : paths) {
const FileDescriptor proc_file =
TEST_CHECK_NO_ERRNO_AND_VALUE(OpenAt(proc.get(), path, O_RDONLY));
// Only two mounts visible from this chroot: the inner and outer. Both
// paths should be relative to the new chroot.
- const std::string contents =
- TEST_CHECK_NO_ERRNO_AND_VALUE(GetContentsFD(proc_file.get()));
- EXPECT_THAT(contents,
- AllOf(HasSubstr(absl::StrCat(Basename(inner_dir.path()))),
- Not(HasSubstr(outer_dir.path())),
- Not(HasSubstr(inner_dir.path()))));
- std::vector<absl::string_view> submounts =
- absl::StrSplit(contents, '\n', absl::SkipWhitespace());
- TEST_CHECK(submounts.size() == 2);
+ ssize_t n = ReadFd(proc_file.get(), buf, sizeof(buf));
+ TEST_PCHECK(n >= 0);
+ buf[n] = '\0';
+ TEST_CHECK(absl::StrContains(buf, Basename(inner_dir_path)));
+ TEST_CHECK(!absl::StrContains(buf, outer_dir_path));
+ TEST_CHECK(!absl::StrContains(buf, inner_dir_path));
+ TEST_CHECK(std::count(buf, buf + n, '\n') == 2);
}
// Chroot to inner mount. We must use an absolute path accessible to our
// chroot.
- const std::string inner_dir_basename =
- absl::StrCat("/", Basename(inner_dir.path()));
- TEST_CHECK_SUCCESS(chroot(inner_dir_basename.c_str()));
+ TEST_CHECK_SUCCESS(chroot(inner_dir_in_outer_chroot_path.c_str()));
for (const std::string& path : paths) {
const FileDescriptor proc_file =
TEST_CHECK_NO_ERRNO_AND_VALUE(OpenAt(proc.get(), path, O_RDONLY));
- const std::string contents =
- TEST_CHECK_NO_ERRNO_AND_VALUE(GetContentsFD(proc_file.get()));
// Only the inner mount visible from this chroot.
- std::vector<absl::string_view> submounts =
- absl::StrSplit(contents, '\n', absl::SkipWhitespace());
- TEST_CHECK(submounts.size() == 1);
+ ssize_t n = ReadFd(proc_file.get(), buf, sizeof(buf));
+ TEST_PCHECK(n >= 0);
+ buf[n] = '\0';
+ TEST_CHECK(std::count(buf, buf + n, '\n') == 1);
}
};
EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0));
--
cgit v1.2.3