From d578b18c2288f8ff0ea5cb4c017ef7d7f448cb86 Mon Sep 17 00:00:00 2001 From: Christian Blichmann Date: Tue, 18 Feb 2020 05:27:08 -0800 Subject: [PATCH] Modernize the transaction API PiperOrigin-RevId: 295712938 Change-Id: Iaf4c9668bb0b48555679fef822fe424277540d1f --- sandboxed_api/examples/sum/main_sum.cc | 20 +++++------ sandboxed_api/sapi_test.cc | 6 ++-- sandboxed_api/transaction.cc | 26 +++++++------- sandboxed_api/transaction.h | 47 +++++++++++++------------- 4 files changed, 49 insertions(+), 50 deletions(-) diff --git a/sandboxed_api/examples/sum/main_sum.cc b/sandboxed_api/examples/sum/main_sum.cc index adb1698..662acd9 100644 --- a/sandboxed_api/examples/sum/main_sum.cc +++ b/sandboxed_api/examples/sum/main_sum.cc @@ -58,7 +58,7 @@ class SumTransaction : public sapi::Transaction { }; sapi::Status SumTransaction::Main() { - SumApi f(GetSandbox()); + SumApi f(sandbox()); SAPI_ASSIGN_OR_RETURN(int v, f.sum(1000, 337)); LOG(INFO) << "1000 + 337 = " << v; TRANSACTION_FAIL_IF_NOT(v == 1337, "1000 + 337 != 1337"); @@ -89,10 +89,10 @@ sapi::Status SumTransaction::Main() { // Gets symbol address and prints its value. int* ssaddr; SAPI_RETURN_IF_ERROR( - GetSandbox()->Symbol("sumsymbol", reinterpret_cast(&ssaddr))); + sandbox()->Symbol("sumsymbol", reinterpret_cast(&ssaddr))); sapi::v::Int sumsymbol; sumsymbol.SetRemote(ssaddr); - SAPI_RETURN_IF_ERROR(GetSandbox()->TransferFromSandboxee(&sumsymbol)); + SAPI_RETURN_IF_ERROR(sandbox()->TransferFromSandboxee(&sumsymbol)); LOG(INFO) << "sumsymbol value (exp: 5): " << sumsymbol.GetValue() << ", address: " << ssaddr; TRANSACTION_FAIL_IF_NOT(sumsymbol.GetValue() == 5, @@ -120,7 +120,7 @@ sapi::Status SumTransaction::Main() { LOG(INFO) << "Print: '" << hwstr << "' via puts()"; sapi::v::Array hwarr(hwstr, sizeof(hwstr)); sapi::v::Int ret; - SAPI_RETURN_IF_ERROR(GetSandbox()->Call("puts", &ret, hwarr.PtrBefore())); + SAPI_RETURN_IF_ERROR(sandbox()->Call("puts", &ret, hwarr.PtrBefore())); TRANSACTION_FAIL_IF_NOT(ret.GetValue() == 15, "puts('Hello World!!!') != 15"); sapi::v::Int vp; @@ -144,13 +144,13 @@ sapi::Status SumTransaction::Main() { // Fd transfer test. int fdesc = open("/proc/self/exe", O_RDONLY); sapi::v::Fd fd(fdesc); - SAPI_RETURN_IF_ERROR(GetSandbox()->TransferToSandboxee(&fd)); + SAPI_RETURN_IF_ERROR(sandbox()->TransferToSandboxee(&fd)); LOG(INFO) << "remote_fd = " << fd.GetRemoteFd(); TRANSACTION_FAIL_IF_NOT(fd.GetRemoteFd() == 3, "remote_fd != 3"); fdesc = open("/proc/self/comm", O_RDONLY); sapi::v::Fd fd2(fdesc); - SAPI_RETURN_IF_ERROR(GetSandbox()->TransferToSandboxee(&fd2)); + SAPI_RETURN_IF_ERROR(sandbox()->TransferToSandboxee(&fd2)); LOG(INFO) << "remote_fd2 = " << fd2.GetRemoteFd(); TRANSACTION_FAIL_IF_NOT(fd2.GetRemoteFd() == 4, "remote_fd2 != 4"); @@ -158,19 +158,19 @@ sapi::Status SumTransaction::Main() { char buffer[1024] = {0}; sapi::v::Array buf(buffer, sizeof(buffer)); sapi::v::UInt size(128); - SAPI_RETURN_IF_ERROR(GetSandbox()->Call("read", &ret, &fd2, buf.PtrBoth(), &size)); + SAPI_RETURN_IF_ERROR(sandbox()->Call("read", &ret, &fd2, buf.PtrBoth(), &size)); LOG(INFO) << "Read from /proc/self/comm = [" << buffer << "]"; // Close test. - SAPI_RETURN_IF_ERROR(fd2.CloseRemoteFd(GetSandbox()->GetRpcChannel())); + SAPI_RETURN_IF_ERROR(fd2.CloseRemoteFd(sandbox()->GetRpcChannel())); memset(buffer, 0, sizeof(buffer)); - SAPI_RETURN_IF_ERROR(GetSandbox()->Call("read", &ret, &fd2, buf.PtrBoth(), &size)); + SAPI_RETURN_IF_ERROR(sandbox()->Call("read", &ret, &fd2, buf.PtrBoth(), &size)); LOG(INFO) << "Read from closed /proc/self/comm = [" << buffer << "]"; // Pass fd as function arg example. fdesc = open("/proc/self/statm", O_RDONLY); sapi::v::Fd fd3(fdesc); - SAPI_RETURN_IF_ERROR(GetSandbox()->TransferToSandboxee(&fd3)); + SAPI_RETURN_IF_ERROR(sandbox()->TransferToSandboxee(&fd3)); SAPI_ASSIGN_OR_RETURN(int r2, f.read_int(fd3.GetRemoteFd())); LOG(INFO) << "statm value (should not be 0) = " << r2; diff --git a/sandboxed_api/sapi_test.cc b/sandboxed_api/sapi_test.cc index 758b96b..81d5e18 100644 --- a/sandboxed_api/sapi_test.cc +++ b/sandboxed_api/sapi_test.cc @@ -72,10 +72,10 @@ void BenchmarkSandboxRestartOverhead(benchmark::State& state) { BENCHMARK(BenchmarkSandboxRestartOverhead); void BenchmarkSandboxRestartForkserverOverhead(benchmark::State& state) { - sapi::BasicTransaction st{absl::make_unique()}; + sapi::BasicTransaction st(absl::make_unique()); for (auto _ : state) { EXPECT_THAT(st.Run(InvokeNop), IsOk()); - EXPECT_THAT(st.GetSandbox()->Restart(true), IsOk()); + EXPECT_THAT(st.sandbox()->Restart(true), IsOk()); } } BENCHMARK(BenchmarkSandboxRestartForkserverOverhead); @@ -84,7 +84,7 @@ void BenchmarkSandboxRestartForkserverOverheadForced(benchmark::State& state) { sapi::BasicTransaction st{absl::make_unique()}; for (auto _ : state) { EXPECT_THAT(st.Run(InvokeNop), IsOk()); - EXPECT_THAT(st.GetSandbox()->Restart(false), IsOk()); + EXPECT_THAT(st.sandbox()->Restart(false), IsOk()); } } BENCHMARK(BenchmarkSandboxRestartForkserverOverheadForced); diff --git a/sandboxed_api/transaction.cc b/sandboxed_api/transaction.cc index 800ba66..b2d4bd1 100644 --- a/sandboxed_api/transaction.cc +++ b/sandboxed_api/transaction.cc @@ -23,23 +23,23 @@ constexpr absl::Duration TransactionBase::kDefaultTimeLimit; sapi::Status TransactionBase::RunTransactionFunctionInSandbox( const std::function& f) { // Run Main(), invoking Init() if this hasn't been yet done. - SAPI_RETURN_IF_ERROR(GetSandbox()->Init()); + SAPI_RETURN_IF_ERROR(sandbox_->Init()); // Set the wall-time limit for this transaction run, and clean it up // afterwards, no matter what the result. - SAPI_RETURN_IF_ERROR(GetSandbox()->SetWallTimeLimit(GetTimeLimit())); + SAPI_RETURN_IF_ERROR(sandbox_->SetWallTimeLimit(GetTimeLimit())); struct TimeCleanup { ~TimeCleanup() { - if (capture->GetSandbox()->IsActive()) { - capture->GetSandbox()->SetWallTimeLimit(0).IgnoreError(); + if (capture->sandbox_->IsActive()) { + capture->sandbox_->SetWallTimeLimit(0).IgnoreError(); } } TransactionBase* capture; - } sandbox_cleanup{this}; + } sandbox_cleanup = {this}; - if (!GetInited()) { + if (!initialized_) { SAPI_RETURN_IF_ERROR(Init()); - SetInited(true); + initialized_ = true; } return f(); @@ -50,24 +50,24 @@ sapi::Status TransactionBase::RunTransactionLoop( // Try to run Main() for a few times, return error if none of the tries // succeeded. sapi::Status status; - for (int i = 0; i <= GetRetryCnt(); i++) { + for (int i = 0; i <= retry_count_; ++i) { status = RunTransactionFunctionInSandbox(f); if (status.ok()) { return status; } - GetSandbox()->Terminate(); - SetInited(false); + sandbox_->Terminate(); + initialized_ = false; } - LOG(ERROR) << "Tried " << (GetRetryCnt() + 1) << " times to run the " + LOG(ERROR) << "Tried " << (retry_count_ + 1) << " times to run the " << "transaction, but it failed. SAPI error: '" << status << "'. Latest sandbox error: '" - << GetSandbox()->AwaitResult().ToString() << "'"; + << sandbox_->AwaitResult().ToString() << "'"; return status; } TransactionBase::~TransactionBase() { - if (GetInited()) { + if (initialized_) { Finish().IgnoreError(); } } diff --git a/sandboxed_api/transaction.h b/sandboxed_api/transaction.h index a7916c8..4edab9e 100644 --- a/sandboxed_api/transaction.h +++ b/sandboxed_api/transaction.h @@ -51,13 +51,14 @@ class TransactionBase { public: TransactionBase(const TransactionBase&) = delete; TransactionBase& operator=(const TransactionBase&) = delete; + virtual ~TransactionBase(); - // Getter/Setter for retry_cnt_. - int GetRetryCnt() const { return retry_cnt_; } - void SetRetryCnt(int retry_count) { - CHECK_GE(retry_count, 0); - retry_cnt_ = retry_count; + // Getter/Setter for retry_count_. + int retry_count() const { return retry_count_; } + void set_retry_count(int value) { + CHECK_GE(value, 0); + retry_count_ = value; } // Getter/Setter for time_limit_. @@ -67,30 +68,28 @@ class TransactionBase { time_limit_ = absl::ToTimeT(absl::UnixEpoch() + time_limit); } - // Getter/Setter for inited_. - bool GetInited() const { return inited_; } - void SetInited(bool inited) { inited_ = inited; } + bool IsInitialized() const { return initialized_; } // Getter for the sandbox_. - Sandbox* GetSandbox() { return sandbox_.get(); } + Sandbox* sandbox() const { return sandbox_.get(); } // Restarts the sandbox. // WARNING: This will invalidate any references to the remote process, make - // sure you don't keep any var's or FD's to the remote process when calling - // this. + // sure you don't keep any vars or FDs to the remote process when + // calling this. sapi::Status Restart() { - if (inited_) { + if (initialized_) { Finish().IgnoreError(); - inited_ = false; + initialized_ = false; } return sandbox_->Restart(true); } protected: explicit TransactionBase(std::unique_ptr sandbox) - : retry_cnt_(kDefaultRetryCnt), + : retry_count_(kDefaultRetryCount), time_limit_(absl::ToTimeT(absl::UnixEpoch() + kDefaultTimeLimit)), - inited_(false), + initialized_(false), sandbox_(std::move(sandbox)) {} // Runs the main (retrying) transaction loop. @@ -98,7 +97,7 @@ class TransactionBase { private: // Number of default transaction execution re-tries, in case of failures. - static constexpr int kDefaultRetryCnt = 1; + static constexpr int kDefaultRetryCount = 1; // Wall-time limit for a single transaction execution (60 s.). static constexpr absl::Duration kDefaultTimeLimit = absl::Seconds(60); @@ -108,7 +107,7 @@ class TransactionBase { sapi::Status RunTransactionFunctionInSandbox( const std::function& f); - // Initialization routine of the sandboxed process that ill be called only + // Initialization routine of the sandboxed process that will be called only // once upon sandboxee startup. virtual sapi::Status Init() { return sapi::OkStatus(); } @@ -117,14 +116,14 @@ class TransactionBase { virtual sapi::Status Finish() { return sapi::OkStatus(); } // Number of tries this transaction will be re-executed until it succeeds. - int retry_cnt_; + int retry_count_; // 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 inited_; + bool initialized_; // The main sapi::Sandbox object. std::unique_ptr sandbox_; @@ -172,13 +171,13 @@ class BasicTransaction final : public TransactionBase { init_function_(static_cast(init_function)), finish_function_(static_cast(fini_function)) {} - // Run any function as body of the transaction that matches our expectations ( - // that is: Returning a sapi::Status and accepting a Sandbox object as first + // Run any function as body of the transaction that matches our expectations + // (that is: Returning a Status and accepting a Sandbox object as first // parameter). template sapi::Status Run(T func, Args&&... args) { return RunTransactionLoop( - [&] { return func(GetSandbox(), std::forward(args)...); }); + [&] { return func(sandbox(), std::forward(args)...); }); } private: @@ -186,11 +185,11 @@ class BasicTransaction final : public TransactionBase { FinishFunction finish_function_; sapi::Status Init() final { - return init_function_ ? init_function_(GetSandbox()) : sapi::OkStatus(); + return init_function_ ? init_function_(sandbox()) : sapi::OkStatus(); } sapi::Status Finish() final { - return finish_function_ ? finish_function_(GetSandbox()) : sapi::OkStatus(); + return finish_function_ ? finish_function_(sandbox()) : sapi::OkStatus(); } };