From b76cb15f26a61783491fa35dabe0cf15d71f8155 Mon Sep 17 00:00:00 2001 From: Christian Blichmann Date: Fri, 7 Aug 2020 00:30:04 -0700 Subject: [PATCH] Rename accessors, move away from `time_t` API - `GetPid()` -> `pid()` - `GetRpcChannel()` -> `rpc_channel()` - `IsActive()` -> `is_active()` - Suggest `SetWallTimeLimit(time_t)` -> `SetWallTimeLimit(absl::Duration)` In addition, remove the protected zero-argument contructor. PiperOrigin-RevId: 325390292 Change-Id: Iba044ad5ce44e78c4064c0a09faaa4227c4d19a5 --- .../examples/stringop/main_stringop.cc | 6 +-- sandboxed_api/examples/sum/main_sum.cc | 2 +- sandboxed_api/sandbox.cc | 51 ++++++++++--------- sandboxed_api/sandbox.h | 30 +++++++---- sandboxed_api/transaction.cc | 13 ++--- sandboxed_api/transaction.h | 8 ++- 6 files changed, 61 insertions(+), 49 deletions(-) diff --git a/sandboxed_api/examples/stringop/main_stringop.cc b/sandboxed_api/examples/stringop/main_stringop.cc index d83dbf9..9880699 100644 --- a/sandboxed_api/examples/stringop/main_stringop.cc +++ b/sandboxed_api/examples/stringop/main_stringop.cc @@ -113,7 +113,7 @@ TEST(StringopTest, RawStringReversal) { { // Let's call it again with different data as argument, reusing the // existing LenVal object. - EXPECT_THAT(param.ResizeData(sandbox.GetRpcChannel(), 16), IsOk()); + EXPECT_THAT(param.ResizeData(sandbox.rpc_channel(), 16), IsOk()); memcpy(param.GetData() + 10, "ABCDEF", 6); absl::string_view data(reinterpret_cast(param.GetData()), param.GetDataSize()); @@ -135,7 +135,7 @@ TEST(StringopTest, RawStringLength) { StringopApi api(&sandbox); SAPI_ASSERT_OK_AND_ASSIGN(void* target_mem_ptr, api.get_raw_c_string()); SAPI_ASSERT_OK_AND_ASSIGN(uint64_t len, - sandbox.GetRpcChannel()->Strlen(target_mem_ptr)); + sandbox.rpc_channel()->Strlen(target_mem_ptr)); EXPECT_THAT(len, Eq(10)); } @@ -145,7 +145,7 @@ TEST(StringopTest, RawStringReading) { StringopApi api(&sandbox); SAPI_ASSERT_OK_AND_ASSIGN(void* target_mem_ptr, api.get_raw_c_string()); SAPI_ASSERT_OK_AND_ASSIGN(uint64_t len, - sandbox.GetRpcChannel()->Strlen(target_mem_ptr)); + sandbox.rpc_channel()->Strlen(target_mem_ptr)); EXPECT_THAT(len, Eq(10)); SAPI_ASSERT_OK_AND_ASSIGN(std::string data, diff --git a/sandboxed_api/examples/sum/main_sum.cc b/sandboxed_api/examples/sum/main_sum.cc index 886be2c..51e233f 100644 --- a/sandboxed_api/examples/sum/main_sum.cc +++ b/sandboxed_api/examples/sum/main_sum.cc @@ -161,7 +161,7 @@ absl::Status SumTransaction::Main() { LOG(INFO) << "Read from /proc/self/comm = [" << buffer << "]"; // Close test. - SAPI_RETURN_IF_ERROR(fd2.CloseRemoteFd(sandbox()->GetRpcChannel())); + SAPI_RETURN_IF_ERROR(fd2.CloseRemoteFd(sandbox()->rpc_channel())); memset(buffer, 0, sizeof(buffer)); SAPI_RETURN_IF_ERROR(sandbox()->Call("read", &ret, &fd2, buf.PtrBoth(), &size)); LOG(INFO) << "Read from closed /proc/self/comm = [" << buffer << "]"; diff --git a/sandboxed_api/sandbox.cc b/sandboxed_api/sandbox.cc index 79d93b1..b29646b 100644 --- a/sandboxed_api/sandbox.cc +++ b/sandboxed_api/sandbox.cc @@ -100,7 +100,7 @@ void InitDefaultPolicyBuilder(sandbox2::PolicyBuilder* builder) { } void Sandbox::Terminate(bool attempt_graceful_exit) { - if (!IsActive()) { + if (!is_active()) { return; } @@ -129,7 +129,7 @@ static std::string PathToSAPILib(const std::string& lib_path) { absl::Status Sandbox::Init() { // It's already initialized - if (IsActive()) { + if (is_active()) { return absl::OkStatus(); } @@ -156,7 +156,7 @@ absl::Status Sandbox::Init() { } } - std::vector args{lib_path}; + std::vector args = {lib_path}; // Additional arguments, if needed. GetArgs(&args); std::vector envs{}; @@ -214,24 +214,24 @@ absl::Status Sandbox::Init() { return absl::OkStatus(); } -bool Sandbox::IsActive() const { return s2_ && !s2_->IsTerminated(); } +bool Sandbox::is_active() const { return s2_ && !s2_->IsTerminated(); } absl::Status Sandbox::Allocate(v::Var* var, bool automatic_free) { - if (!IsActive()) { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } return var->Allocate(GetRpcChannel(), automatic_free); } absl::Status Sandbox::Free(v::Var* var) { - if (!IsActive()) { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } return var->Free(GetRpcChannel()); } absl::Status Sandbox::SynchronizePtrBefore(v::Callable* ptr) { - if (!IsActive()) { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } if (ptr->GetType() != v::Type::kPointer) { @@ -259,11 +259,11 @@ absl::Status Sandbox::SynchronizePtrBefore(v::Callable* ptr) { VLOG(3) << "Synchronization (TO), ptr " << p << ", Type: " << p->GetSyncType() << " for var: " << p->GetPointedVar()->ToString(); - return p->GetPointedVar()->TransferToSandboxee(GetRpcChannel(), GetPid()); + return p->GetPointedVar()->TransferToSandboxee(GetRpcChannel(), pid()); } absl::Status Sandbox::SynchronizePtrAfter(v::Callable* ptr) const { - if (!IsActive()) { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } if (ptr->GetType() != v::Type::kPointer) { @@ -287,12 +287,12 @@ absl::Status Sandbox::SynchronizePtrAfter(v::Callable* ptr) const { p->ToString())); } - return p->GetPointedVar()->TransferFromSandboxee(GetRpcChannel(), GetPid()); + return p->GetPointedVar()->TransferFromSandboxee(GetRpcChannel(), pid()); } absl::Status Sandbox::Call(const std::string& func, v::Callable* ret, std::initializer_list args) { - if (!IsActive()) { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } // Send data. @@ -371,29 +371,29 @@ absl::Status Sandbox::Call(const std::string& func, v::Callable* ret, } absl::Status Sandbox::Symbol(const char* symname, void** addr) { - if (!IsActive()) { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } return rpc_channel_->Symbol(symname, addr); } absl::Status Sandbox::TransferToSandboxee(v::Var* var) { - if (!IsActive()) { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } - return var->TransferToSandboxee(GetRpcChannel(), GetPid()); + return var->TransferToSandboxee(GetRpcChannel(), pid()); } absl::Status Sandbox::TransferFromSandboxee(v::Var* var) { - if (!IsActive()) { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } - return var->TransferFromSandboxee(GetRpcChannel(), GetPid()); + return var->TransferFromSandboxee(GetRpcChannel(), pid()); } sapi::StatusOr Sandbox::GetCString(const v::RemotePtr& str, uint64_t max_length) { - if (!IsActive()) { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } @@ -436,22 +436,25 @@ const sandbox2::Result& Sandbox::AwaitResult() { return result_; } -absl::Status Sandbox::SetWallTimeLimit(time_t limit) const { - if (!IsActive()) { +absl::Status Sandbox::SetWallTimeLimit(absl::Duration limit) const { + if (!is_active()) { return absl::UnavailableError("Sandbox not active"); } - s2_->SetWallTimeLimit(limit); + s2_->set_walltime_limit(limit); return absl::OkStatus(); } +absl::Status Sandbox::SetWallTimeLimit(time_t limit) const { + return SetWallTimeLimit(absl::Seconds(limit)); +} + void Sandbox::Exit() const { - if (!IsActive()) { + if (!is_active()) { return; } - // Give it 1 second - s2_->SetWallTimeLimit(1); + s2_->set_walltime_limit(absl::Seconds(1)); if (!rpc_channel_->Exit().ok()) { - LOG(WARNING) << "rpc_channel->Exit() failed, killing PID: " << GetPid(); + LOG(WARNING) << "rpc_channel->Exit() failed, killing PID: " << pid(); s2_->Kill(); } } diff --git a/sandboxed_api/sandbox.h b/sandboxed_api/sandbox.h index cb49a45..d8f37ef 100644 --- a/sandboxed_api/sandbox.h +++ b/sandboxed_api/sandbox.h @@ -35,18 +35,21 @@ namespace sapi { // means to communicate with it (make function calls, transfer memory). class Sandbox { public: + explicit Sandbox(const FileToc* embed_lib_toc) + : embed_lib_toc_(embed_lib_toc) {} + Sandbox(const Sandbox&) = delete; Sandbox& operator=(const Sandbox&) = delete; - explicit Sandbox(const FileToc* embed_lib_toc) - : comms_(nullptr), pid_(0), embed_lib_toc_(embed_lib_toc) {} virtual ~Sandbox(); // Initializes a new sandboxing session. absl::Status Init(); - // Is the current sandboxing session alive? - bool IsActive() const; + ABSL_DEPRECATED("Use sapi::Sandbox::is_active() instead") + bool IsActive() const { return is_active(); } + // Returns whether the current sandboxing session is active. + bool is_active() const; // Terminates the current sandboxing session (if it exists). void Terminate(bool attempt_graceful_exit = true); @@ -60,9 +63,13 @@ class Sandbox { // Getters for common fields. sandbox2::Comms* comms() const { return comms_; } + ABSL_DEPRECATED("Use sapi::Sandbox::rpc_channel() instead") RPCChannel* GetRpcChannel() const { return rpc_channel_.get(); } + RPCChannel* rpc_channel() const { return rpc_channel_.get(); } + ABSL_DEPRECATED("Use sapi::Sandbox::pid() instead") int GetPid() const { return pid_; } + int pid() const { return pid_; } // Synchronizes the underlying memory for the pointer before the call. absl::Status SynchronizePtrBefore(v::Callable* ptr); @@ -87,7 +94,7 @@ class Sandbox { // Frees memory in the sandboxee. absl::Status Free(v::Var* var); - // Finds address of a symbol in the sandboxee. + // Finds the address of a symbol in the sandboxee. absl::Status Symbol(const char* symname, void** addr); // Transfers memory (both directions). Status is returned (memory transfer @@ -95,14 +102,17 @@ class Sandbox { absl::Status TransferToSandboxee(v::Var* var); absl::Status TransferFromSandboxee(v::Var* var); - sapi::StatusOr GetCString(const v::RemotePtr& str, - uint64_t max_length = 10 * 1024 * - 1024); + sapi::StatusOr GetCString( + const v::RemotePtr& str, uint64_t max_length = 10ULL << 20 /* 10 MiB*/ + ); // Waits until the sandbox terminated and returns the result. const sandbox2::Result& AwaitResult(); const sandbox2::Result& result() const { return result_; } + absl::Status SetWallTimeLimit(absl::Duration limit) const; + ABSL_DEPRECATED( + "Use sapi::Sandbox::SetWallTimeLimit(absl::Duration) overload instead") absl::Status SetWallTimeLimit(time_t limit) const; protected: @@ -144,11 +154,11 @@ class Sandbox { sandbox2::Result result_; // Comms with the sandboxee. - sandbox2::Comms* comms_; + sandbox2::Comms* comms_ = nullptr; // RPCChannel object. std::unique_ptr rpc_channel_; // The main pid of the sandboxee. - pid_t pid_; + pid_t pid_ = 0; // FileTOC with the embedded library, takes precedence over GetLibPath if // present (not nullptr). diff --git a/sandboxed_api/transaction.cc b/sandboxed_api/transaction.cc index 4ecf9a6..a17c148 100644 --- a/sandboxed_api/transaction.cc +++ b/sandboxed_api/transaction.cc @@ -26,12 +26,10 @@ absl::Status TransactionBase::RunTransactionFunctionInSandbox( // Set the wall-time limit for this transaction run, and clean it up // afterwards, no matter what the result. - SAPI_RETURN_IF_ERROR(sandbox_->SetWallTimeLimit(GetTimeLimit())); + SAPI_RETURN_IF_ERROR(sandbox_->SetWallTimeLimit(absl::Seconds(GetTimeLimit()))); struct TimeCleanup { ~TimeCleanup() { - if (capture->sandbox_->IsActive()) { - capture->sandbox_->SetWallTimeLimit(0).IgnoreError(); - } + capture->sandbox_->SetWallTimeLimit(absl::ZeroDuration()).IgnoreError(); } TransactionBase* capture; } sandbox_cleanup = {this}; @@ -66,8 +64,11 @@ absl::Status TransactionBase::RunTransactionLoop( } TransactionBase::~TransactionBase() { - if (initialized_) { - Finish().IgnoreError(); + if (!initialized_) { + return; + } + if (absl::Status status = Finish(); !status.ok()) { + LOG(ERROR) << "Transaction finalizer returned an error: " << status; } } diff --git a/sandboxed_api/transaction.h b/sandboxed_api/transaction.h index 3ff00c2..b194a80 100644 --- a/sandboxed_api/transaction.h +++ b/sandboxed_api/transaction.h @@ -86,9 +86,7 @@ class TransactionBase { protected: explicit TransactionBase(std::unique_ptr sandbox) - : retry_count_(kDefaultRetryCount), - time_limit_(absl::ToTimeT(absl::UnixEpoch() + kDefaultTimeLimit)), - initialized_(false), + : time_limit_(absl::ToTimeT(absl::UnixEpoch() + kDefaultTimeLimit)), sandbox_(std::move(sandbox)) {} // Runs the main (retrying) transaction loop. @@ -115,14 +113,14 @@ class TransactionBase { virtual absl::Status Finish() { return absl::OkStatus(); } // Number of tries this transaction will be re-executed until it succeeds. - int retry_count_; + int retry_count_ = kDefaultRetryCount; // Time (wall-time) limit for a single Run() call (in seconds). 0 means: no // wall-time limit. time_t time_limit_; // Has Init() finished with success? - bool initialized_; + bool initialized_ = false; // The main sapi::Sandbox object. std::unique_ptr sandbox_;