More efficient fork request handling and #Cleanup

- Assign to `*mutable_XXX()` instead of looping
- Use a const ref for capabilities

PiperOrigin-RevId: 384192675
Change-Id: I4db3d0c8ce0d7f6acc9fd486a2409962516b5fe7
This commit is contained in:
Christian Blichmann 2021-07-12 02:37:17 -07:00 committed by Copybara-Service
parent 372b8e2696
commit 002cb9ae01
5 changed files with 18 additions and 24 deletions

View File

@ -50,7 +50,7 @@ std::vector<std::string> Executor::CopyEnviron() {
} }
pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns, pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns,
const std::vector<int>* caps, const std::vector<int>& caps,
pid_t* init_pid_out) { pid_t* init_pid_out) {
if (started_) { if (started_) {
LOG(ERROR) << "This executor has already been started"; LOG(ERROR) << "This executor has already been started";
@ -76,12 +76,8 @@ pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns,
} }
ForkRequest request; ForkRequest request;
for (size_t i = 0; i < argv_.size(); i++) { *request.mutable_args() = {argv_.begin(), argv_.end()};
request.add_args(argv_[i]); *request.mutable_envs() = {envp_.begin(), envp_.end()};
}
for (size_t i = 0; i < envp_.size(); i++) {
request.add_envs(envp_[i]);
}
// Add LD_ORIGIN_PATH to envs, as it'll make the amount of syscalls invoked by // Add LD_ORIGIN_PATH to envs, as it'll make the amount of syscalls invoked by
// ld.so smaller. // ld.so smaller.
@ -113,15 +109,13 @@ pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns,
request.set_clone_flags(clone_flags); request.set_clone_flags(clone_flags);
if (caps) { for (auto cap : caps) {
for (auto cap : *caps) { request.add_capabilities(cap);
request.add_capabilities(cap);
}
} }
int ns_fd = -1; int ns_fd = -1;
if (libunwind_sbox_for_pid_ != 0) { if (libunwind_sbox_for_pid_ != 0) {
std::string ns_path = const std::string ns_path =
absl::StrCat("/proc/", libunwind_sbox_for_pid_, "/ns/user"); absl::StrCat("/proc/", libunwind_sbox_for_pid_, "/ns/user");
PCHECK((ns_fd = open(ns_path.c_str(), O_RDONLY)) != -1) PCHECK((ns_fd = open(ns_path.c_str(), O_RDONLY)) != -1)
<< "Could not open user ns fd (" << ns_path << ")"; << "Could not open user ns fd (" << ns_path << ")";

View File

@ -125,7 +125,7 @@ class Executor final {
// //
// Returns the same values as fork(). // Returns the same values as fork().
pid_t StartSubProcess(int clone_flags, const Namespace* ns = nullptr, pid_t StartSubProcess(int clone_flags, const Namespace* ns = nullptr,
const std::vector<int>* caps = nullptr, const std::vector<int>& caps = {},
pid_t* init_pid_out = nullptr); pid_t* init_pid_out = nullptr);
// Whether the Executor has been started yet // Whether the Executor has been started yet

View File

@ -266,7 +266,7 @@ void Monitor::Run() {
// Get PID of the sandboxee. // Get PID of the sandboxee.
pid_t init_pid = 0; pid_t init_pid = 0;
bool should_have_init = ns && (ns->GetCloneFlags() & CLONE_NEWPID); bool should_have_init = ns && (ns->GetCloneFlags() & CLONE_NEWPID);
pid_ = executor_->StartSubProcess(clone_flags, ns, policy_->GetCapabilities(), pid_ = executor_->StartSubProcess(clone_flags, ns, policy_->capabilities(),
&init_pid); &init_pid);
if (init_pid > 0) { if (init_pid > 0) {

View File

@ -169,7 +169,11 @@ void Policy::AllowUnsafeKeepCapabilities(
if (namespace_) { if (namespace_) {
namespace_->DisableUserNamespace(); namespace_->DisableUserNamespace();
} }
capabilities_ = std::move(caps); if (!caps) {
capabilities_.clear();
} else {
capabilities_ = {caps->begin(), caps->end()};
}
} }
void Policy::GetPolicyDescription(PolicyDescription* policy) const { void Policy::GetPolicyDescription(PolicyDescription* policy) const {
@ -185,10 +189,8 @@ void Policy::GetPolicyDescription(PolicyDescription* policy) const {
policy->mutable_namespace_description()); policy->mutable_namespace_description());
} }
if (capabilities_) { for (const auto& cap : capabilities_) {
for (const auto& cap : *capabilities_) { policy->add_capabilities(cap);
policy->add_capabilities(cap);
}
} }
} }

View File

@ -42,7 +42,7 @@ namespace sandbox2 {
namespace internal { namespace internal {
// Magic values of registers when executing sys_execveat, so we can recognize // Magic values of registers when executing sys_execveat, so we can recognize
// the pre-sandboxing state and notify the Monitor // the pre-sandboxing state and notify the Monitor
constexpr uintptr_t kExecveMagic = 0x921c2c34; inline constexpr uintptr_t kExecveMagic = 0x921c2c34;
} // namespace internal } // namespace internal
class Comms; class Comms;
@ -79,9 +79,7 @@ class Policy final {
namespace_ = std::move(ns); namespace_ = std::move(ns);
} }
const std::vector<int>* GetCapabilities() const { const std::vector<int>& capabilities() const { return capabilities_; }
return capabilities_.get();
}
// Returns the default policy, which blocks certain dangerous syscalls and // Returns the default policy, which blocks certain dangerous syscalls and
// mismatched syscall tables. // mismatched syscall tables.
@ -100,7 +98,7 @@ class Policy final {
bool collect_stacktrace_on_kill_ = true; bool collect_stacktrace_on_kill_ = true;
// The capabilities to keep in the sandboxee. // The capabilities to keep in the sandboxee.
std::unique_ptr<std::vector<int>> capabilities_; std::vector<int> capabilities_;
// Optional pointer to a PolicyBuilder description pb object. // Optional pointer to a PolicyBuilder description pb object.
std::unique_ptr<PolicyBuilderDescription> policy_builder_description_; std::unique_ptr<PolicyBuilderDescription> policy_builder_description_;