From 7ea515e6fcf01ec6ea35e28248d9246ce0be2f72 Mon Sep 17 00:00:00 2001
From: Tao Bao <tbao@google.com>
Date: Mon, 9 Jul 2018 15:16:13 -0700
Subject: applypatch: Fix a potential nullptr dereferencing.

Note that the code exists in debugging path only, and won't be hit
unless device has flaky flash.

Test: Run recovery_unit_test and recovery_component_test on marlin.
Change-Id: I0c71adc271f08f00e3eabd9d14cd8af3186c5bae
---
 applypatch/applypatch.cpp | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/applypatch/applypatch.cpp b/applypatch/applypatch.cpp
index e6fd5f6ae..69928bcfb 100644
--- a/applypatch/applypatch.cpp
+++ b/applypatch/applypatch.cpp
@@ -622,10 +622,13 @@ static int GenerateTarget(const FileContents& source_file, const std::unique_ptr
     SHA1(reinterpret_cast<const uint8_t*>(patch->data.data()), patch->data.size(), patch_digest);
     LOG(ERROR) << "patch size " << patch->data.size() << " SHA-1 " << short_sha1(patch_digest);
 
-    uint8_t bonus_digest[SHA_DIGEST_LENGTH];
-    SHA1(reinterpret_cast<const uint8_t*>(bonus_data->data.data()), bonus_data->data.size(),
-         bonus_digest);
-    LOG(ERROR) << "bonus size " << bonus_data->data.size() << " SHA-1 " << short_sha1(bonus_digest);
+    if (bonus_data != nullptr) {
+      uint8_t bonus_digest[SHA_DIGEST_LENGTH];
+      SHA1(reinterpret_cast<const uint8_t*>(bonus_data->data.data()), bonus_data->data.size(),
+           bonus_digest);
+      LOG(ERROR) << "bonus size " << bonus_data->data.size() << " SHA-1 "
+                 << short_sha1(bonus_digest);
+    }
 
     // TODO(b/67849209) Remove after debugging the unit test flakiness.
     if (android::base::GetMinimumLogSeverity() <= android::base::LogSeverity::DEBUG) {
-- 
cgit v1.2.3


From 7c1d426dbc4f5539929247027e4bd1c33ec63471 Mon Sep 17 00:00:00 2001
From: Tao Bao <tbao@google.com>
Date: Fri, 6 Jul 2018 23:18:14 -0700
Subject: applypatch: Restrict applypatch_check to eMMC targets.

Also fix an error-pone behavior in previous code when verifying an eMMC
target. As long as it loads the partition content successfully according
to the SHAs embedded in the filename, it shouldn't further check against
the SHAs given in the second argument. Because the loaded contents
relate to a specific partition size.

For example:

  apply_patch_check(
      "EMMC:/boot.img:src_size:src_hash:tgt_size:tgt_hash",
      "src_hash");

Assume "/boot.img" already has the desired hash of "tgt_hash", the
previous code would give wrong verification result. The issue can be
addressed by additionally listing "tgt_hash" as one of the desired SHAs
(or by applying this CL).

Bug: 110106408
Test: Run recovery_unit_test and recovery_component_test on marlin.
Change-Id: I8daafdbecd083f687e24d563ab089caa25667633
---
 applypatch/applypatch.cpp                  |  22 ++---
 applypatch/include/applypatch/applypatch.h |   7 +-
 tests/component/updater_test.cpp           |   4 +-
 tests/unit/applypatch_test.cpp             | 132 ++++++++++-------------------
 4 files changed, 66 insertions(+), 99 deletions(-)

diff --git a/applypatch/applypatch.cpp b/applypatch/applypatch.cpp
index e6fd5f6ae..6daa0d693 100644
--- a/applypatch/applypatch.cpp
+++ b/applypatch/applypatch.cpp
@@ -376,24 +376,26 @@ static int FindMatchingPatch(const uint8_t* sha1, const std::vector<std::string>
   return -1;
 }
 
-int applypatch_check(const char* filename, const std::vector<std::string>& patch_sha1s) {
-  // It's okay to specify no SHA-1s; the check will pass if the LoadFileContents is successful.
-  // (Useful for reading partitions, where the filename encodes the SHA-1s; no need to check them
-  // twice.)
+int applypatch_check(const std::string& filename, const std::vector<std::string>& sha1s) {
+  if (!android::base::StartsWith(filename, "EMMC:")) {
+    return 1;
+  }
+
+  // The check will pass if LoadPartitionContents is successful, because the filename already
+  // encodes the desired SHA-1s.
   FileContents file;
-  if (LoadFileContents(filename, &file) != 0 ||
-      (!patch_sha1s.empty() && FindMatchingPatch(file.sha1, patch_sha1s) < 0)) {
+  if (LoadPartitionContents(filename, &file) != 0) {
     LOG(INFO) << "\"" << filename << "\" doesn't have any of expected SHA-1 sums; checking cache";
 
-    // If the source file is missing or corrupted, it might be because we were killed in the middle
-    // of patching it. A copy should have been made in cache_temp_source. If that file exists and
-    // matches the SHA-1 we're looking for, the check still passes.
+    // If the partition is corrupted, it might be because we were killed in the middle of patching
+    // it. A copy should have been made in cache_temp_source. If that file exists and matches the
+    // SHA-1 we're looking for, the check still passes.
     if (LoadFileContents(Paths::Get().cache_temp_source(), &file) != 0) {
       LOG(ERROR) << "Failed to load cache file";
       return 1;
     }
 
-    if (FindMatchingPatch(file.sha1, patch_sha1s) < 0) {
+    if (FindMatchingPatch(file.sha1, sha1s) < 0) {
       LOG(ERROR) << "The cache bits don't match any SHA-1 for \"" << filename << "\"";
       return 1;
     }
diff --git a/applypatch/include/applypatch/applypatch.h b/applypatch/include/applypatch/applypatch.h
index f074e3681..92db59c3a 100644
--- a/applypatch/include/applypatch/applypatch.h
+++ b/applypatch/include/applypatch/applypatch.h
@@ -78,9 +78,10 @@ int applypatch(const char* source_filename, const char* target_filename,
                const std::vector<std::string>& patch_sha1s,
                const std::vector<std::unique_ptr<Value>>& patch_data, const Value* bonus_data);
 
-// Returns 0 if the contents of the file or the cached file match any of the given SHA-1's. Returns
-// nonzero otherwise.
-int applypatch_check(const char* filename, const std::vector<std::string>& patch_sha1s);
+// Returns 0 if the contents of the eMMC target or the cached file match any of the given SHA-1's.
+// Returns nonzero otherwise. 'filename' must refer to an eMMC partition target. It would only use
+// 'sha1s' to find a match on /cache if the hashes embedded in the filename fail to match.
+int applypatch_check(const std::string& filename, const std::vector<std::string>& sha1s);
 
 // Flashes a given image to the target partition. It verifies the target cheksum first, and will
 // return if target already has the desired hash. Otherwise it checks the checksum of the given
diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp
index 0b6b96f7c..f50e861b0 100644
--- a/tests/component/updater_test.cpp
+++ b/tests/component/updater_test.cpp
@@ -250,8 +250,10 @@ TEST_F(UpdaterTest, apply_patch_check) {
   expect("t", cmd.c_str(), kNoCause);
 
   // Multiple arguments.
+  // As long as it successfully loads the partition specified in filename, it won't check against
+  // any given SHAs.
   cmd = "apply_patch_check(\"" + filename + "\", \"wrong_sha1\", \"wrong_sha2\")";
-  expect("", cmd.c_str(), kNoCause);
+  expect("t", cmd.c_str(), kNoCause);
 
   cmd = "apply_patch_check(\"" + filename + "\", \"wrong_sha1\", \"" + src_hash +
         "\", \"wrong_sha2\")";
diff --git a/tests/unit/applypatch_test.cpp b/tests/unit/applypatch_test.cpp
index 4fbdd37fa..5cc03bc7b 100644
--- a/tests/unit/applypatch_test.cpp
+++ b/tests/unit/applypatch_test.cpp
@@ -33,7 +33,6 @@
 #include <android-base/test_utils.h>
 #include <android-base/unique_fd.h>
 #include <gtest/gtest.h>
-#include <openssl/sha.h>
 
 #include "applypatch/applypatch.h"
 #include "common/test_constants.h"
@@ -42,139 +41,102 @@
 
 using namespace std::string_literals;
 
-static void sha1sum(const std::string& fname, std::string* sha1, size_t* fsize = nullptr) {
-  ASSERT_TRUE(sha1 != nullptr);
-
-  std::string data;
-  ASSERT_TRUE(android::base::ReadFileToString(fname, &data));
-
-  if (fsize != nullptr) {
-    *fsize = data.size();
-  }
-
-  uint8_t digest[SHA_DIGEST_LENGTH];
-  SHA1(reinterpret_cast<const uint8_t*>(data.c_str()), data.size(), digest);
-  *sha1 = print_sha1(digest);
-}
-
-static void mangle_file(const std::string& fname) {
-  std::string content(1024, '\0');
-  for (size_t i = 0; i < 1024; i++) {
-    content[i] = rand() % 256;
-  }
-  ASSERT_TRUE(android::base::WriteStringToFile(content, fname));
-}
-
 class ApplyPatchTest : public ::testing::Test {
- public:
+ protected:
   void SetUp() override {
-    // set up files
     old_file = from_testdata_base("old.file");
+    FileContents old_fc;
+    ASSERT_EQ(0, LoadFileContents(old_file, &old_fc));
+    old_sha1 = print_sha1(old_fc.sha1);
+    old_size = old_fc.data.size();
+
     new_file = from_testdata_base("new.file");
-    nonexistent_file = from_testdata_base("nonexistent.file");
+    FileContents new_fc;
+    ASSERT_EQ(0, LoadFileContents(new_file, &new_fc));
+    new_sha1 = print_sha1(new_fc.sha1);
+    new_size = new_fc.data.size();
 
-    // set up SHA constants
-    sha1sum(old_file, &old_sha1, &old_size);
-    sha1sum(new_file, &new_sha1, &new_size);
     srand(time(nullptr));
     bad_sha1_a = android::base::StringPrintf("%040x", rand());
     bad_sha1_b = android::base::StringPrintf("%040x", rand());
+
+    // Reset the cache backup file.
+    Paths::Get().set_cache_temp_source("/cache/saved.file");
   }
 
   std::string old_file;
-  std::string new_file;
-  std::string nonexistent_file;
-
   std::string old_sha1;
+  size_t old_size;
+
+  std::string new_file;
   std::string new_sha1;
+  size_t new_size;
+
   std::string bad_sha1_a;
   std::string bad_sha1_b;
-
-  size_t old_size;
-  size_t new_size;
 };
 
-TEST_F(ApplyPatchTest, CheckModeSkip) {
-  std::vector<std::string> sha1s;
-  ASSERT_EQ(0, applypatch_check(&old_file[0], sha1s));
-}
-
-TEST_F(ApplyPatchTest, CheckModeSingle) {
-  std::vector<std::string> sha1s = { old_sha1 };
-  ASSERT_EQ(0, applypatch_check(&old_file[0], sha1s));
+TEST_F(ApplyPatchTest, CheckMode) {
+  std::string partition = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + old_sha1;
+  ASSERT_EQ(0, applypatch_check(partition, {}));
+  ASSERT_EQ(0, applypatch_check(partition, { old_sha1 }));
+  ASSERT_EQ(0, applypatch_check(partition, { bad_sha1_a, bad_sha1_b }));
+  ASSERT_EQ(0, applypatch_check(partition, { bad_sha1_a, old_sha1, bad_sha1_b }));
 }
 
-TEST_F(ApplyPatchTest, CheckModeMultiple) {
-  std::vector<std::string> sha1s = { bad_sha1_a, old_sha1, bad_sha1_b };
-  ASSERT_EQ(0, applypatch_check(&old_file[0], sha1s));
+TEST_F(ApplyPatchTest, CheckMode_NonEmmcTarget) {
+  ASSERT_NE(0, applypatch_check(old_file, {}));
+  ASSERT_NE(0, applypatch_check(old_file, { old_sha1 }));
+  ASSERT_NE(0, applypatch_check(old_file, { bad_sha1_a, bad_sha1_b }));
+  ASSERT_NE(0, applypatch_check(old_file, { bad_sha1_a, old_sha1, bad_sha1_b }));
 }
 
-TEST_F(ApplyPatchTest, CheckModeFailure) {
-  std::vector<std::string> sha1s = { bad_sha1_a, bad_sha1_b };
-  ASSERT_NE(0, applypatch_check(&old_file[0], sha1s));
-}
-
-TEST_F(ApplyPatchTest, CheckModeEmmcTarget) {
+TEST_F(ApplyPatchTest, CheckMode_EmmcTarget) {
   // EMMC:old_file:size:sha1 should pass the check.
   std::string src_file = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + old_sha1;
-  std::vector<std::string> sha1s;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 
   // EMMC:old_file:(size-1):sha1:(size+1):sha1 should fail the check.
   src_file = "EMMC:" + old_file + ":" + std::to_string(old_size - 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size + 1) + ":" + old_sha1;
-  ASSERT_EQ(1, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_NE(0, applypatch_check(src_file, {}));
 
   // EMMC:old_file:(size-1):sha1:size:sha1:(size+1):sha1 should pass the check.
   src_file = "EMMC:" + old_file + ":" + std::to_string(old_size - 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size) + ":" + old_sha1 + ":" + std::to_string(old_size + 1) + ":" +
              old_sha1;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 
   // EMMC:old_file:(size+1):sha1:(size-1):sha1:size:sha1 should pass the check.
   src_file = "EMMC:" + old_file + ":" + std::to_string(old_size + 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size - 1) + ":" + old_sha1 + ":" + std::to_string(old_size) + ":" +
              old_sha1;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 
   // EMMC:new_file:(size+1):old_sha1:(size-1):old_sha1:size:old_sha1:size:new_sha1
   // should pass the check.
   src_file = "EMMC:" + new_file + ":" + std::to_string(old_size + 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size - 1) + ":" + old_sha1 + ":" + std::to_string(old_size) + ":" +
              old_sha1 + ":" + std::to_string(new_size) + ":" + new_sha1;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 }
 
-class ApplyPatchCacheTest : public ApplyPatchTest {
- protected:
-  void SetUp() override {
-    ApplyPatchTest::SetUp();
-    Paths::Get().set_cache_temp_source(old_file);
-  }
-};
+TEST_F(ApplyPatchTest, CheckMode_UseBackup) {
+  std::string corrupted = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + bad_sha1_a;
+  ASSERT_NE(0, applypatch_check(corrupted, { old_sha1 }));
 
-TEST_F(ApplyPatchCacheTest, CheckCacheCorruptedSourceSingle) {
-  TemporaryFile temp_file;
-  mangle_file(temp_file.path);
-  std::vector<std::string> sha1s_single = { old_sha1 };
-  ASSERT_EQ(0, applypatch_check(temp_file.path, sha1s_single));
-  ASSERT_EQ(0, applypatch_check(nonexistent_file.c_str(), sha1s_single));
+  Paths::Get().set_cache_temp_source(old_file);
+  ASSERT_EQ(0, applypatch_check(corrupted, { old_sha1 }));
+  ASSERT_EQ(0, applypatch_check(corrupted, { bad_sha1_a, old_sha1, bad_sha1_b }));
 }
 
-TEST_F(ApplyPatchCacheTest, CheckCacheCorruptedSourceMultiple) {
-  TemporaryFile temp_file;
-  mangle_file(temp_file.path);
-  std::vector<std::string> sha1s_multiple = { bad_sha1_a, old_sha1, bad_sha1_b };
-  ASSERT_EQ(0, applypatch_check(temp_file.path, sha1s_multiple));
-  ASSERT_EQ(0, applypatch_check(nonexistent_file.c_str(), sha1s_multiple));
-}
+TEST_F(ApplyPatchTest, CheckMode_UseBackup_BothCorrupted) {
+  std::string corrupted = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + bad_sha1_a;
+  ASSERT_NE(0, applypatch_check(corrupted, {}));
+  ASSERT_NE(0, applypatch_check(corrupted, { old_sha1 }));
 
-TEST_F(ApplyPatchCacheTest, CheckCacheCorruptedSourceFailure) {
-  TemporaryFile temp_file;
-  mangle_file(temp_file.path);
-  std::vector<std::string> sha1s_failure = { bad_sha1_a, bad_sha1_b };
-  ASSERT_NE(0, applypatch_check(temp_file.path, sha1s_failure));
-  ASSERT_NE(0, applypatch_check(nonexistent_file.c_str(), sha1s_failure));
+  Paths::Get().set_cache_temp_source(old_file);
+  ASSERT_NE(0, applypatch_check(corrupted, { bad_sha1_a, bad_sha1_b }));
 }
 
 class FreeCacheTest : public ::testing::Test {
-- 
cgit v1.2.3


From 511d75962789837231459e53e86eaaa6a1ff6e06 Mon Sep 17 00:00:00 2001
From: Tao Bao <tbao@google.com>
Date: Tue, 19 Jun 2018 15:56:49 -0700
Subject: edify: Remove VAL_INVALID and move ValueType into Value class.

Test: mmma -j bootable/recovery
Test: Run recovery_component_test and recovery_unit_test on marlin.
Change-Id: I4b240e3e771c387b9694be9c0f2f74e0265ab4cb
---
 applypatch/applypatch.cpp        |  2 +-
 applypatch/applypatch_modes.cpp  | 77 ++++++++++++++++++++--------------------
 applypatch/imgpatch.cpp          | 14 ++++----
 edify/expr.cpp                   |  8 ++---
 edify/include/edify/expr.h       | 19 +++++-----
 tests/component/updater_test.cpp |  4 +--
 updater/blockimg.cpp             | 21 +++++------
 updater/install.cpp              | 22 ++++++------
 8 files changed, 82 insertions(+), 85 deletions(-)

diff --git a/applypatch/applypatch.cpp b/applypatch/applypatch.cpp
index fc29493b4..b1f5607a6 100644
--- a/applypatch/applypatch.cpp
+++ b/applypatch/applypatch.cpp
@@ -553,7 +553,7 @@ int applypatch_flash(const char* source_filename, const char* target_filename,
 static int GenerateTarget(const FileContents& source_file, const std::unique_ptr<Value>& patch,
                           const std::string& target_filename,
                           const uint8_t target_sha1[SHA_DIGEST_LENGTH], const Value* bonus_data) {
-  if (patch->type != VAL_BLOB) {
+  if (patch->type != Value::Type::BLOB) {
     LOG(ERROR) << "patch is not a blob";
     return 1;
   }
diff --git a/applypatch/applypatch_modes.cpp b/applypatch/applypatch_modes.cpp
index 6437e1be6..ec95325fc 100644
--- a/applypatch/applypatch_modes.cpp
+++ b/applypatch/applypatch_modes.cpp
@@ -81,52 +81,51 @@ static int FlashMode(const char* src_filename, const char* tgt_filename,
 }
 
 static int PatchMode(int argc, const char** argv) {
-    FileContents bonusFc;
-    Value bonus(VAL_INVALID, "");
-
-    if (argc >= 3 && strcmp(argv[1], "-b") == 0) {
-        if (LoadFileContents(argv[2], &bonusFc) != 0) {
-          LOG(ERROR) << "Failed to load bonus file " << argv[2];
-          return 1;
-        }
-        bonus.type = VAL_BLOB;
-        bonus.data = std::string(bonusFc.data.cbegin(), bonusFc.data.cend());
-        argc -= 2;
-        argv += 2;
-    }
-
-    if (argc < 4) {
-        return 2;
-    }
-
-    size_t target_size;
-    if (!android::base::ParseUint(argv[4], &target_size) || target_size == 0) {
-      LOG(ERROR) << "Failed to parse \"" << argv[4] << "\" as byte count";
+  std::unique_ptr<Value> bonus;
+  if (argc >= 3 && strcmp(argv[1], "-b") == 0) {
+    FileContents bonus_fc;
+    if (LoadFileContents(argv[2], &bonus_fc) != 0) {
+      LOG(ERROR) << "Failed to load bonus file " << argv[2];
       return 1;
     }
+    bonus = std::make_unique<Value>(Value::Type::BLOB,
+                                    std::string(bonus_fc.data.cbegin(), bonus_fc.data.cend()));
+    argc -= 2;
+    argv += 2;
+  }
 
-    // If no <src-sha1>:<patch> is provided, it is in flash mode.
-    if (argc == 5) {
-        if (bonus.type != VAL_INVALID) {
-          LOG(ERROR) << "bonus file not supported in flash mode";
-          return 1;
-        }
-        return FlashMode(argv[1], argv[2], argv[3], target_size);
-    }
+  if (argc < 4) {
+    return 2;
+  }
 
-    std::vector<std::string> sha1s;
-    std::vector<FileContents> files;
-    if (!ParsePatchArgs(argc-5, argv+5, &sha1s, &files)) {
-      LOG(ERROR) << "Failed to parse patch args";
+  size_t target_size;
+  if (!android::base::ParseUint(argv[4], &target_size) || target_size == 0) {
+    LOG(ERROR) << "Failed to parse \"" << argv[4] << "\" as byte count";
+    return 1;
+  }
+
+  // If no <src-sha1>:<patch> is provided, it is in flash mode.
+  if (argc == 5) {
+    if (bonus) {
+      LOG(ERROR) << "bonus file not supported in flash mode";
       return 1;
     }
+    return FlashMode(argv[1], argv[2], argv[3], target_size);
+  }
 
-    std::vector<std::unique_ptr<Value>> patches;
-    for (size_t i = 0; i < files.size(); ++i) {
-        patches.push_back(std::make_unique<Value>(
-                VAL_BLOB, std::string(files[i].data.cbegin(), files[i].data.cend())));
-    }
-    return applypatch(argv[1], argv[2], argv[3], target_size, sha1s, patches, &bonus);
+  std::vector<std::string> sha1s;
+  std::vector<FileContents> files;
+  if (!ParsePatchArgs(argc - 5, argv + 5, &sha1s, &files)) {
+    LOG(ERROR) << "Failed to parse patch args";
+    return 1;
+  }
+
+  std::vector<std::unique_ptr<Value>> patches;
+  for (const auto& file : files) {
+    patches.push_back(std::make_unique<Value>(Value::Type::BLOB,
+                                              std::string(file.data.cbegin(), file.data.cend())));
+  }
+  return applypatch(argv[1], argv[2], argv[3], target_size, sha1s, patches, bonus.get());
 }
 
 // This program (applypatch) applies binary patches to files in a way that
diff --git a/applypatch/imgpatch.cpp b/applypatch/imgpatch.cpp
index 2f8f4851d..da7569219 100644
--- a/applypatch/imgpatch.cpp
+++ b/applypatch/imgpatch.cpp
@@ -151,7 +151,8 @@ static bool ApplyBSDiffPatchAndStreamOutput(const uint8_t* src_data, size_t src_
 
 int ApplyImagePatch(const unsigned char* old_data, size_t old_size, const unsigned char* patch_data,
                     size_t patch_size, SinkFn sink) {
-  Value patch(VAL_BLOB, std::string(reinterpret_cast<const char*>(patch_data), patch_size));
+  Value patch(Value::Type::BLOB,
+              std::string(reinterpret_cast<const char*>(patch_data), patch_size));
   return ApplyImagePatch(old_data, old_size, patch, sink, nullptr);
 }
 
@@ -246,11 +247,10 @@ int ApplyImagePatch(const unsigned char* old_data, size_t old_size, const Value&
       // Decompress the source data; the chunk header tells us exactly
       // how big we expect it to be when decompressed.
 
-      // Note: expanded_len will include the bonus data size if
-      // the patch was constructed with bonus data.  The
-      // deflation will come up 'bonus_size' bytes short; these
-      // must be appended from the bonus_data value.
-      size_t bonus_size = (i == 1 && bonus_data != NULL) ? bonus_data->data.size() : 0;
+      // Note: expanded_len will include the bonus data size if the patch was constructed with
+      // bonus data. The deflation will come up 'bonus_size' bytes short; these must be appended
+      // from the bonus_data value.
+      size_t bonus_size = (i == 1 && bonus_data != nullptr) ? bonus_data->data.size() : 0;
 
       std::vector<unsigned char> expanded_source(expanded_len);
 
@@ -288,7 +288,7 @@ int ApplyImagePatch(const unsigned char* old_data, size_t old_size, const Value&
         inflateEnd(&strm);
 
         if (bonus_size) {
-          memcpy(expanded_source.data() + (expanded_len - bonus_size), &bonus_data->data[0],
+          memcpy(expanded_source.data() + (expanded_len - bonus_size), bonus_data->data.data(),
                  bonus_size);
         }
       }
diff --git a/edify/expr.cpp b/edify/expr.cpp
index 6823b7339..c090eb28a 100644
--- a/edify/expr.cpp
+++ b/edify/expr.cpp
@@ -51,9 +51,9 @@ bool Evaluate(State* state, const std::unique_ptr<Expr>& expr, std::string* resu
     if (!v) {
         return false;
     }
-    if (v->type != VAL_STRING) {
-        ErrorAbort(state, kArgsParsingFailure, "expecting string, got value type %d", v->type);
-        return false;
+    if (v->type != Value::Type::STRING) {
+      ErrorAbort(state, kArgsParsingFailure, "expecting string, got value type %d", v->type);
+      return false;
     }
 
     *result = v->data;
@@ -68,7 +68,7 @@ Value* StringValue(const char* str) {
     if (str == nullptr) {
         return nullptr;
     }
-    return new Value(VAL_STRING, str);
+    return new Value(Value::Type::STRING, str);
 }
 
 Value* StringValue(const std::string& str) {
diff --git a/edify/include/edify/expr.h b/edify/include/edify/expr.h
index 770d1cf0d..ccd200778 100644
--- a/edify/include/edify/expr.h
+++ b/edify/include/edify/expr.h
@@ -53,19 +53,16 @@ struct State {
   bool is_retry = false;
 };
 
-enum ValueType {
-    VAL_INVALID = -1,
-    VAL_STRING = 1,
-    VAL_BLOB = 2,
-};
-
 struct Value {
-    ValueType type;
-    std::string data;
+  enum class Type {
+    STRING = 1,
+    BLOB = 2,
+  };
+
+  Value(Type type, const std::string& str) : type(type), data(str) {}
 
-    Value(ValueType type, const std::string& str) :
-        type(type),
-        data(str) {}
+  Type type;
+  std::string data;
 };
 
 struct Expr;
diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp
index f50e861b0..8e520f337 100644
--- a/tests/component/updater_test.cpp
+++ b/tests/component/updater_test.cpp
@@ -149,11 +149,11 @@ static Value* BlobToString(const char* name, State* state,
     return nullptr;
   }
 
-  if (args[0]->type != VAL_BLOB) {
+  if (args[0]->type != Value::Type::BLOB) {
     return ErrorAbort(state, kArgsParsingFailure, "%s() expects a BLOB argument", name);
   }
 
-  args[0]->type = VAL_STRING;
+  args[0]->type = Value::Type::STRING;
   return args[0].release();
 }
 
diff --git a/updater/blockimg.cpp b/updater/blockimg.cpp
index fc713859b..6a6236b1b 100644
--- a/updater/blockimg.cpp
+++ b/updater/blockimg.cpp
@@ -1403,7 +1403,8 @@ static int PerformCommandDiff(CommandParameters& params) {
     if (status == 0) {
       LOG(INFO) << "patching " << blocks << " blocks to " << tgt.blocks();
       Value patch_value(
-          VAL_BLOB, std::string(reinterpret_cast<const char*>(params.patch_start + offset), len));
+          Value::Type::BLOB,
+          std::string(reinterpret_cast<const char*>(params.patch_start + offset), len));
 
       RangeSinkWriter writer(params.fd, tgt);
       if (params.cmdname[0] == 'i') {  // imgdiff
@@ -1531,19 +1532,19 @@ static Value* PerformBlockImageUpdate(const char* name, State* state,
   const std::unique_ptr<Value>& new_data_fn = args[2];
   const std::unique_ptr<Value>& patch_data_fn = args[3];
 
-  if (blockdev_filename->type != VAL_STRING) {
+  if (blockdev_filename->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "blockdev_filename argument to %s must be string", name);
     return StringValue("");
   }
-  if (transfer_list_value->type != VAL_BLOB) {
+  if (transfer_list_value->type != Value::Type::BLOB) {
     ErrorAbort(state, kArgsParsingFailure, "transfer_list argument to %s must be blob", name);
     return StringValue("");
   }
-  if (new_data_fn->type != VAL_STRING) {
+  if (new_data_fn->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "new_data_fn argument to %s must be string", name);
     return StringValue("");
   }
-  if (patch_data_fn->type != VAL_STRING) {
+  if (patch_data_fn->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "patch_data_fn argument to %s must be string", name);
     return StringValue("");
   }
@@ -1944,11 +1945,11 @@ Value* RangeSha1Fn(const char* name, State* state, const std::vector<std::unique
   const std::unique_ptr<Value>& blockdev_filename = args[0];
   const std::unique_ptr<Value>& ranges = args[1];
 
-  if (blockdev_filename->type != VAL_STRING) {
+  if (blockdev_filename->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "blockdev_filename argument to %s must be string", name);
     return StringValue("");
   }
-  if (ranges->type != VAL_STRING) {
+  if (ranges->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "ranges argument to %s must be string", name);
     return StringValue("");
   }
@@ -2010,7 +2011,7 @@ Value* CheckFirstBlockFn(const char* name, State* state,
 
   const std::unique_ptr<Value>& arg_filename = args[0];
 
-  if (arg_filename->type != VAL_STRING) {
+  if (arg_filename->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "filename argument to %s must be string", name);
     return StringValue("");
   }
@@ -2065,11 +2066,11 @@ Value* BlockImageRecoverFn(const char* name, State* state,
   const std::unique_ptr<Value>& filename = args[0];
   const std::unique_ptr<Value>& ranges = args[1];
 
-  if (filename->type != VAL_STRING) {
+  if (filename->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "filename argument to %s must be string", name);
     return StringValue("");
   }
-  if (ranges->type != VAL_STRING) {
+  if (ranges->type != Value::Type::STRING) {
     ErrorAbort(state, kArgsParsingFailure, "ranges argument to %s must be string", name);
     return StringValue("");
   }
diff --git a/updater/install.cpp b/updater/install.cpp
index 9d108e0ed..02a6fe7c5 100644
--- a/updater/install.cpp
+++ b/updater/install.cpp
@@ -191,7 +191,7 @@ Value* PackageExtractFileFn(const char* name, State* state,
                         zip_path.c_str(), buffer.size(), ErrorCodeString(ret));
     }
 
-    return new Value(VAL_BLOB, buffer);
+    return new Value(Value::Type::BLOB, buffer);
   }
 }
 
@@ -238,10 +238,10 @@ Value* ApplyPatchFn(const char* name, State* state,
   }
 
   for (int i = 0; i < patchcount; ++i) {
-    if (arg_values[i * 2]->type != VAL_STRING) {
+    if (arg_values[i * 2]->type != Value::Type::STRING) {
       return ErrorAbort(state, kArgsParsingFailure, "%s(): sha-1 #%d is not string", name, i * 2);
     }
-    if (arg_values[i * 2 + 1]->type != VAL_BLOB) {
+    if (arg_values[i * 2 + 1]->type != Value::Type::BLOB) {
       return ErrorAbort(state, kArgsParsingFailure, "%s(): patch #%d is not blob", name, i * 2 + 1);
     }
   }
@@ -741,8 +741,8 @@ Value* RunProgramFn(const char* name, State* state, const std::vector<std::uniqu
   return StringValue(std::to_string(status));
 }
 
-// Read a local file and return its contents (the Value* returned
-// is actually a FileContents*).
+// read_file(filename)
+//   Reads a local file 'filename' and returns its contents as a Value string.
 Value* ReadFileFn(const char* name, State* state, const std::vector<std::unique_ptr<Expr>>& argv) {
   if (argv.size() != 1) {
     return ErrorAbort(state, kArgsParsingFailure, "%s() expects 1 arg, got %zu", name, argv.size());
@@ -750,18 +750,18 @@ Value* ReadFileFn(const char* name, State* state, const std::vector<std::unique_
 
   std::vector<std::string> args;
   if (!ReadArgs(state, argv, &args)) {
-    return ErrorAbort(state, kArgsParsingFailure, "%s() Failed to parse the argument(s)", name);
+    return ErrorAbort(state, kArgsParsingFailure, "%s(): Failed to parse the argument(s)", name);
   }
   const std::string& filename = args[0];
 
-  Value* v = new Value(VAL_INVALID, "");
-
   FileContents fc;
   if (LoadFileContents(filename.c_str(), &fc) == 0) {
-    v->type = VAL_BLOB;
-    v->data = std::string(fc.data.begin(), fc.data.end());
+    return new Value(Value::Type::BLOB, std::string(fc.data.cbegin(), fc.data.cend()));
   }
-  return v;
+
+  // Leave it to caller to handle the failure.
+  LOG(ERROR) << name << ": Failed to read " << filename;
+  return StringValue("");
 }
 
 // write_value(value, filename)
-- 
cgit v1.2.3


From d8d514fa33a2f2f9504732a19fdba81a6ccccbd7 Mon Sep 17 00:00:00 2001
From: Tao Bao <tbao@google.com>
Date: Mon, 9 Jul 2018 13:32:28 -0700
Subject: edify: Rename parse_string to ParseString and let it take
 std::string.

Also simplify the helper function expect() in {edify,updater}_test.cpp.

Test: Run recovery_component_test on marlin.
Change-Id: If54febba4b5013f6d71546318a1ca6b635204ac8
---
 edify/include/edify/expr.h       |  2 +-
 edify/parser.yy                  |  6 ++--
 tests/component/edify_test.cpp   | 75 +++++++++++++++++++---------------------
 tests/component/updater_test.cpp | 60 ++++++++++++++++----------------
 updater/updater.cpp              |  2 +-
 5 files changed, 71 insertions(+), 74 deletions(-)

diff --git a/edify/include/edify/expr.h b/edify/include/edify/expr.h
index 770d1cf0d..b922a9bd2 100644
--- a/edify/include/edify/expr.h
+++ b/edify/include/edify/expr.h
@@ -156,6 +156,6 @@ Value* StringValue(const char* str);
 
 Value* StringValue(const std::string& str);
 
-int parse_string(const char* str, std::unique_ptr<Expr>* root, int* error_count);
+int ParseString(const std::string& str, std::unique_ptr<Expr>* root, int* error_count);
 
 #endif  // _EXPRESSION_H
diff --git a/edify/parser.yy b/edify/parser.yy
index bd2e0105f..3a63c37f8 100644
--- a/edify/parser.yy
+++ b/edify/parser.yy
@@ -138,7 +138,7 @@ void yyerror(std::unique_ptr<Expr>* root, int* error_count, const char* s) {
   ++*error_count;
 }
 
-int parse_string(const char* str, std::unique_ptr<Expr>* root, int* error_count) {
-    yy_switch_to_buffer(yy_scan_string(str));
-    return yyparse(root, error_count);
+int ParseString(const std::string& str, std::unique_ptr<Expr>* root, int* error_count) {
+  yy_switch_to_buffer(yy_scan_string(str.c_str()));
+  return yyparse(root, error_count);
 }
diff --git a/tests/component/edify_test.cpp b/tests/component/edify_test.cpp
index 61a1e6b64..8397bd38e 100644
--- a/tests/component/edify_test.cpp
+++ b/tests/component/edify_test.cpp
@@ -21,30 +21,29 @@
 
 #include "edify/expr.h"
 
-static void expect(const char* expr_str, const char* expected) {
-    std::unique_ptr<Expr> e;
-    int error_count = 0;
-    EXPECT_EQ(0, parse_string(expr_str, &e, &error_count));
-    EXPECT_EQ(0, error_count);
-
-    State state(expr_str, nullptr);
-
-    std::string result;
-    bool status = Evaluate(&state, e, &result);
-
-    if (expected == nullptr) {
-        EXPECT_FALSE(status);
-    } else {
-        EXPECT_STREQ(expected, result.c_str());
-    }
-
+static void expect(const std::string& expr_str, const char* expected) {
+  std::unique_ptr<Expr> e;
+  int error_count = 0;
+  EXPECT_EQ(0, ParseString(expr_str, &e, &error_count));
+  EXPECT_EQ(0, error_count);
+
+  State state(expr_str, nullptr);
+
+  std::string result;
+  bool status = Evaluate(&state, e, &result);
+
+  if (expected == nullptr) {
+    EXPECT_FALSE(status);
+  } else {
+    EXPECT_STREQ(expected, result.c_str());
+  }
 }
 
 class EdifyTest : public ::testing::Test {
-  protected:
-    virtual void SetUp() {
-        RegisterBuiltins();
-    }
+ protected:
+  void SetUp() {
+    RegisterBuiltins();
+  }
 };
 
 TEST_F(EdifyTest, parsing) {
@@ -146,25 +145,23 @@ TEST_F(EdifyTest, comparison) {
 }
 
 TEST_F(EdifyTest, big_string) {
-    // big string
-    expect(std::string(8192, 's').c_str(), std::string(8192, 's').c_str());
+  expect(std::string(8192, 's'), std::string(8192, 's').c_str());
 }
 
 TEST_F(EdifyTest, unknown_function) {
-    // unknown function
-    const char* script1 = "unknown_function()";
-    std::unique_ptr<Expr> expr;
-    int error_count = 0;
-    EXPECT_EQ(1, parse_string(script1, &expr, &error_count));
-    EXPECT_EQ(1, error_count);
-
-    const char* script2 = "abc; unknown_function()";
-    error_count = 0;
-    EXPECT_EQ(1, parse_string(script2, &expr, &error_count));
-    EXPECT_EQ(1, error_count);
-
-    const char* script3 = "unknown_function1() || yes";
-    error_count = 0;
-    EXPECT_EQ(1, parse_string(script3, &expr, &error_count));
-    EXPECT_EQ(1, error_count);
+  const char* script1 = "unknown_function()";
+  std::unique_ptr<Expr> expr;
+  int error_count = 0;
+  EXPECT_EQ(1, ParseString(script1, &expr, &error_count));
+  EXPECT_EQ(1, error_count);
+
+  const char* script2 = "abc; unknown_function()";
+  error_count = 0;
+  EXPECT_EQ(1, ParseString(script2, &expr, &error_count));
+  EXPECT_EQ(1, error_count);
+
+  const char* script3 = "unknown_function1() || yes";
+  error_count = 0;
+  EXPECT_EQ(1, ParseString(script3, &expr, &error_count));
+  EXPECT_EQ(1, error_count);
 }
diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp
index f50e861b0..81ea3b61b 100644
--- a/tests/component/updater_test.cpp
+++ b/tests/component/updater_test.cpp
@@ -57,11 +57,11 @@ static constexpr size_t kTransferListHeaderLines = 4;
 
 struct selabel_handle* sehandle = nullptr;
 
-static void expect(const char* expected, const char* expr_str, CauseCode cause_code,
+static void expect(const char* expected, const std::string& expr_str, CauseCode cause_code,
                    UpdaterInfo* info = nullptr) {
   std::unique_ptr<Expr> e;
   int error_count = 0;
-  ASSERT_EQ(0, parse_string(expr_str, &e, &error_count));
+  ASSERT_EQ(0, ParseString(expr_str, &e, &error_count));
   ASSERT_EQ(0, error_count);
 
   State state(expr_str, info);
@@ -126,7 +126,7 @@ static void RunBlockImageUpdate(bool is_verify, const PackageEntries& entries,
   std::string script = is_verify ? "block_image_verify" : "block_image_update";
   script += R"((")" + image_file + R"(", package_extract_file("transfer_list"), ")" + new_data +
             R"(", "patch_data"))";
-  expect(result.c_str(), script.c_str(), cause_code, &updater_info);
+  expect(result.c_str(), script, cause_code, &updater_info);
 
   ASSERT_EQ(0, fclose(updater_info.cmd_pipe));
   CloseArchive(handle);
@@ -230,7 +230,7 @@ TEST_F(UpdaterTest, apply_patch_check) {
   std::string filename = android::base::Join(
       std::vector<std::string>{ "EMMC", src_file, std::to_string(src_size), src_hash }, ":");
   std::string cmd = "apply_patch_check(\"" + filename + "\")";
-  expect("t", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 
   // EMMC:file:(size-1):sha1:(size+1):sha1 should fail the check.
   std::string filename_bad = android::base::Join(
@@ -238,7 +238,7 @@ TEST_F(UpdaterTest, apply_patch_check) {
                                 std::to_string(src_size + 1), src_hash },
       ":");
   cmd = "apply_patch_check(\"" + filename_bad + "\")";
-  expect("", cmd.c_str(), kNoCause);
+  expect("", cmd, kNoCause);
 
   // EMMC:file:(size-1):sha1:size:sha1:(size+1):sha1 should pass the check.
   filename_bad =
@@ -247,21 +247,21 @@ TEST_F(UpdaterTest, apply_patch_check) {
                                                     std::to_string(src_size + 1), src_hash },
                           ":");
   cmd = "apply_patch_check(\"" + filename_bad + "\")";
-  expect("t", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 
   // Multiple arguments.
   // As long as it successfully loads the partition specified in filename, it won't check against
   // any given SHAs.
   cmd = "apply_patch_check(\"" + filename + "\", \"wrong_sha1\", \"wrong_sha2\")";
-  expect("t", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 
   cmd = "apply_patch_check(\"" + filename + "\", \"wrong_sha1\", \"" + src_hash +
         "\", \"wrong_sha2\")";
-  expect("t", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 
   cmd = "apply_patch_check(\"" + filename_bad + "\", \"wrong_sha1\", \"" + src_hash +
         "\", \"wrong_sha2\")";
-  expect("t", cmd.c_str(), kNoCause);
+  expect("t", cmd, kNoCause);
 }
 
 TEST_F(UpdaterTest, file_getprop) {
@@ -288,28 +288,28 @@ TEST_F(UpdaterTest, file_getprop) {
 
     std::string script1("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.name\")");
-    expect("tardis", script1.c_str(), kNoCause);
+    expect("tardis", script1, kNoCause);
 
     std::string script2("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.board\")");
-    expect("magic", script2.c_str(), kNoCause);
+    expect("magic", script2, kNoCause);
 
     // No match.
     std::string script3("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.wrong\")");
-    expect("", script3.c_str(), kNoCause);
+    expect("", script3, kNoCause);
 
     std::string script4("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.name=\")");
-    expect("", script4.c_str(), kNoCause);
+    expect("", script4, kNoCause);
 
     std::string script5("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.nam\")");
-    expect("", script5.c_str(), kNoCause);
+    expect("", script5, kNoCause);
 
     std::string script6("file_getprop(\"" + std::string(temp_file2.path) +
                        "\", \"ro.product.model\")");
-    expect("", script6.c_str(), kNoCause);
+    expect("", script6, kNoCause);
 }
 
 // TODO: Test extracting to block device.
@@ -329,7 +329,7 @@ TEST_F(UpdaterTest, package_extract_file) {
   // Two-argument version.
   TemporaryFile temp_file1;
   std::string script("package_extract_file(\"a.txt\", \"" + std::string(temp_file1.path) + "\")");
-  expect("t", script.c_str(), kNoCause, &updater_info);
+  expect("t", script, kNoCause, &updater_info);
 
   // Verify the extracted entry.
   std::string data;
@@ -338,30 +338,30 @@ TEST_F(UpdaterTest, package_extract_file) {
 
   // Now extract another entry to the same location, which should overwrite.
   script = "package_extract_file(\"b.txt\", \"" + std::string(temp_file1.path) + "\")";
-  expect("t", script.c_str(), kNoCause, &updater_info);
+  expect("t", script, kNoCause, &updater_info);
 
   ASSERT_TRUE(android::base::ReadFileToString(temp_file1.path, &data));
   ASSERT_EQ(kBTxtContents, data);
 
   // Missing zip entry. The two-argument version doesn't abort.
   script = "package_extract_file(\"doesntexist\", \"" + std::string(temp_file1.path) + "\")";
-  expect("", script.c_str(), kNoCause, &updater_info);
+  expect("", script, kNoCause, &updater_info);
 
   // Extract to /dev/full should fail.
   script = "package_extract_file(\"a.txt\", \"/dev/full\")";
-  expect("", script.c_str(), kNoCause, &updater_info);
+  expect("", script, kNoCause, &updater_info);
 
   // One-argument version. package_extract_file() gives a VAL_BLOB, which needs to be converted to
   // VAL_STRING for equality test.
   script = "blob_to_string(package_extract_file(\"a.txt\")) == \"" + kATxtContents + "\"";
-  expect("t", script.c_str(), kNoCause, &updater_info);
+  expect("t", script, kNoCause, &updater_info);
 
   script = "blob_to_string(package_extract_file(\"b.txt\")) == \"" + kBTxtContents + "\"";
-  expect("t", script.c_str(), kNoCause, &updater_info);
+  expect("t", script, kNoCause, &updater_info);
 
   // Missing entry. The one-argument version aborts the evaluation.
   script = "package_extract_file(\"doesntexist\")";
-  expect(nullptr, script.c_str(), kPackageExtractFileFailure, &updater_info);
+  expect(nullptr, script, kPackageExtractFileFailure, &updater_info);
 
   CloseArchive(handle);
 }
@@ -379,7 +379,7 @@ TEST_F(UpdaterTest, write_value) {
   TemporaryFile temp_file;
   std::string value = "magicvalue";
   std::string script("write_value(\"" + value + "\", \"" + std::string(temp_file.path) + "\")");
-  expect("t", script.c_str(), kNoCause);
+  expect("t", script, kNoCause);
 
   // Verify the content.
   std::string content;
@@ -388,7 +388,7 @@ TEST_F(UpdaterTest, write_value) {
 
   // Allow writing empty string.
   script = "write_value(\"\", \"" + std::string(temp_file.path) + "\")";
-  expect("t", script.c_str(), kNoCause);
+  expect("t", script, kNoCause);
 
   // Verify the content.
   ASSERT_TRUE(android::base::ReadFileToString(temp_file.path, &content));
@@ -396,7 +396,7 @@ TEST_F(UpdaterTest, write_value) {
 
   // It should fail gracefully when write fails.
   script = "write_value(\"value\", \"/proc/0/file1\")";
-  expect("", script.c_str(), kNoCause);
+  expect("", script, kNoCause);
 }
 
 TEST_F(UpdaterTest, get_stage) {
@@ -415,11 +415,11 @@ TEST_F(UpdaterTest, get_stage) {
 
   // Can read the stage value.
   std::string script("get_stage(\"" + temp_file + "\")");
-  expect("2/3", script.c_str(), kNoCause);
+  expect("2/3", script, kNoCause);
 
   // Bad BCB path.
   script = "get_stage(\"doesntexist\")";
-  expect("", script.c_str(), kNoCause);
+  expect("", script, kNoCause);
 }
 
 TEST_F(UpdaterTest, set_stage) {
@@ -439,7 +439,7 @@ TEST_F(UpdaterTest, set_stage) {
 
   // Write with set_stage().
   std::string script("set_stage(\"" + temp_file + "\", \"1/3\")");
-  expect(tf.path, script.c_str(), kNoCause);
+  expect(tf.path, script, kNoCause);
 
   // Verify.
   bootloader_message boot_verify;
@@ -451,10 +451,10 @@ TEST_F(UpdaterTest, set_stage) {
 
   // Bad BCB path.
   script = "set_stage(\"doesntexist\", \"1/3\")";
-  expect("", script.c_str(), kNoCause);
+  expect("", script, kNoCause);
 
   script = "set_stage(\"/dev/full\", \"1/3\")";
-  expect("", script.c_str(), kNoCause);
+  expect("", script, kNoCause);
 }
 
 TEST_F(UpdaterTest, set_progress) {
diff --git a/updater/updater.cpp b/updater/updater.cpp
index 40e3f1fc1..e06d45355 100644
--- a/updater/updater.cpp
+++ b/updater/updater.cpp
@@ -134,7 +134,7 @@ int main(int argc, char** argv) {
 
   std::unique_ptr<Expr> root;
   int error_count = 0;
-  int error = parse_string(script.c_str(), &root, &error_count);
+  int error = ParseString(script, &root, &error_count);
   if (error != 0 || error_count > 0) {
     LOG(ERROR) << error_count << " parse errors";
     CloseArchive(za);
-- 
cgit v1.2.3


From bafd6c7afb134b78673b4d8ff680082cb3d5a805 Mon Sep 17 00:00:00 2001
From: Tao Bao <tbao@google.com>
Date: Mon, 9 Jul 2018 15:08:50 -0700
Subject: updater: Let read_file() return Value::Type::STRING.

It used to return a Value blob to be consumed by sha1_check() (which has
been deprecated). Currently there's no other generic updater function
that works with BLOB Values. This CL changes read_file() to return a
string Value to make it more useful (e.g. allowing equality check).

Test: Run recovery_component_test and recovery_unit_test on marlin.
Change-Id: Iba986ba649030112babefe898f26aa9ffe69eeb7
---
 tests/component/updater_test.cpp | 23 +++++++++++++++++++++++
 updater/install.cpp              | 10 +++++-----
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp
index 91e5cc1aa..9fcf17f13 100644
--- a/tests/component/updater_test.cpp
+++ b/tests/component/updater_test.cpp
@@ -51,6 +51,8 @@
 #include "updater/install.h"
 #include "updater/updater.h"
 
+using namespace std::string_literals;
+
 using PackageEntries = std::unordered_map<std::string, std::string>;
 
 static constexpr size_t kTransferListHeaderLines = 4;
@@ -366,6 +368,27 @@ TEST_F(UpdaterTest, package_extract_file) {
   CloseArchive(handle);
 }
 
+TEST_F(UpdaterTest, read_file) {
+  // read_file() expects one argument.
+  expect(nullptr, "read_file()", kArgsParsingFailure);
+  expect(nullptr, "read_file(\"arg1\", \"arg2\")", kArgsParsingFailure);
+
+  // Write some value to file and read back.
+  TemporaryFile temp_file;
+  std::string script("write_value(\"foo\", \""s + temp_file.path + "\");");
+  expect("t", script, kNoCause);
+
+  script = "read_file(\""s + temp_file.path + "\") == \"foo\"";
+  expect("t", script, kNoCause);
+
+  script = "read_file(\""s + temp_file.path + "\") == \"bar\"";
+  expect("", script, kNoCause);
+
+  // It should fail gracefully when read fails.
+  script = "read_file(\"/doesntexist\")";
+  expect("", script, kNoCause);
+}
+
 TEST_F(UpdaterTest, write_value) {
   // write_value() expects two arguments.
   expect(nullptr, "write_value()", kArgsParsingFailure);
diff --git a/updater/install.cpp b/updater/install.cpp
index 02a6fe7c5..d0be955a7 100644
--- a/updater/install.cpp
+++ b/updater/install.cpp
@@ -742,7 +742,7 @@ Value* RunProgramFn(const char* name, State* state, const std::vector<std::uniqu
 }
 
 // read_file(filename)
-//   Reads a local file 'filename' and returns its contents as a Value string.
+//   Reads a local file 'filename' and returns its contents as a string Value.
 Value* ReadFileFn(const char* name, State* state, const std::vector<std::unique_ptr<Expr>>& argv) {
   if (argv.size() != 1) {
     return ErrorAbort(state, kArgsParsingFailure, "%s() expects 1 arg, got %zu", name, argv.size());
@@ -754,13 +754,13 @@ Value* ReadFileFn(const char* name, State* state, const std::vector<std::unique_
   }
   const std::string& filename = args[0];
 
-  FileContents fc;
-  if (LoadFileContents(filename.c_str(), &fc) == 0) {
-    return new Value(Value::Type::BLOB, std::string(fc.data.cbegin(), fc.data.cend()));
+  std::string contents;
+  if (android::base::ReadFileToString(filename, &contents)) {
+    return new Value(Value::Type::STRING, std::move(contents));
   }
 
   // Leave it to caller to handle the failure.
-  LOG(ERROR) << name << ": Failed to read " << filename;
+  PLOG(ERROR) << name << ": Failed to read " << filename;
   return StringValue("");
 }
 
-- 
cgit v1.2.3