5 files changed, 162 insertions, 152 deletions

diff --git a/applypatch/applypatch.cpp b/applypatch/applypatch.cpp
index 48e4c8386..9d505f9f6 100644
--- a/applypatch/applypatch.cpp
+++ b/applypatch/applypatch.cpp
@@ -530,8 +530,8 @@ int applypatch(const char* source_filename,
                const char* target_sha1_str,
                size_t target_size,
                const std::vector<std::string>& patch_sha1_str,
-               Value** patch_data,
-               Value* bonus_data) {
+               const std::vector<std::unique_ptr<Value>>& patch_data,
+               const Value* bonus_data) {
     printf("patch %s: ", source_filename);
 
     if (target_filename[0] == '-' && target_filename[1] == '\0') {
@@ -546,8 +546,8 @@ int applypatch(const char* source_filename,
 
     FileContents copy_file;
     FileContents source_file;
-    const Value* source_patch_value = NULL;
-    const Value* copy_patch_value = NULL;
+    const Value* source_patch_value = nullptr;
+    const Value* copy_patch_value = nullptr;
 
     // We try to load the target file into the source_file object.
     if (LoadFileContents(target_filename, &source_file) == 0) {
@@ -571,11 +571,11 @@ int applypatch(const char* source_filename,
     if (!source_file.data.empty()) {
         int to_use = FindMatchingPatch(source_file.sha1, patch_sha1_str);
         if (to_use >= 0) {
-            source_patch_value = patch_data[to_use];
+            source_patch_value = patch_data[to_use].get();
         }
     }
 
-    if (source_patch_value == NULL) {
+    if (source_patch_value == nullptr) {
         source_file.data.clear();
         printf("source file is bad; trying copy\n");
 
@@ -587,10 +587,10 @@ int applypatch(const char* source_filename,
 
         int to_use = FindMatchingPatch(copy_file.sha1, patch_sha1_str);
         if (to_use >= 0) {
-            copy_patch_value = patch_data[to_use];
+            copy_patch_value = patch_data[to_use].get();
         }
 
-        if (copy_patch_value == NULL) {
+        if (copy_patch_value == nullptr) {
             // fail.
             printf("copy file doesn't match source SHA-1s either\n");
             return 1;
diff --git a/applypatch/include/applypatch/applypatch.h b/applypatch/include/applypatch/applypatch.h
index 80d681638..ca3dafbc9 100644
--- a/applypatch/include/applypatch/applypatch.h
+++ b/applypatch/include/applypatch/applypatch.h
@@ -20,10 +20,12 @@
 #include <stdint.h>
 #include <sys/stat.h>
 
+#include <memory>
 #include <string>
 #include <vector>
 
-#include "openssl/sha.h"
+#include <openssl/sha.h>
+
 #include "edify/expr.h"
 
 struct FileContents {
@@ -41,27 +43,26 @@ struct FileContents {
 
 typedef ssize_t (*SinkFn)(const unsigned char*, ssize_t, void*);
 
-// applypatch.c
+// applypatch.cpp
 int ShowLicenses();
 size_t FreeSpaceForFile(const char* filename);
 int CacheSizeCheck(size_t bytes);
 int ParseSha1(const char* str, uint8_t* digest);
 
-int applypatch_flash(const char* source_filename, const char* target_filename,
-                     const char* target_sha1_str, size_t target_size);
 int applypatch(const char* source_filename,
                const char* target_filename,
                const char* target_sha1_str,
                size_t target_size,
                const std::vector<std::string>& patch_sha1_str,
-               Value** patch_data,
-               Value* bonus_data);
+               const std::vector<std::unique_ptr<Value>>& patch_data,
+               const Value* bonus_data);
 int applypatch_check(const char* filename,
                      const std::vector<std::string>& patch_sha1_str);
+int applypatch_flash(const char* source_filename, const char* target_filename,
+                     const char* target_sha1_str, size_t target_size);
 
 int LoadFileContents(const char* filename, FileContents* file);
 int SaveFileContents(const char* filename, const FileContents* file);
-void FreeFileContents(FileContents* file);
 int FindMatchingPatch(uint8_t* sha1, const std::vector<std::string>& patch_sha1_str);
 
 // bsdiff.cpp
diff --git a/applypatch/main.cpp b/applypatch/main.cpp
index 294f7ee21..988b6f9b1 100644
--- a/applypatch/main.cpp
+++ b/applypatch/main.cpp
@@ -23,9 +23,10 @@
 #include <string>
 #include <vector>
 
+#include <openssl/sha.h>
+
 #include "applypatch/applypatch.h"
 #include "edify/expr.h"
-#include "openssl/sha.h"
 
 static int CheckMode(int argc, char** argv) {
     if (argc < 3) {
@@ -129,15 +130,13 @@ static int PatchMode(int argc, char** argv) {
         printf("failed to parse patch args\n");
         return 1;
     }
-    std::vector<Value> patches;
-    std::vector<Value*> patch_ptrs;
+
+    std::vector<std::unique_ptr<Value>> patches;
     for (size_t i = 0; i < files.size(); ++i) {
-        patches.push_back(Value(VAL_BLOB,
-                                std::string(files[i].data.cbegin(), files[i].data.cend())));
-        patch_ptrs.push_back(&patches[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, patch_ptrs.data(), &bonus);
+    return applypatch(argv[1], argv[2], argv[3], target_size, sha1s, patches, &bonus);
 }
 
 // This program applies binary patches to files in a way that is safe
diff --git a/tests/component/applypatch_test.cpp b/tests/component/applypatch_test.cpp
index 908a9f5f5..e09c1e7ba 100644
--- a/tests/component/applypatch_test.cpp
+++ b/tests/component/applypatch_test.cpp
@@ -23,102 +23,98 @@
 #include <sys/types.h>
 #include <time.h>
 
+#include <memory>
 #include <string>
+#include <vector>
 
 #include <android-base/file.h>
 #include <android-base/stringprintf.h>
 #include <android-base/test_utils.h>
+#include <openssl/sha.h>
 
 #include "applypatch/applypatch.h"
 #include "common/test_constants.h"
-#include "openssl/sha.h"
 #include "print_sha1.h"
 
 static const std::string DATA_PATH = getenv("ANDROID_DATA");
 static const std::string TESTDATA_PATH = "/recovery/testdata";
-static const std::string WORK_FS = "/data";
 
-static std::string sha1sum(const std::string& fname) {
-    uint8_t digest[SHA_DIGEST_LENGTH];
+static void sha1sum(const std::string& fname, std::string* sha1) {
+    ASSERT_NE(nullptr, sha1);
+
     std::string data;
-    android::base::ReadFileToString(fname, &data);
+    ASSERT_TRUE(android::base::ReadFileToString(fname, &data));
 
-    SHA1((const uint8_t*)data.c_str(), data.size(), digest);
-    return print_sha1(digest);
+    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) {
-    FILE* fh = fopen(&fname[0], "w");
-    int r;
-    for (int i=0; i < 1024; i++) {
-        r = rand();
-        fwrite(&r, sizeof(short), 1, fh);
+    std::string content;
+    content.reserve(1024);
+    for (size_t i = 0; i < 1024; i++) {
+        content[i] = rand() % 256;
     }
-    fclose(fh);
+    ASSERT_TRUE(android::base::WriteStringToFile(content, fname));
 }
 
-static bool file_cmp(std::string& f1, std::string& f2) {
+static bool file_cmp(const std::string& f1, const std::string& f2) {
     std::string c1;
-    std::string c2;
     android::base::ReadFileToString(f1, &c1);
+    std::string c2;
     android::base::ReadFileToString(f2, &c2);
     return c1 == c2;
 }
 
 static std::string from_testdata_base(const std::string& fname) {
-    return android::base::StringPrintf("%s%s%s/%s",
-            &DATA_PATH[0],
-            &NATIVE_TEST_PATH[0],
-            &TESTDATA_PATH[0],
-            &fname[0]);
+    return DATA_PATH + NATIVE_TEST_PATH + TESTDATA_PATH + "/" + fname;
 }
 
 class ApplyPatchTest : public ::testing::Test {
-    public:
-        static void SetUpTestCase() {
-            // set up files
-            old_file = from_testdata_base("old.file");
-            new_file = from_testdata_base("new.file");
-            patch_file = from_testdata_base("patch.bsdiff");
-            rand_file = "/cache/applypatch_test_rand.file";
-            cache_file = "/cache/saved.file";
-
-            // write stuff to rand_file
-            android::base::WriteStringToFile("hello", rand_file);
-
-            // set up SHA constants
-            old_sha1 = sha1sum(old_file);
-            new_sha1 = sha1sum(new_file);
-            srand(time(NULL));
-            bad_sha1_a = android::base::StringPrintf("%040x", rand());
-            bad_sha1_b = android::base::StringPrintf("%040x", rand());
-
-            struct stat st;
-            stat(&new_file[0], &st);
-            new_size = st.st_size;
-        }
-
-        static std::string old_file;
-        static std::string new_file;
-        static std::string rand_file;
-        static std::string cache_file;
-        static std::string patch_file;
-
-        static std::string old_sha1;
-        static std::string new_sha1;
-        static std::string bad_sha1_a;
-        static std::string bad_sha1_b;
-
-        static size_t new_size;
+  public:
+    static void SetUpTestCase() {
+        // set up files
+        old_file = from_testdata_base("old.file");
+        new_file = from_testdata_base("new.file");
+        patch_file = from_testdata_base("patch.bsdiff");
+        rand_file = "/cache/applypatch_test_rand.file";
+        cache_file = "/cache/saved.file";
+
+        // write stuff to rand_file
+        ASSERT_TRUE(android::base::WriteStringToFile("hello", rand_file));
+
+        // set up SHA constants
+        sha1sum(old_file, &old_sha1);
+        sha1sum(new_file, &new_sha1);
+        srand(time(NULL));
+        bad_sha1_a = android::base::StringPrintf("%040x", rand());
+        bad_sha1_b = android::base::StringPrintf("%040x", rand());
+
+        struct stat st;
+        stat(&new_file[0], &st);
+        new_size = st.st_size;
+    }
+
+    static std::string old_file;
+    static std::string new_file;
+    static std::string rand_file;
+    static std::string cache_file;
+    static std::string patch_file;
+
+    static std::string old_sha1;
+    static std::string new_sha1;
+    static std::string bad_sha1_a;
+    static std::string bad_sha1_b;
+
+    static size_t new_size;
 };
 
 std::string ApplyPatchTest::old_file;
 std::string ApplyPatchTest::new_file;
 
-static void cp(std::string src, std::string tgt) {
-    std::string cmd = android::base::StringPrintf("cp %s %s",
-            &src[0],
-            &tgt[0]);
+static void cp(const std::string& src, const std::string& tgt) {
+    std::string cmd = "cp " + src + " " + tgt;
     system(&cmd[0]);
 }
 
@@ -131,59 +127,55 @@ static void restore_old() {
 }
 
 class ApplyPatchCacheTest : public ApplyPatchTest {
-    public:
-        virtual void SetUp() {
-            backup_old();
-        }
-
-        virtual void TearDown() {
-            restore_old();
-        }
+  public:
+    virtual void SetUp() {
+        backup_old();
+    }
+
+    virtual void TearDown() {
+        restore_old();
+    }
 };
 
 class ApplyPatchFullTest : public ApplyPatchCacheTest {
-    public:
-        static void SetUpTestCase() {
-            ApplyPatchTest::SetUpTestCase();
-            unsigned long free_kb = FreeSpaceForFile(&WORK_FS[0]);
-            ASSERT_GE(free_kb * 1024, new_size * 3 / 2);
-            output_f = new TemporaryFile();
-            output_loc = std::string(output_f->path);
-
-            struct FileContents fc;
-
-            ASSERT_EQ(0, LoadFileContents(&rand_file[0], &fc));
-            Value* patch1 = new Value(VAL_BLOB, std::string(fc.data.begin(), fc.data.end()));
-            patches.push_back(patch1);
-
-            ASSERT_EQ(0, LoadFileContents(&patch_file[0], &fc));
-            Value* patch2 = new Value(VAL_BLOB, std::string(fc.data.begin(), fc.data.end()));
-            patches.push_back(patch2);
-        }
-        static void TearDownTestCase() {
-            delete output_f;
-            for (auto it = patches.begin(); it != patches.end(); ++it) {
-                delete *it;
-            }
-            patches.clear();
-        }
-
-        static std::vector<Value*> patches;
-        static TemporaryFile* output_f;
-        static std::string output_loc;
+  public:
+    static void SetUpTestCase() {
+        ApplyPatchTest::SetUpTestCase();
+
+        output_f = new TemporaryFile();
+        output_loc = std::string(output_f->path);
+
+        struct FileContents fc;
+
+        ASSERT_EQ(0, LoadFileContents(&rand_file[0], &fc));
+        patches.push_back(
+            std::make_unique<Value>(VAL_BLOB, std::string(fc.data.begin(), fc.data.end())));
+
+        ASSERT_EQ(0, LoadFileContents(&patch_file[0], &fc));
+        patches.push_back(
+            std::make_unique<Value>(VAL_BLOB, std::string(fc.data.begin(), fc.data.end())));
+    }
+
+    static void TearDownTestCase() {
+        delete output_f;
+    }
+
+    static std::vector<std::unique_ptr<Value>> patches;
+    static TemporaryFile* output_f;
+    static std::string output_loc;
 };
 
 class ApplyPatchDoubleCacheTest : public ApplyPatchFullTest {
-    public:
-        virtual void SetUp() {
-            ApplyPatchCacheTest::SetUp();
-            cp(cache_file, "/cache/reallysaved.file");
-        }
-
-        virtual void TearDown() {
-            cp("/cache/reallysaved.file", cache_file);
-            ApplyPatchCacheTest::TearDown();
-        }
+  public:
+    virtual void SetUp() {
+        ApplyPatchCacheTest::SetUp();
+        cp(cache_file, "/cache/reallysaved.file");
+    }
+
+    virtual void TearDown() {
+        cp("/cache/reallysaved.file", cache_file);
+        ApplyPatchCacheTest::TearDown();
+    }
 };
 
 std::string ApplyPatchTest::rand_file;
@@ -196,7 +188,7 @@ std::string ApplyPatchTest::bad_sha1_b;
 
 size_t ApplyPatchTest::new_size;
 
-std::vector<Value*> ApplyPatchFullTest::patches;
+std::vector<std::unique_ptr<Value>> ApplyPatchFullTest::patches;
 TemporaryFile* ApplyPatchFullTest::output_f;
 std::string ApplyPatchFullTest::output_loc;
 
@@ -287,7 +279,7 @@ TEST_F(ApplyPatchFullTest, ApplyInPlace) {
             &new_sha1[0],
             new_size,
             sha1s,
-            patches.data(),
+            patches,
             nullptr);
     ASSERT_EQ(0, ap_result);
     ASSERT_TRUE(file_cmp(old_file, new_file));
@@ -297,7 +289,7 @@ TEST_F(ApplyPatchFullTest, ApplyInPlace) {
             &new_sha1[0],
             new_size,
             sha1s,
-            patches.data(),
+            patches,
             nullptr);
     ASSERT_EQ(0, ap_result);
     ASSERT_TRUE(file_cmp(old_file, new_file));
@@ -308,75 +300,95 @@ TEST_F(ApplyPatchFullTest, ApplyInNewLocation) {
         bad_sha1_a,
         old_sha1
     };
-    int ap_result = applypatch(&old_file[0],
+    // Apply bsdiff patch to new location.
+    ASSERT_EQ(0, applypatch(&old_file[0],
             &output_loc[0],
             &new_sha1[0],
             new_size,
             sha1s,
-            patches.data(),
-            nullptr);
-    ASSERT_EQ(0, ap_result);
+            patches,
+            nullptr));
     ASSERT_TRUE(file_cmp(output_loc, new_file));
-    ap_result = applypatch(&old_file[0],
+
+    // Reapply to the same location.
+    ASSERT_EQ(0, applypatch(&old_file[0],
             &output_loc[0],
             &new_sha1[0],
             new_size,
             sha1s,
-            patches.data(),
-            nullptr);
-    ASSERT_EQ(0, ap_result);
+            patches,
+            nullptr));
     ASSERT_TRUE(file_cmp(output_loc, new_file));
 }
 
 TEST_F(ApplyPatchFullTest, ApplyCorruptedInNewLocation) {
-    mangle_file(old_file);
     std::vector<std::string> sha1s = {
         bad_sha1_a,
         old_sha1
     };
+    // Apply bsdiff patch to new location with corrupted source.
+    mangle_file(old_file);
     int ap_result = applypatch(&old_file[0],
             &output_loc[0],
             &new_sha1[0],
             new_size,
             sha1s,
-            patches.data(),
+            patches,
             nullptr);
     ASSERT_EQ(0, ap_result);
     ASSERT_TRUE(file_cmp(output_loc, new_file));
+
+    // Reapply bsdiff patch to new location with corrupted source.
     ap_result = applypatch(&old_file[0],
             &output_loc[0],
             &new_sha1[0],
             new_size,
             sha1s,
-            patches.data(),
+            patches,
             nullptr);
     ASSERT_EQ(0, ap_result);
     ASSERT_TRUE(file_cmp(output_loc, new_file));
 }
 
 TEST_F(ApplyPatchDoubleCacheTest, ApplyDoubleCorruptedInNewLocation) {
-    mangle_file(old_file);
-    mangle_file(cache_file);
-
     std::vector<std::string> sha1s = {
         bad_sha1_a,
         old_sha1
     };
+
+    // Apply bsdiff patch to new location with corrupted source and copy (no new file).
+    // Expected to fail.
+    mangle_file(old_file);
+    mangle_file(cache_file);
     int ap_result = applypatch(&old_file[0],
             &output_loc[0],
             &new_sha1[0],
             new_size,
             sha1s,
-            patches.data(),
+            patches,
+            nullptr);
+    ASSERT_NE(0, ap_result);
+    ASSERT_FALSE(file_cmp(output_loc, new_file));
+
+    // Expected to fail again on retry.
+    ap_result = applypatch(&old_file[0],
+            &output_loc[0],
+            &new_sha1[0],
+            new_size,
+            sha1s,
+            patches,
             nullptr);
     ASSERT_NE(0, ap_result);
     ASSERT_FALSE(file_cmp(output_loc, new_file));
+
+    // Expected to fail with incorrect new file.
+    mangle_file(output_loc);
     ap_result = applypatch(&old_file[0],
             &output_loc[0],
             &new_sha1[0],
             new_size,
             sha1s,
-            patches.data(),
+            patches,
             nullptr);
     ASSERT_NE(0, ap_result);
     ASSERT_FALSE(file_cmp(output_loc, new_file));
diff --git a/updater/install.cpp b/updater/install.cpp
index a41c5db3c..5f3f675b1 100644
--- a/updater/install.cpp
+++ b/updater/install.cpp
@@ -1061,15 +1061,13 @@ Value* ApplyPatchFn(const char* name, State* state, int argc, Expr* argv[]) {
     }
 
     std::vector<std::string> patch_sha_str;
-    std::vector<Value*> patch_ptrs;
     for (int i = 0; i < patchcount; ++i) {
         patch_sha_str.push_back(patch_shas[i]->data);
-        patch_ptrs.push_back(patches[i].get());
     }
 
     int result = applypatch(source_filename, target_filename,
                             target_sha1, target_size,
-                            patch_sha_str, patch_ptrs.data(), NULL);
+                            patch_sha_str, patches, NULL);
 
     return StringValue(result == 0 ? "t" : "");
 }