From 0ec4f07f9677fe8fe5338e8415a6ac8e9c2b6c0b Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Tue, 22 Jun 2021 07:48:25 -0700 Subject: [PATCH] Fix rare failure while starting the global forkserver This bug only manifests if a lot of fds are open when global forkserver is started. If the allocated exec_fd number was equal Comms::kSandbox2ClientCommsFD then it would be replaced by the comms fd and result in EACCESS at execveat. PiperOrigin-RevId: 380805414 Change-Id: I31427fa929abfc60890477b55790cc14c749f7f5 --- sandboxed_api/sandbox2/BUILD.bazel | 1 - sandboxed_api/sandbox2/CMakeLists.txt | 1 - sandboxed_api/sandbox2/global_forkclient.cc | 22 +++++++++++++-------- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index b677e70..4c8e9ef 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/sandboxed_api/sandbox2/BUILD.bazel @@ -225,7 +225,6 @@ cc_library( ":forkserver_cc_proto", ":util", "//sandboxed_api:embed_file", - "//sandboxed_api/util:fileops", "//sandboxed_api/util:flags", "//sandboxed_api/util:raw_logging", "//sandboxed_api/util:strerror", diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index f8f58ab..f3425a8 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -226,7 +226,6 @@ target_link_libraries(sandbox2_global_forkserver absl::strings glog::glog sandbox2::client - sapi::fileops sandbox2::forkserver_bin_embed sapi::strerror sandbox2::util diff --git a/sandboxed_api/sandbox2/global_forkclient.cc b/sandboxed_api/sandbox2/global_forkclient.cc index af0286b..505372b 100644 --- a/sandboxed_api/sandbox2/global_forkclient.cc +++ b/sandboxed_api/sandbox2/global_forkclient.cc @@ -41,14 +41,11 @@ #include "sandboxed_api/sandbox2/fork_client.h" #include "sandboxed_api/sandbox2/forkserver_bin_embed.h" #include "sandboxed_api/sandbox2/util.h" -#include "sandboxed_api/util/fileops.h" #include "sandboxed_api/util/raw_logging.h" #include "sandboxed_api/util/strerror.h" namespace sandbox2 { -namespace file_util = ::sapi::file_util; - bool AbslParseFlag(absl::string_view text, GlobalForkserverStartModeSet* out, std::string* error) { *out = {}; @@ -120,10 +117,10 @@ GlobalForkserverStartModeSet GetForkserverStartMode() { } std::unique_ptr StartGlobalForkServer() { - file_util::fileops::FDCloser exec_fd( - sapi::EmbedFile::GetEmbedFileSingleton()->GetDupFdForFileToc( - forkserver_bin_embed_create())); - SAPI_RAW_CHECK(exec_fd.get() >= 0, "Getting FD for init binary failed"); + // The fd is owned by EmbedFile + int exec_fd = sapi::EmbedFile::GetEmbedFileSingleton()->GetFdForFileToc( + forkserver_bin_embed_create()); + SAPI_RAW_CHECK(exec_fd >= 0, "Getting FD for init binary failed"); std::string proc_name = "S2-FORK-SERV"; @@ -139,11 +136,20 @@ std::unique_ptr StartGlobalForkServer() { if (pid == 0) { // Move the comms FD to the proper, expected FD number. // The new FD will not be CLOEXEC, which is what we want. + // If exec_fd == Comms::kSandbox2ClientCommsFD then it would be replaced by + // the comms fd and result in EACCESS at execveat. + // So first move exec_fd also making sure it will not clash with sv[0]... + int new_exec_fd = Comms::kSandbox2ClientCommsFD + 1; + if (sv[0] == new_exec_fd) { + ++new_exec_fd; + } + dup2(exec_fd, new_exec_fd); + fcntl(new_exec_fd, F_SETFD, FD_CLOEXEC); dup2(sv[0], Comms::kSandbox2ClientCommsFD); char* const args[] = {proc_name.data(), nullptr}; char* const envp[] = {nullptr}; - syscall(__NR_execveat, exec_fd.get(), "", args, envp, AT_EMPTY_PATH); + syscall(__NR_execveat, new_exec_fd, "", args, envp, AT_EMPTY_PATH); SAPI_RAW_PLOG(FATAL, "Could not launch forkserver binary"); abort(); }