Snap for 4829593 from f0e3a368c80b7d32e2f85ffa948751525a9f903b to pi-releaseandroid-wear-9.0.0_r9android-wear-9.0.0_r8android-wear-9.0.0_r7android-wear-9.0.0_r6android-wear-9.0.0_r5android-wear-9.0.0_r4android-wear-9.0.0_r34android-wear-9.0.0_r33android-wear-9.0.0_r32android-wear-9.0.0_r30android-wear-9.0.0_r3android-wear-9.0.0_r29android-wear-9.0.0_r28android-wear-9.0.0_r27android-wear-9.0.0_r26android-wear-9.0.0_r25android-wear-9.0.0_r24android-wear-9.0.0_r23android-wear-9.0.0_r22android-wear-9.0.0_r21android-wear-9.0.0_r20android-wear-9.0.0_r2android-wear-9.0.0_r19android-wear-9.0.0_r18android-wear-9.0.0_r17android-wear-9.0.0_r16android-wear-9.0.0_r15android-wear-9.0.0_r14android-wear-9.0.0_r13android-wear-9.0.0_r12android-wear-9.0.0_r11android-wear-9.0.0_r10android-wear-9.0.0_r1android-cts-9.0_r1android-9.0.0_r9android-9.0.0_r8android-9.0.0_r7android-9.0.0_r6android-9.0.0_r5android-9.0.0_r3android-9.0.0_r2android-9.0.0_r18android-9.0.0_r17android-9.0.0_r10android-9.0.0_r1pie-s2-releasepie-release-2pie-releasepie-r2-s2-releasepie-r2-s1-releasepie-r2-release

Change-Id: Ief80ee009c18b0b26d378e7b0658d6acec88ea7b

2 files changed, 156 insertions, 82 deletions

