Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 1fc44ebb authored by Yurii Zubrytskyi's avatar Yurii Zubrytskyi
Browse files

Clean up some resources code 

More moves and fewer allocations

Bug: 237583012
Test: unit tests
Change-Id: I5cf43c8af0743c0e4d96808f1e55ceb4f02d7021
parent 1712deb1

cmds/idmap2/libidmap2/Idmap.cpp

+1 −2
Original line number Diff line number Diff line
@@ -77,8 +77,7 @@ bool WARN_UNUSED ReadString(std::istream& stream, std::string* out) { 
    return false;

  }

  uint32_t padding_size = CalculatePadding(size);

  std::string padding(padding_size, '\0');

  if (!stream.read(padding.data(), padding_size)) {

  if (padding_size != 0 && !stream.seekg(padding_size, std::ios_base::cur)) {

    return false;

  }

  *out = buf;

cmds/idmap2/tests/Idmap2BinaryTests.cpp

+40 −41
Original line number Diff line number Diff line
@@ -46,7 +46,6 @@ 


using ::android::base::StringPrintf;

using ::android::util::ExecuteBinary;

using ::testing::NotNull;



namespace android::idmap2 {
@@ -95,8 +94,8 @@ TEST_F(Idmap2BinaryTests, Create) { 
                               "--overlay-name", TestConstants::OVERLAY_NAME_DEFAULT,

                               "--idmap-path", GetIdmapPath()});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_EQ(result->status, EXIT_SUCCESS) << result->stderr_str;

  ASSERT_TRUE((bool)result);

  ASSERT_EQ(result.status, EXIT_SUCCESS) << result.stderr_str;



  struct stat st;

  ASSERT_EQ(stat(GetIdmapPath().c_str(), &st), 0);
@@ -122,33 +121,33 @@ TEST_F(Idmap2BinaryTests, Dump) { 
                               "--overlay-name", TestConstants::OVERLAY_NAME_DEFAULT,

                               "--idmap-path", GetIdmapPath()});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_EQ(result->status, EXIT_SUCCESS) << result->stderr_str;

  ASSERT_TRUE((bool)result);

  ASSERT_EQ(result.status, EXIT_SUCCESS) << result.stderr_str;



  // clang-format off

  result = ExecuteBinary({"idmap2",

                          "dump",

                          "--idmap-path", GetIdmapPath()});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_EQ(result->status, EXIT_SUCCESS) << result->stderr_str;

  ASSERT_TRUE((bool)result);

  ASSERT_EQ(result.status, EXIT_SUCCESS) << result.stderr_str;



  ASSERT_NE(result->stdout_str.find(StringPrintf("0x%08x -> 0x%08x", R::target::integer::int1,

  ASSERT_NE(result.stdout_str.find(StringPrintf("0x%08x -> 0x%08x", R::target::integer::int1,

                                                 R::overlay::integer::int1)),

            std::string::npos)

      << result->stdout_str;

  ASSERT_NE(result->stdout_str.find(StringPrintf("0x%08x -> 0x%08x", R::target::string::str1,

      << result.stdout_str;

  ASSERT_NE(result.stdout_str.find(StringPrintf("0x%08x -> 0x%08x", R::target::string::str1,

                                                 R::overlay::string::str1)),

            std::string::npos)

      << result->stdout_str;

  ASSERT_NE(result->stdout_str.find(StringPrintf("0x%08x -> 0x%08x", R::target::string::str3,

      << result.stdout_str;

  ASSERT_NE(result.stdout_str.find(StringPrintf("0x%08x -> 0x%08x", R::target::string::str3,

                                                 R::overlay::string::str3)),

            std::string::npos)

      << result->stdout_str;

  ASSERT_NE(result->stdout_str.find(StringPrintf("0x%08x -> 0x%08x", R::target::string::str4,

      << result.stdout_str;

  ASSERT_NE(result.stdout_str.find(StringPrintf("0x%08x -> 0x%08x", R::target::string::str4,

                                                 R::overlay::string::str4)),

            std::string::npos)

      << result->stdout_str;

      << result.stdout_str;



  // clang-format off

  result = ExecuteBinary({"idmap2",
@@ -156,9 +155,9 @@ TEST_F(Idmap2BinaryTests, Dump) { 
                          "--verbose",

                          "--idmap-path", GetIdmapPath()});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_EQ(result->status, EXIT_SUCCESS) << result->stderr_str;

  ASSERT_NE(result->stdout_str.find("00000000: 504d4449  magic"), std::string::npos);

  ASSERT_TRUE((bool)result);

  ASSERT_EQ(result.status, EXIT_SUCCESS) << result.stderr_str;

  ASSERT_NE(result.stdout_str.find("00000000: 504d4449  magic"), std::string::npos);



  // clang-format off

  result = ExecuteBinary({"idmap2",
@@ -166,8 +165,8 @@ TEST_F(Idmap2BinaryTests, Dump) { 
                          "--verbose",

                          "--idmap-path", GetTestDataPath() + "/DOES-NOT-EXIST"});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_NE(result->status, EXIT_SUCCESS);

  ASSERT_TRUE((bool)result);

  ASSERT_NE(result.status, EXIT_SUCCESS);



  unlink(GetIdmapPath().c_str());

}
@@ -183,8 +182,8 @@ TEST_F(Idmap2BinaryTests, Lookup) { 
                               "--overlay-name", TestConstants::OVERLAY_NAME_DEFAULT,

                               "--idmap-path", GetIdmapPath()});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_EQ(result->status, EXIT_SUCCESS) << result->stderr_str;

  ASSERT_TRUE((bool)result);

  ASSERT_EQ(result.status, EXIT_SUCCESS) << result.stderr_str;



  // clang-format off

  result = ExecuteBinary({"idmap2",
@@ -193,10 +192,10 @@ TEST_F(Idmap2BinaryTests, Lookup) { 
                          "--config", "",

                          "--resid", StringPrintf("0x%08x", R::target::string::str1)});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_EQ(result->status, EXIT_SUCCESS) << result->stderr_str;

  ASSERT_NE(result->stdout_str.find("overlay-1"), std::string::npos);

  ASSERT_EQ(result->stdout_str.find("overlay-1-sv"), std::string::npos);

  ASSERT_TRUE((bool)result);

  ASSERT_EQ(result.status, EXIT_SUCCESS) << result.stderr_str;

  ASSERT_NE(result.stdout_str.find("overlay-1"), std::string::npos);

  ASSERT_EQ(result.stdout_str.find("overlay-1-sv"), std::string::npos);



  // clang-format off

  result = ExecuteBinary({"idmap2",
@@ -205,10 +204,10 @@ TEST_F(Idmap2BinaryTests, Lookup) { 
                          "--config", "",

                          "--resid", "test.target:string/str1"});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_EQ(result->status, EXIT_SUCCESS) << result->stderr_str;

  ASSERT_NE(result->stdout_str.find("overlay-1"), std::string::npos);

  ASSERT_EQ(result->stdout_str.find("overlay-1-sv"), std::string::npos);

  ASSERT_TRUE((bool)result);

  ASSERT_EQ(result.status, EXIT_SUCCESS) << result.stderr_str;

  ASSERT_NE(result.stdout_str.find("overlay-1"), std::string::npos);

  ASSERT_EQ(result.stdout_str.find("overlay-1-sv"), std::string::npos);



  // clang-format off

  result = ExecuteBinary({"idmap2",
@@ -217,9 +216,9 @@ TEST_F(Idmap2BinaryTests, Lookup) { 
                          "--config", "sv",

                          "--resid", "test.target:string/str1"});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_EQ(result->status, EXIT_SUCCESS) << result->stderr_str;

  ASSERT_NE(result->stdout_str.find("overlay-1-sv"), std::string::npos);

  ASSERT_TRUE((bool)result);

  ASSERT_EQ(result.status, EXIT_SUCCESS) << result.stderr_str;

  ASSERT_NE(result.stdout_str.find("overlay-1-sv"), std::string::npos);



  unlink(GetIdmapPath().c_str());

}
@@ -234,8 +233,8 @@ TEST_F(Idmap2BinaryTests, InvalidCommandLineOptions) { 
  auto result = ExecuteBinary({"idmap2",

                               "create"});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_NE(result->status, EXIT_SUCCESS);

  ASSERT_TRUE((bool)result);

  ASSERT_NE(result.status, EXIT_SUCCESS);



  // missing argument to option

  // clang-format off
@@ -246,8 +245,8 @@ TEST_F(Idmap2BinaryTests, InvalidCommandLineOptions) { 
                          "--overlay-name", TestConstants::OVERLAY_NAME_DEFAULT,

                          "--idmap-path"});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_NE(result->status, EXIT_SUCCESS);

  ASSERT_TRUE((bool)result);

  ASSERT_NE(result.status, EXIT_SUCCESS);



  // invalid target apk path

  // clang-format off
@@ -258,8 +257,8 @@ TEST_F(Idmap2BinaryTests, InvalidCommandLineOptions) { 
                          "--overlay-name", TestConstants::OVERLAY_NAME_DEFAULT,

                          "--idmap-path", GetIdmapPath()});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_NE(result->status, EXIT_SUCCESS);

  ASSERT_TRUE((bool)result);

  ASSERT_NE(result.status, EXIT_SUCCESS);



  // unknown policy

  // clang-format off
@@ -271,8 +270,8 @@ TEST_F(Idmap2BinaryTests, InvalidCommandLineOptions) { 
                          "--idmap-path", GetIdmapPath(),

                          "--policy", "this-does-not-exist"});

  // clang-format on

  ASSERT_THAT(result, NotNull());

  ASSERT_NE(result->status, EXIT_SUCCESS);

  ASSERT_TRUE((bool)result);

  ASSERT_NE(result.status, EXIT_SUCCESS);

}



}  // namespace android::idmap2

cmds/idmap2/tests/TestHelpers.h

+1 −1
Original line number Diff line number Diff line
@@ -192,7 +192,7 @@ const unsigned char kIdmapRawData[] = { 
    // 0x100 string contents "test"

    0x74, 0x65, 0x73, 0x74};



const unsigned int kIdmapRawDataLen = 0x104;

constexpr unsigned int kIdmapRawDataLen = std::size(kIdmapRawData);

const unsigned int kIdmapRawDataOffset = 0x54;

const unsigned int kIdmapRawDataTargetCrc = 0x1234;

const unsigned int kIdmapRawOverlayCrc = 0x5678;

core/jni/com_android_internal_content_om_OverlayConfig.cpp

+10 −10
Original line number Diff line number Diff line
@@ -66,23 +66,23 @@ static jobjectArray createIdmap(JNIEnv* env, jclass /*clazz*/, jstring targetPat 
    argv.emplace_back("--ignore-overlayable");

  }



  const auto result = ExecuteBinary(argv);

  auto result = ExecuteBinary(argv);

  if (!result) {

      LOG(ERROR) << "failed to execute idmap2";

      return nullptr;

  }



  if (result->status != 0) {

      LOG(ERROR) << "idmap2: " << result->stderr_str;

  if (result.status != 0) {

      LOG(ERROR) << "idmap2: " << result.stderr_str;

      return nullptr;

  }



  // Return the paths of the idmaps created or updated during the idmap invocation.

  std::vector<std::string> idmap_paths;

  std::istringstream input(result->stdout_str);

  std::istringstream input(std::move(result.stdout_str));

  std::string path;

  while (std::getline(input, path)) {

    idmap_paths.push_back(path);

      idmap_paths.push_back(std::move(path));

  }



  jobjectArray array = env->NewObjectArray(idmap_paths.size(), g_stringClass, nullptr);
@@ -90,7 +90,7 @@ static jobjectArray createIdmap(JNIEnv* env, jclass /*clazz*/, jstring targetPat 
    return nullptr;

  }

  for (size_t i = 0; i < idmap_paths.size(); i++) {

    const std::string path = idmap_paths[i];

      const std::string& path = idmap_paths[i];

      jstring java_string = env->NewStringUTF(path.c_str());

      if (env->ExceptionCheck()) {

          return nullptr;

libs/androidfw/PosixUtils.cpp

+27 −27
Original line number Diff line number Diff line
@@ -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 str;

    return std::nullopt;

  }



  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);

  }

}