Clean up some resources code

More moves and fewer allocations

Bug: 237583012
Test: unit tests
Change-Id: I5cf43c8af0743c0e4d96808f1e55ceb4f02d7021

3 files changed, 46 insertions, 40 deletions

diff --git a/libs/androidfw/PosixUtils.cpp b/libs/androidfw/PosixUtils.cpp
index 026912883a73..8ddc57240129 100644
--- a/libs/androidfw/PosixUtils.cpp
+++ b/libs/androidfw/PosixUtils.cpp
@@ -17,7 +17,7 @@
 #ifdef _WIN32
 // nothing to see here
 #else
-#include <memory>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -29,45 +29,42 @@
 
 #include "androidfw/PosixUtils.h"
 
-namespace {
-
-std::unique_ptr<std::string> ReadFile(int fd) {
-  std::unique_ptr<std::string> str(new std::string());
+static std::optional<std::string> ReadFile(int fd) {
+  std::string str;
   char buf[1024];
   ssize_t r;
   while ((r = read(fd, buf, sizeof(buf))) > 0) {
-    str->append(buf, r);
+    str.append(buf, r);
   }
   if (r != 0) {
-    return nullptr;
+    return std::nullopt;
   }
-  return str;
-}
-
+  return std::move(str);
 }
 
 namespace android {
 namespace util {
 
-std::unique_ptr<ProcResult> ExecuteBinary(const std::vector<std::string>& argv) {
-  int stdout[2];  // stdout[0] read, stdout[1] write
+ProcResult ExecuteBinary(const std::vector<std::string>& argv) {
+  int stdout[2];  // [0] read, [1] write
   if (pipe(stdout) != 0) {
-    PLOG(ERROR) << "pipe";
-    return nullptr;
+    PLOG(ERROR) << "out pipe";
+    return ProcResult{-1};
   }
 
-  int stderr[2];  // stdout[0] read, stdout[1] write
+  int stderr[2];  // [0] read, [1] write
   if (pipe(stderr) != 0) {
-    PLOG(ERROR) << "pipe";
+    PLOG(ERROR) << "err pipe";
     close(stdout[0]);
     close(stdout[1]);
-    return nullptr;
+    return ProcResult{-1};
   }
 
   auto gid = getgid();
   auto uid = getuid();
 
-  char const** argv0 = (char const**)malloc(sizeof(char*) * (argv.size() + 1));
+  // better keep no C++ objects going into the child here
+  auto argv0 = (char const**)malloc(sizeof(char*) * (argv.size() + 1));
   for (size_t i = 0; i < argv.size(); i++) {
     argv0[i] = argv[i].c_str();
   }
@@ -76,8 +73,12 @@ std::unique_ptr<ProcResult> ExecuteBinary(const std::vector<std::string>& argv)
   switch (pid) {
     case -1: // error
       free(argv0);
+      close(stdout[0]);
+      close(stdout[1]);
+      close(stderr[0]);
+      close(stderr[1]);
       PLOG(ERROR) << "fork";
-      return nullptr;
+      return ProcResult{-1};
     case 0: // child
       if (setgid(gid) != 0) {
         PLOG(ERROR) << "setgid";
@@ -109,17 +110,16 @@ std::unique_ptr<ProcResult> ExecuteBinary(const std::vector<std::string>& argv)
       if (!WIFEXITED(status)) {
           close(stdout[0]);
           close(stderr[0]);
-          return nullptr;
+          return ProcResult{-1};
       }
-      std::unique_ptr<ProcResult> result(new ProcResult());
-      result->status = status;
-      const auto out = ReadFile(stdout[0]);
-      result->stdout_str = out ? *out : "";
+      ProcResult result(status);
+      auto out = ReadFile(stdout[0]);
+      result.stdout_str = out ? std::move(*out) : "";
       close(stdout[0]);
-      const auto err = ReadFile(stderr[0]);
-      result->stderr_str = err ? *err : "";
+      auto err = ReadFile(stderr[0]);
+      result.stderr_str = err ? std::move(*err) : "";
       close(stderr[0]);
-      return result;
+      return std::move(result);
   }
 }
 
diff --git a/libs/androidfw/include/androidfw/PosixUtils.h b/libs/androidfw/include/androidfw/PosixUtils.h
index bb2084740a44..c46e5e6b3fb5 100644
--- a/libs/androidfw/include/androidfw/PosixUtils.h
+++ b/libs/androidfw/include/androidfw/PosixUtils.h
@@ -25,12 +25,18 @@ struct ProcResult {
   int status;
   std::string stdout_str;
   std::string stderr_str;
+
+  explicit ProcResult(int status) : status(status) {}
+  ProcResult(ProcResult&&) noexcept = default;
+  ProcResult& operator=(ProcResult&&) noexcept = default;
+
+  explicit operator bool() const { return status >= 0; }
 };
 
-// Fork, exec and wait for an external process. Return nullptr if the process could not be launched,
-// otherwise a ProcResult containing the external process' exit status and captured stdout and
-// stderr.
-std::unique_ptr<ProcResult> ExecuteBinary(const std::vector<std::string>& argv);
+// Fork, exec and wait for an external process. Returns status < 0 if the process could not be
+// launched, otherwise a ProcResult containing the external process' exit status and captured
+// stdout and stderr.
+ProcResult ExecuteBinary(const std::vector<std::string>& argv);
 
 } // namespace util
 } // namespace android
diff --git a/libs/androidfw/tests/PosixUtils_test.cpp b/libs/androidfw/tests/PosixUtils_test.cpp
index 8c49350796ec..097e6b0bba65 100644
--- a/libs/androidfw/tests/PosixUtils_test.cpp
+++ b/libs/androidfw/tests/PosixUtils_test.cpp
@@ -28,27 +28,27 @@ namespace util {
 
 TEST(PosixUtilsTest, AbsolutePathToBinary) {
   const auto result = ExecuteBinary({"/bin/date", "--help"});
-  ASSERT_THAT(result, NotNull());
-  ASSERT_EQ(result->status, 0);
-  ASSERT_GE(result->stdout_str.find("usage: date "), 0);
+  ASSERT_TRUE((bool)result);
+  ASSERT_EQ(result.status, 0);
+  ASSERT_GE(result.stdout_str.find("usage: date "), 0);
 }
 
 TEST(PosixUtilsTest, RelativePathToBinary) {
   const auto result = ExecuteBinary({"date", "--help"});
-  ASSERT_THAT(result, NotNull());
-  ASSERT_EQ(result->status, 0);
-  ASSERT_GE(result->stdout_str.find("usage: date "), 0);
+  ASSERT_TRUE((bool)result);
+  ASSERT_EQ(result.status, 0);
+  ASSERT_GE(result.stdout_str.find("usage: date "), 0);
 }
 
 TEST(PosixUtilsTest, BadParameters) {
   const auto result = ExecuteBinary({"/bin/date", "--this-parameter-is-not-supported"});
-  ASSERT_THAT(result, NotNull());
-  ASSERT_NE(result->status, 0);
+  ASSERT_TRUE((bool)result);
+  ASSERT_GT(result.status, 0);
 }
 
 TEST(PosixUtilsTest, NoSuchBinary) {
   const auto result = ExecuteBinary({"/this/binary/does/not/exist"});
-  ASSERT_THAT(result, IsNull());
+  ASSERT_FALSE((bool)result);
 }
 
 } // android