diff --git a/patchoat/patchoat.cc b/patchoat/patchoat.cc
index b9a9a69ab5..c742c1656f 100644
--- a/patchoat/patchoat.cc
+++ b/patchoat/patchoat.cc
@@ -362,6 +362,10 @@ static bool CheckImageIdenticalToOriginalExceptForRelocation(
     uint32_t offset_delta = 0;
     if (DecodeUnsignedLeb128Checked(&rel_ptr, rel_end, &offset_delta)) {
       offset += offset_delta;
+      if (static_cast<int64_t>(offset) + static_cast<int64_t>(sizeof(uint32_t)) > image_size) {
+        *error_msg = StringPrintf("Relocation out of bounds in %s", relocated_filename.c_str());
+        return false;
+      }
       uint32_t *image_value = reinterpret_cast<uint32_t*>(image_start + offset);
       *image_value -= expected_diff;
     } else {
diff --git a/patchoat/patchoat_test.cc b/patchoat/patchoat_test.cc
index 974ed3217d..4cfb0db2e5 100644
--- a/patchoat/patchoat_test.cc
+++ b/patchoat/patchoat_test.cc
@@ -438,103 +438,173 @@ TEST_F(PatchoatTest, PatchoatRelocationSameAsDex2oatRelocation) {
 #endif
 }
 
-TEST_F(PatchoatTest, RelFileVerification) {
-  // This test checks that a boot image relocated using patchoat can be unrelocated using the .rel
-  // file created by patchoat.
+// These tests check that a boot image relocated using patchoat can be unrelocated
+// using the .rel file created by patchoat.
+//
+// The tests don't work when heap poisoning is enabled because some of the
+// references are negated. b/72117833 is tracking the effort to have patchoat
+// and its tests support heap poisoning.
+class PatchoatVerificationTest : public PatchoatTest {
+ protected:
+  void CreateRelocatedBootImage() {
+    // Compile boot image into a random directory using dex2oat
+    ScratchFile dex2oat_orig_scratch;
+    dex2oat_orig_scratch.Unlink();
+    dex2oat_orig_dir_ = dex2oat_orig_scratch.GetFilename();
+    ASSERT_EQ(0, mkdir(dex2oat_orig_dir_.c_str(), 0700));
+    const uint32_t orig_base_addr = 0x60000000;
+    std::vector<std::string> dex2oat_extra_args;
+    std::string error_msg;
+    if (!CompileBootImageToDir(dex2oat_orig_dir_, dex2oat_extra_args, orig_base_addr, &error_msg)) {
+      FAIL() << "CompileBootImage1 failed: " << error_msg;
+    }
 
-  // This test doesn't work when heap poisoning is enabled because some of the
-  // references are negated. b/72117833 is tracking the effort to have patchoat
-  // and its tests support heap poisoning.
-  TEST_DISABLED_FOR_HEAP_POISONING();
+    // Generate image relocation file for the original boot image
+    std::string dex2oat_orig_with_arch_dir =
+        dex2oat_orig_dir_ + "/" + GetInstructionSetString(kRuntimeISA);
+    // The arch-including symlink is needed by patchoat
+    ASSERT_EQ(0, symlink(dex2oat_orig_dir_.c_str(), dex2oat_orig_with_arch_dir.c_str()));
+    base_addr_delta_ = 0x100000;
+    if (!GenerateBootImageRelFile(
+        dex2oat_orig_dir_ + "/boot.art",
+        dex2oat_orig_dir_,
+        base_addr_delta_,
+        &error_msg)) {
+      FAIL() << "RelocateBootImage failed: " << error_msg;
+    }
 
-  // Compile boot image into a random directory using dex2oat
-  ScratchFile dex2oat_orig_scratch;
-  dex2oat_orig_scratch.Unlink();
-  std::string dex2oat_orig_dir = dex2oat_orig_scratch.GetFilename();
-  ASSERT_EQ(0, mkdir(dex2oat_orig_dir.c_str(), 0700));
-  const uint32_t orig_base_addr = 0x60000000;
-  std::vector<std::string> dex2oat_extra_args;
-  std::string error_msg;
-  if (!CompileBootImageToDir(dex2oat_orig_dir, dex2oat_extra_args, orig_base_addr, &error_msg)) {
-    FAIL() << "CompileBootImage1 failed: " << error_msg;
+    // Relocate the original boot image using patchoat
+    ScratchFile relocated_scratch;
+    relocated_scratch.Unlink();
+    relocated_dir_ = relocated_scratch.GetFilename();
+    ASSERT_EQ(0, mkdir(relocated_dir_.c_str(), 0700));
+    // Use a different relocation delta from the one used when generating .rel files above. This is
+    // to make sure .rel files are not specific to a particular relocation delta.
+    base_addr_delta_ -= 0x10000;
+    if (!RelocateBootImage(
+        dex2oat_orig_dir_ + "/boot.art",
+        relocated_dir_,
+        base_addr_delta_,
+        &error_msg)) {
+      FAIL() << "RelocateBootImage failed: " << error_msg;
+    }
+
+    // Assert that patchoat created the same set of .art and .art.rel files
+    std::vector<std::string> rel_basenames;
+    std::vector<std::string> relocated_image_basenames;
+    if (!ListDirFilesEndingWith(dex2oat_orig_dir_, ".rel", &rel_basenames, &error_msg)) {
+      FAIL() << "Failed to list *.art.rel files in " << dex2oat_orig_dir_ << ": " << error_msg;
+    }
+    if (!ListDirFilesEndingWith(relocated_dir_, ".art", &relocated_image_basenames, &error_msg)) {
+      FAIL() << "Failed to list *.art files in " << relocated_dir_ << ": " << error_msg;
+    }
+    std::sort(rel_basenames.begin(), rel_basenames.end());
+    std::sort(relocated_image_basenames.begin(), relocated_image_basenames.end());
+
+    // .art and .art.rel file names output by patchoat look like
+    // tmp@art-data-<random>-<random>@boot*.art, encoding the name of the directory in their name.
+    // To compare these with each other, we retain only the part of the file name after the last @,
+    // and we also drop the extension.
+    std::vector<std::string> rel_shortened_basenames(rel_basenames.size());
+    std::vector<std::string> relocated_image_shortened_basenames(relocated_image_basenames.size());
+    for (size_t i = 0; i < rel_basenames.size(); i++) {
+      rel_shortened_basenames[i] = rel_basenames[i].substr(rel_basenames[i].find_last_of("@") + 1);
+      rel_shortened_basenames[i] =
+          rel_shortened_basenames[i].substr(0, rel_shortened_basenames[i].find("."));
+    }
+    for (size_t i = 0; i < relocated_image_basenames.size(); i++) {
+      relocated_image_shortened_basenames[i] =
+          relocated_image_basenames[i].substr(relocated_image_basenames[i].find_last_of("@") + 1);
+      relocated_image_shortened_basenames[i] =
+          relocated_image_shortened_basenames[i].substr(
+              0, relocated_image_shortened_basenames[i].find("."));
+    }
+    ASSERT_EQ(rel_shortened_basenames, relocated_image_shortened_basenames);
   }
 
-  // Generate image relocation file for the original boot image
-  std::string dex2oat_orig_with_arch_dir =
-      dex2oat_orig_dir + "/" + GetInstructionSetString(kRuntimeISA);
-  // The arch-including symlink is needed by patchoat
-  ASSERT_EQ(0, symlink(dex2oat_orig_dir.c_str(), dex2oat_orig_with_arch_dir.c_str()));
-  off_t base_addr_delta = 0x100000;
-  if (!GenerateBootImageRelFile(
-      dex2oat_orig_dir + "/boot.art",
-      dex2oat_orig_dir,
-      base_addr_delta,
-      &error_msg)) {
-    FAIL() << "RelocateBootImage failed: " << error_msg;
+  virtual void TearDown() {
+    if (!dex2oat_orig_dir_.empty()) {
+      ClearDirectory(dex2oat_orig_dir_.c_str(), /*recursive*/ true);
+      rmdir(dex2oat_orig_dir_.c_str());
+    }
+    if (!relocated_dir_.empty()) {
+      ClearDirectory(relocated_dir_.c_str(), /*recursive*/ true);
+      rmdir(relocated_dir_.c_str());
+    }
+    PatchoatTest::TearDown();
   }
 
-  // Relocate the original boot image using patchoat
-  ScratchFile relocated_scratch;
-  relocated_scratch.Unlink();
-  std::string relocated_dir = relocated_scratch.GetFilename();
-  ASSERT_EQ(0, mkdir(relocated_dir.c_str(), 0700));
-  // Use a different relocation delta from the one used when generating .rel files above. This is
-  // to make sure .rel files are not specific to a particular relocation delta.
-  base_addr_delta -= 0x10000;
-  if (!RelocateBootImage(
-      dex2oat_orig_dir + "/boot.art",
-      relocated_dir,
-      base_addr_delta,
+  std::string dex2oat_orig_dir_;
+  std::string relocated_dir_;
+  off_t base_addr_delta_;
+};
+
+// Assert that verification works with the .rel files.
+TEST_F(PatchoatVerificationTest, Sucessful) {
+  TEST_DISABLED_FOR_HEAP_POISONING();
+  CreateRelocatedBootImage();
+
+  std::string error_msg;
+  if (!VerifyBootImage(
+      dex2oat_orig_dir_ + "/boot.art",
+      relocated_dir_,
+      base_addr_delta_,
       &error_msg)) {
-    FAIL() << "RelocateBootImage failed: " << error_msg;
+    FAIL() << "VerifyBootImage failed: " << error_msg;
   }
+}
 
-  // Assert that patchoat created the same set of .art and .art.rel files
-  std::vector<std::string> rel_basenames;
-  std::vector<std::string> relocated_image_basenames;
-  if (!ListDirFilesEndingWith(dex2oat_orig_dir, ".rel", &rel_basenames, &error_msg)) {
-    FAIL() << "Failed to list *.art.rel files in " << dex2oat_orig_dir << ": " << error_msg;
-  }
-  if (!ListDirFilesEndingWith(relocated_dir, ".art", &relocated_image_basenames, &error_msg)) {
-    FAIL() << "Failed to list *.art files in " << relocated_dir << ": " << error_msg;
-  }
-  std::sort(rel_basenames.begin(), rel_basenames.end());
-  std::sort(relocated_image_basenames.begin(), relocated_image_basenames.end());
-
-  // .art and .art.rel file names output by patchoat look like
-  // tmp@art-data-<random>-<random>@boot*.art, encoding the name of the directory in their name.
-  // To compare these with each other, we retain only the part of the file name after the last @,
-  // and we also drop the extension.
-  std::vector<std::string> rel_shortened_basenames(rel_basenames.size());
-  std::vector<std::string> relocated_image_shortened_basenames(relocated_image_basenames.size());
-  for (size_t i = 0; i < rel_basenames.size(); i++) {
-    rel_shortened_basenames[i] = rel_basenames[i].substr(rel_basenames[i].find_last_of("@") + 1);
-    rel_shortened_basenames[i] =
-        rel_shortened_basenames[i].substr(0, rel_shortened_basenames[i].find("."));
-  }
-  for (size_t i = 0; i < relocated_image_basenames.size(); i++) {
-    relocated_image_shortened_basenames[i] =
-        relocated_image_basenames[i].substr(relocated_image_basenames[i].find_last_of("@") + 1);
-    relocated_image_shortened_basenames[i] =
-        relocated_image_shortened_basenames[i].substr(
-            0, relocated_image_shortened_basenames[i].find("."));
+// Corrupt the image file and check that the verification fails gracefully.
+TEST_F(PatchoatVerificationTest, CorruptedImage) {
+  TEST_DISABLED_FOR_HEAP_POISONING();
+  CreateRelocatedBootImage();
+
+  std::string error_msg;
+  std::string relocated_image_filename;
+  if (!GetDalvikCacheFilename((dex2oat_orig_dir_ + "/boot.art").c_str(),
+                               relocated_dir_.c_str(),
+                               &relocated_image_filename,
+                               &error_msg)) {
+    FAIL() << "Failed to find relocated image file name: " << error_msg;
   }
-  ASSERT_EQ(rel_shortened_basenames, relocated_image_shortened_basenames);
+  ASSERT_EQ(truncate(relocated_image_filename.c_str(), sizeof(ImageHeader)), 0)
+    << relocated_image_filename;
 
-  // Assert that verification works with the .rel files.
-  if (!VerifyBootImage(
-      dex2oat_orig_dir + "/boot.art",
-      relocated_dir,
-      base_addr_delta,
+  if (VerifyBootImage(
+      dex2oat_orig_dir_ + "/boot.art",
+      relocated_dir_,
+      base_addr_delta_,
       &error_msg)) {
-    FAIL() << "VerifyBootImage failed: " << error_msg;
+    FAIL() << "VerifyBootImage should have failed since the image was intentionally corrupted";
   }
+}
 
-  ClearDirectory(dex2oat_orig_dir.c_str(), /*recursive*/ true);
-  ClearDirectory(relocated_dir.c_str(), /*recursive*/ true);
+// Corrupt the relocation file and check that the verification fails gracefully.
+TEST_F(PatchoatVerificationTest, CorruptedRelFile) {
+  TEST_DISABLED_FOR_HEAP_POISONING();
+  CreateRelocatedBootImage();
 
-  rmdir(dex2oat_orig_dir.c_str());
-  rmdir(relocated_dir.c_str());
+  std::string error_msg;
+  std::string art_filename = dex2oat_orig_dir_ + "/boot.art";
+  std::string rel_filename = dex2oat_orig_dir_ + "/boot.art.rel";
+  std::unique_ptr<File> art_file(OS::OpenFileForReading(art_filename.c_str()));
+  std::unique_ptr<File> rel_file(OS::OpenFileReadWrite(rel_filename.c_str()));
+  rel_file->ClearContent();
+  uint8_t buffer[64] = {};
+  ASSERT_TRUE(rel_file->WriteFully(&buffer, SHA256_DIGEST_LENGTH));
+  // Encode single relocation which is just past the end of the image file.
+  size_t leb_size = EncodeUnsignedLeb128(buffer, art_file->GetLength()) - buffer;
+  ASSERT_TRUE(rel_file->WriteFully(&buffer, leb_size));
+  ASSERT_EQ(rel_file->FlushClose(), 0);
+  ASSERT_EQ(art_file->Close(), 0);
+
+  if (VerifyBootImage(
+      dex2oat_orig_dir_ + "/boot.art",
+      relocated_dir_,
+      base_addr_delta_,
+      &error_msg)) {
+    FAIL() << "VerifyBootImage should have failed since the rel file was intentionally corrupted";
+  }
 }
 
 }  // namespace art