From f7d3f442dfc6fdc88c2ec756480ec477f3d66382 Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Fri, 17 Jul 2020 04:54:20 -0700 Subject: [PATCH] Extract ForkClient to a separate target PiperOrigin-RevId: 321757582 Change-Id: I48b89ab4e4b1d87dd9444874de5bf5bd2526531a --- sandboxed_api/sandbox2/BUILD.bazel | 30 +++++--- sandboxed_api/sandbox2/CMakeLists.txt | 23 ++++-- sandboxed_api/sandbox2/executor.cc | 2 +- sandboxed_api/sandbox2/executor.h | 3 +- sandboxed_api/sandbox2/fork_client.cc | 77 +++++++++++++++++++++ sandboxed_api/sandbox2/fork_client.h | 50 +++++++++++++ sandboxed_api/sandbox2/forkserver.cc | 56 +-------------- sandboxed_api/sandbox2/forkserver.h | 22 ------ sandboxed_api/sandbox2/global_forkclient.cc | 1 - sandboxed_api/sandbox2/global_forkclient.h | 4 +- 10 files changed, 174 insertions(+), 94 deletions(-) create mode 100644 sandboxed_api/sandbox2/fork_client.cc create mode 100644 sandboxed_api/sandbox2/fork_client.h diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index 81916d0..09dbab4 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/sandboxed_api/sandbox2/BUILD.bazel @@ -211,7 +211,7 @@ cc_library( copts = sapi_platform_copts(), deps = [ ":comms", - ":forkserver", + ":fork_client", ":forkserver_bin_embed", ":sanitizer", "//sandboxed_api:embed_file", @@ -228,7 +228,7 @@ cc_library( hdrs = ["executor.h"], copts = sapi_platform_copts(), deps = [ - ":forkserver", + ":fork_client", ":forkserver_cc_proto", ":global_forkserver", ":ipc", @@ -273,7 +273,7 @@ cc_library( ":client", ":comms", ":executor", - ":forkserver", + ":fork_client", ":forkserver_cc_proto", ":global_forkserver", ":ipc", @@ -362,6 +362,7 @@ cc_library( deps = [ ":client", ":comms", + ":fork_client", ":forkserver_cc_proto", ":namespace", ":policy", @@ -369,7 +370,6 @@ cc_library( ":syscall", ":util", "//sandboxed_api/sandbox2/unwind", - "//sandboxed_api/sandbox2/unwind:unwind_cc_proto", "//sandboxed_api/sandbox2/util:bpf_helper", "//sandboxed_api/sandbox2/util:fileops", "//sandboxed_api/sandbox2/util:strerror", @@ -379,11 +379,25 @@ cc_library( "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:str_format", - "@com_google_absl//absl/synchronization", "@org_kernel_libcap//:libcap", ], ) +cc_library( + name = "fork_client", + srcs = ["fork_client.cc"], + hdrs = ["fork_client.h"], + copts = sapi_platform_copts(), + visibility = ["//visibility:public"], + deps = [ + ":comms", + ":forkserver_cc_proto", + "//sandboxed_api/util:raw_logging", + "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/synchronization", + ], +) + cc_library( name = "mounts", srcs = ["mounts.cc"], @@ -593,16 +607,14 @@ cc_test( cc_test( name = "forkserver_test", - srcs = [ - "forkserver_test.cc", - "global_forkclient.h", - ], + srcs = ["forkserver_test.cc"], copts = sapi_platform_copts(), data = ["//sandboxed_api/sandbox2/testcases:minimal"], deps = [ ":comms", ":forkserver", ":forkserver_cc_proto", + ":global_forkserver", ":sandbox2", ":testing", "@com_google_absl//absl/strings", diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index cc36252..f5919ec 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -221,7 +221,7 @@ target_link_libraries(sandbox2_global_forkserver PRIVATE absl::strings sandbox2::client sandbox2::comms - sandbox2::forkserver + sandbox2::fork_client sandbox2::forkserver_bin_embed sandbox2::sanitizer sandbox2::strerror @@ -242,7 +242,7 @@ target_link_libraries(sandbox2_executor PRIVATE absl::strings glog::glog sandbox2::fileops - sandbox2::forkserver + sandbox2::fork_client sandbox2::forkserver_proto sandbox2::global_forkserver sandbox2::ipc @@ -281,7 +281,7 @@ target_link_libraries(sandbox2_sandbox2 sandbox2::executor sandbox2::file_base sandbox2::fileops - sandbox2::forkserver + sandbox2::fork_client sandbox2::forkserver_proto sandbox2::global_forkserver sandbox2::ipc @@ -353,12 +353,12 @@ target_link_libraries(sandbox2_forkserver PRIVATE absl::memory absl::str_format absl::strings - absl::synchronization libcap::libcap sandbox2::bpf_helper sandbox2::client sandbox2::comms sandbox2::fileops + sandbox2::fork_client sandbox2::forkserver_proto sandbox2::namespace sandbox2::policy @@ -372,6 +372,21 @@ target_link_libraries(sandbox2_forkserver PRIVATE sapi::statusor ) +# sandboxed_api/sandbox2:fork_client +add_library(sandbox2_fork_client STATIC + fork_client.cc + fork_client.h +) +add_library(sandbox2::fork_client ALIAS sandbox2_fork_client) +target_link_libraries(sandbox2_fork_client PRIVATE + absl::core_headers + absl::synchronization + sandbox2::comms + sandbox2::forkserver_proto + sapi::base + sapi::raw_logging +) + # sandboxed_api/sandbox2:mounts add_library(sandbox2_mounts STATIC mounts.cc diff --git a/sandboxed_api/sandbox2/executor.cc b/sandboxed_api/sandbox2/executor.cc index fa886c4..6170a9d 100644 --- a/sandboxed_api/sandbox2/executor.cc +++ b/sandboxed_api/sandbox2/executor.cc @@ -26,7 +26,7 @@ #include "absl/memory/memory.h" #include "absl/strings/str_cat.h" -#include "sandboxed_api/sandbox2/forkserver.h" +#include "sandboxed_api/sandbox2/fork_client.h" #include "sandboxed_api/sandbox2/forkserver.pb.h" #include "sandboxed_api/sandbox2/global_forkclient.h" #include "sandboxed_api/sandbox2/ipc.h" diff --git a/sandboxed_api/sandbox2/executor.h b/sandboxed_api/sandbox2/executor.h index 505b381..86dec1b 100644 --- a/sandboxed_api/sandbox2/executor.h +++ b/sandboxed_api/sandbox2/executor.h @@ -17,13 +17,14 @@ #include #include + #include #include #include #include #include "absl/base/macros.h" -#include "sandboxed_api/sandbox2/forkserver.h" +#include "sandboxed_api/sandbox2/fork_client.h" #include "sandboxed_api/sandbox2/ipc.h" #include "sandboxed_api/sandbox2/limits.h" #include "sandboxed_api/sandbox2/namespace.h" diff --git a/sandboxed_api/sandbox2/fork_client.cc b/sandboxed_api/sandbox2/fork_client.cc new file mode 100644 index 0000000..567faa8 --- /dev/null +++ b/sandboxed_api/sandbox2/fork_client.cc @@ -0,0 +1,77 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "sandboxed_api/sandbox2/fork_client.h" + +#include "sandboxed_api/sandbox2/comms.h" +#include "sandboxed_api/sandbox2/forkserver.pb.h" +#include "sandboxed_api/util/raw_logging.h" + +namespace sandbox2 { + +const char kForkServerDisableEnv[] = "SANDBOX2_NOFORKSERVER"; + +pid_t ForkClient::SendRequest(const ForkRequest& request, int exec_fd, + int comms_fd, int user_ns_fd, pid_t* init_pid) { + // Acquire the channel ownership for this request (transaction). + absl::MutexLock l(&comms_mutex_); + + if (!comms_->SendProtoBuf(request)) { + SAPI_RAW_LOG(ERROR, "Sending PB to the ForkServer failed"); + return -1; + } + SAPI_RAW_CHECK(comms_fd != -1, "comms_fd was not properly set up"); + if (!comms_->SendFD(comms_fd)) { + SAPI_RAW_LOG(ERROR, "Sending Comms FD (%d) to the ForkServer failed", + comms_fd); + return -1; + } + if (request.mode() == FORKSERVER_FORK_EXECVE || + request.mode() == FORKSERVER_FORK_EXECVE_SANDBOX) { + SAPI_RAW_CHECK(exec_fd != -1, "exec_fd cannot be -1 in execve mode"); + if (!comms_->SendFD(exec_fd)) { + SAPI_RAW_LOG(ERROR, "Sending Exec FD (%d) to the ForkServer failed", + exec_fd); + return -1; + } + } + + if (request.mode() == FORKSERVER_FORK_JOIN_SANDBOX_UNWIND) { + SAPI_RAW_CHECK(user_ns_fd != -1, "user_ns_fd cannot be -1 in unwind mode"); + if (!comms_->SendFD(user_ns_fd)) { + SAPI_RAW_LOG(ERROR, "Sending user ns FD (%d) to the ForkServer failed", + user_ns_fd); + return -1; + } + } + + int32_t pid; + // Receive init process ID. + if (!comms_->RecvInt32(&pid)) { + SAPI_RAW_LOG(ERROR, "Receiving init PID from the ForkServer failed"); + return -1; + } + if (init_pid) { + *init_pid = static_cast(pid); + } + + // Receive sandboxee process ID. + if (!comms_->RecvInt32(&pid)) { + SAPI_RAW_LOG(ERROR, "Receiving sandboxee PID from the ForkServer failed"); + return -1; + } + return static_cast(pid); +} + +} // namespace sandbox2 diff --git a/sandboxed_api/sandbox2/fork_client.h b/sandboxed_api/sandbox2/fork_client.h new file mode 100644 index 0000000..cdfa3de --- /dev/null +++ b/sandboxed_api/sandbox2/fork_client.h @@ -0,0 +1,50 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef SANDBOXED_API_SANDBOX2_FORK_CLIENT_H_ +#define SANDBOXED_API_SANDBOX2_FORK_CLIENT_H_ + +#include + +#include "absl/base/attributes.h" +#include "absl/synchronization/mutex.h" + +namespace sandbox2 { + +// Envvar indicating that this process should not start the fork-server. +ABSL_CONST_INIT extern const char kForkServerDisableEnv[]; + +class Comms; +class ForkRequest; + +class ForkClient { + public: + ForkClient(const ForkClient&) = delete; + ForkClient& operator=(const ForkClient&) = delete; + + explicit ForkClient(Comms* comms) : comms_(comms) {} + + // Sends the fork request over the supplied Comms channel. + pid_t SendRequest(const ForkRequest& request, int exec_fd, int comms_fd, + int user_ns_fd = -1, pid_t* init_pid = nullptr); + + private: + // Comms channel connecting with the ForkServer. Not owned by the object. + Comms* comms_; + // Mutex locking transactions (requests) over the Comms channel. + absl::Mutex comms_mutex_; +}; +} // namespace sandbox2 + +#endif // SANDBOXED_API_SANDBOX2_FORK_CLIENT_H_ diff --git a/sandboxed_api/sandbox2/forkserver.cc b/sandboxed_api/sandbox2/forkserver.cc index e9e8172..2125927 100644 --- a/sandboxed_api/sandbox2/forkserver.cc +++ b/sandboxed_api/sandbox2/forkserver.cc @@ -34,24 +34,22 @@ #include #include -#include #include "absl/memory/memory.h" #include "absl/status/status.h" #include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" -#include "absl/synchronization/mutex.h" #include "libcap/include/sys/capability.h" #include "sandboxed_api/sandbox2/client.h" #include "sandboxed_api/sandbox2/comms.h" +#include "sandboxed_api/sandbox2/fork_client.h" #include "sandboxed_api/sandbox2/forkserver.pb.h" #include "sandboxed_api/sandbox2/namespace.h" #include "sandboxed_api/sandbox2/policy.h" #include "sandboxed_api/sandbox2/sanitizer.h" #include "sandboxed_api/sandbox2/syscall.h" #include "sandboxed_api/sandbox2/unwind/unwind.h" -#include "sandboxed_api/sandbox2/unwind/unwind.pb.h" #include "sandboxed_api/sandbox2/util.h" #include "sandboxed_api/sandbox2/util/bpf_helper.h" #include "sandboxed_api/sandbox2/util/fileops.h" @@ -178,58 +176,6 @@ sapi::StatusOr ReceivePid(int signaling_fd) { namespace sandbox2 { -pid_t ForkClient::SendRequest(const ForkRequest& request, int exec_fd, - int comms_fd, int user_ns_fd, pid_t* init_pid) { - // Acquire the channel ownership for this request (transaction). - absl::MutexLock l(&comms_mutex_); - - if (!comms_->SendProtoBuf(request)) { - SAPI_RAW_LOG(ERROR, "Sending PB to the ForkServer failed"); - return -1; - } - SAPI_RAW_CHECK(comms_fd != -1, "comms_fd was not properly set up"); - if (!comms_->SendFD(comms_fd)) { - SAPI_RAW_LOG(ERROR, "Sending Comms FD (%d) to the ForkServer failed", - comms_fd); - return -1; - } - if (request.mode() == FORKSERVER_FORK_EXECVE || - request.mode() == FORKSERVER_FORK_EXECVE_SANDBOX) { - SAPI_RAW_CHECK(exec_fd != -1, "exec_fd cannot be -1 in execve mode"); - if (!comms_->SendFD(exec_fd)) { - SAPI_RAW_LOG(ERROR, "Sending Exec FD (%d) to the ForkServer failed", - exec_fd); - return -1; - } - } - - if (request.mode() == FORKSERVER_FORK_JOIN_SANDBOX_UNWIND) { - SAPI_RAW_CHECK(user_ns_fd != -1, "user_ns_fd cannot be -1 in unwind mode"); - if (!comms_->SendFD(user_ns_fd)) { - SAPI_RAW_LOG(ERROR, "Sending user ns FD (%d) to the ForkServer failed", - user_ns_fd); - return -1; - } - } - - int32_t pid; - // Receive init process ID. - if (!comms_->RecvInt32(&pid)) { - SAPI_RAW_LOG(ERROR, "Receiving init PID from the ForkServer failed"); - return -1; - } - if (init_pid) { - *init_pid = static_cast(pid); - } - - // Receive sandboxee process ID. - if (!comms_->RecvInt32(&pid)) { - SAPI_RAW_LOG(ERROR, "Receiving sandboxee PID from the ForkServer failed"); - return -1; - } - return static_cast(pid); -} - void ForkServer::PrepareExecveArgs(const ForkRequest& request, std::vector* args, std::vector* envp) { diff --git a/sandboxed_api/sandbox2/forkserver.h b/sandboxed_api/sandbox2/forkserver.h index 1dc1657..acf9852 100644 --- a/sandboxed_api/sandbox2/forkserver.h +++ b/sandboxed_api/sandbox2/forkserver.h @@ -24,34 +24,12 @@ #include #include -#include "absl/synchronization/mutex.h" namespace sandbox2 { class Comms; class ForkRequest; -// Envvar indicating that this process should not start the fork-server. -static constexpr const char* kForkServerDisableEnv = "SANDBOX2_NOFORKSERVER"; - -class ForkClient { - public: - ForkClient(const ForkClient&) = delete; - ForkClient& operator=(const ForkClient&) = delete; - - explicit ForkClient(Comms* comms) : comms_(comms) {} - - // Sends the fork request over the supplied Comms channel. - pid_t SendRequest(const ForkRequest& request, int exec_fd, int comms_fd, - int user_ns_fd = -1, pid_t* init_pid = nullptr); - - private: - // Comms channel connecting with the ForkServer. Not owned by the object. - Comms* comms_; - // Mutex locking transactions (requests) over the Comms channel. - absl::Mutex comms_mutex_; -}; - class ForkServer { public: ForkServer(const ForkServer&) = delete; diff --git a/sandboxed_api/sandbox2/global_forkclient.cc b/sandboxed_api/sandbox2/global_forkclient.cc index c87d65b..05063c1 100644 --- a/sandboxed_api/sandbox2/global_forkclient.cc +++ b/sandboxed_api/sandbox2/global_forkclient.cc @@ -29,7 +29,6 @@ #include "absl/strings/str_cat.h" #include "sandboxed_api/embed_file.h" #include "sandboxed_api/sandbox2/comms.h" -#include "sandboxed_api/sandbox2/forkserver.h" #include "sandboxed_api/sandbox2/forkserver_bin_embed.h" #include "sandboxed_api/sandbox2/sanitizer.h" #include "sandboxed_api/sandbox2/util/strerror.h" diff --git a/sandboxed_api/sandbox2/global_forkclient.h b/sandboxed_api/sandbox2/global_forkclient.h index 335f660..00ab359 100644 --- a/sandboxed_api/sandbox2/global_forkclient.h +++ b/sandboxed_api/sandbox2/global_forkclient.h @@ -18,7 +18,9 @@ #ifndef SANDBOXED_API_SANDBOX2_GLOBAL_FORKCLIENT_H_ #define SANDBOXED_API_SANDBOX2_GLOBAL_FORKCLIENT_H_ -#include "sandboxed_api/sandbox2/forkserver.h" +#include + +#include "sandboxed_api/sandbox2/fork_client.h" namespace sandbox2 {