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

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<const char*>(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,

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 << "]";

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<std::string> args{lib_path};
+    std::vector<std::string> args = {lib_path};
     // Additional arguments, if needed.
     GetArgs(&args);
     std::vector<std::string> 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<v::Callable*> 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<std::string> 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 {
+absl::Status Sandbox::SetWallTimeLimit(absl::Duration limit) const {
-  if (!IsActive()) {
+  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_->set_walltime_limit(absl::Seconds(1));
-  s2_->SetWallTimeLimit(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();
   }
 }

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?
+  ABSL_DEPRECATED("Use sapi::Sandbox::is_active() instead")
-  bool IsActive() const;
+  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<std::string> GetCString(const v::RemotePtr& str,
+  sapi::StatusOr<std::string> GetCString(
-                                           uint64_t max_length = 10 * 1024 *
+      const v::RemotePtr& str, uint64_t max_length = 10ULL << 20 /* 10 MiB*/
-                                                                 1024);
+  );
 
   // 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<RPCChannel> 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).

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(absl::ZeroDuration()).IgnoreError();
-        capture->sandbox_->SetWallTimeLimit(0).IgnoreError();
-      }
     }
     TransactionBase* capture;
   } sandbox_cleanup = {this};
@@ -66,8 +64,11 @@ absl::Status TransactionBase::RunTransactionLoop(
 }
 
 TransactionBase::~TransactionBase() {
-  if (initialized_) {
+  if (!initialized_) {
-    Finish().IgnoreError();
+    return;
+  }
+  if (absl::Status status = Finish(); !status.ok()) {
+    LOG(ERROR) << "Transaction finalizer returned an error: " << status;
   }
 }
 

sandboxed_api/transaction.h

@@ -86,9 +86,7 @@ class TransactionBase {
 
  protected:
   explicit TransactionBase(std::unique_ptr<Sandbox> sandbox)
-      : retry_count_(kDefaultRetryCount),
+      : time_limit_(absl::ToTimeT(absl::UnixEpoch() + kDefaultTimeLimit)),
-        time_limit_(absl::ToTimeT(absl::UnixEpoch() + kDefaultTimeLimit)),
-        initialized_(false),
         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> sandbox_;