Fix OatHeader.isValid

Change-Id: I4371b9e7082a5131622680fea472da9d7e24f750

3 files changed, 47 insertions, 19 deletions

diff --git a/src/oat.cc b/src/oat.cc
index 6284f9a4eb..a328a1149b 100644
--- a/src/oat.cc
+++ b/src/oat.cc
@@ -37,6 +37,7 @@ OatHeader::OatHeader(InstructionSet instruction_set,
 
   adler32_checksum_ = adler32(0L, Z_NULL, 0);
 
+  CHECK_NE(instruction_set, kNone);
   instruction_set_ = instruction_set;
   UpdateChecksum(&instruction_set_, sizeof(instruction_set_));
 
@@ -58,10 +59,10 @@ OatHeader::OatHeader(InstructionSet instruction_set,
 }
 
 bool OatHeader::IsValid() const {
-  if (memcmp(magic_, kOatMagic, sizeof(kOatMagic) != 0)) {
+  if (memcmp(magic_, kOatMagic, sizeof(kOatMagic)) != 0) {
     return false;
   }
-  if (memcmp(version_, kOatVersion, sizeof(kOatVersion) != 0)) {
+  if (memcmp(version_, kOatVersion, sizeof(kOatVersion)) != 0) {
     return false;
   }
   return true;
diff --git a/src/oat_file.cc b/src/oat_file.cc
index 58ba44c716..1e9d820716 100644
--- a/src/oat_file.cc
+++ b/src/oat_file.cc
@@ -49,7 +49,7 @@ OatFile* OatFile::Open(File& file,
                        bool writable) {
   CHECK(!location.empty());
   if (!IsValidOatFilename(location)) {
-    LOG(WARNING) << "Attempting to open dex file with unknown extension '" << location << "'";
+    LOG(WARNING) << "Attempting to open oat file with unknown extension '" << location << "'";
   }
   UniquePtr<OatFile> oat_file(new OatFile(location));
   bool success = oat_file->Map(file, requested_base, reloc, writable);
@@ -102,24 +102,27 @@ bool OatFile::Map(File& file,
                                                  file.Fd(),
                                                  0));
   if (map.get() == NULL) {
-    LOG(WARNING) << "Failed to map oat file " << GetLocation();
+    LOG(WARNING) << "Failed to map oat file from " << file.name() << " for " << GetLocation();
     return false;
   }
   CHECK(requested_base == 0 || requested_base == map->Begin())
-      << GetLocation() << " " << reinterpret_cast<void*>(map->Begin());
-  DCHECK_EQ(0, memcmp(&oat_header, map->Begin(), sizeof(OatHeader))) << GetLocation();
+      << file.name() << " for " << GetLocation() << " " << reinterpret_cast<void*>(map->Begin());
+  DCHECK_EQ(0, memcmp(&oat_header, map->Begin(), sizeof(OatHeader)))
+      << file.name() << " for " << GetLocation();
 
   off_t code_offset = oat_header.GetExecutableOffset();
   if (code_offset < file.Length()) {
     byte* code_address = map->Begin() + code_offset;
     size_t code_length = file.Length() - code_offset;
     if (mprotect(code_address, code_length, prot | PROT_EXEC) != 0) {
-      PLOG(ERROR) << "Failed to make oat code executable in " << GetLocation();
+      PLOG(ERROR) << "Failed to make oat code executable in "
+                  << file.name() << " for " << GetLocation();
       return false;
     }
   } else {
     // its possible to have no code if all the methods were abstract, native, etc
-    DCHECK_EQ(code_offset, RoundUp(file.Length(), kPageSize)) << GetLocation();
+    DCHECK_EQ(code_offset, RoundUp(file.Length(), kPageSize))
+        << file.name() << " for " << GetLocation();
   }
 
   const byte* oat = map->Begin();
@@ -132,37 +135,40 @@ bool OatFile::Map(File& file,
     << "+" << sizeof(OatHeader)
     << "+" << oat_header.GetImageFileLocationSize()
     << "<=" << reinterpret_cast<void*>(map->End())
-    << " " << GetLocation();
+    << " " << file.name() << " for " << GetLocation();
   for (size_t i = 0; i < oat_header.GetDexFileCount(); i++) {
     size_t dex_file_location_size = *reinterpret_cast<const uint32_t*>(oat);
-    CHECK_GT(dex_file_location_size, 0U) << GetLocation();
+    CHECK_GT(dex_file_location_size, 0U) << file.name() << " for " << GetLocation();
     oat += sizeof(dex_file_location_size);
-    CHECK_LT(oat, map->End()) << GetLocation();
+    CHECK_LT(oat, map->End()) << file.name() << " for " << GetLocation();
 
     const char* dex_file_location_data = reinterpret_cast<const char*>(oat);
     oat += dex_file_location_size;
-    CHECK_LT(oat, map->End()) << GetLocation();
+    CHECK_LT(oat, map->End()) << file.name() << " for " << GetLocation();
 
     std::string dex_file_location(dex_file_location_data, dex_file_location_size);
 
     uint32_t dex_file_checksum = *reinterpret_cast<const uint32_t*>(oat);
     oat += sizeof(dex_file_checksum);
-    CHECK_LT(oat, map->End()) << GetLocation();
+    CHECK_LT(oat, map->End()) << file.name() << " for " << GetLocation();
 
     uint32_t dex_file_offset = *reinterpret_cast<const uint32_t*>(oat);
-    CHECK_GT(dex_file_offset, 0U) << GetLocation();
-    CHECK_LT(dex_file_offset, static_cast<uint32_t>(file.Length())) << GetLocation();
+    CHECK_GT(dex_file_offset, 0U) << file.name() << " for " << GetLocation();
+    CHECK_LT(dex_file_offset, static_cast<uint32_t>(file.Length()))
+        << file.name() << " for " << GetLocation();
     oat += sizeof(dex_file_offset);
-    CHECK_LT(oat, map->End()) << GetLocation();
+    CHECK_LT(oat, map->End()) << file.name() << " for " << GetLocation();
 
     uint8_t* dex_file_pointer = map->Begin() + dex_file_offset;
-    CHECK(DexFile::IsMagicValid(dex_file_pointer)) << GetLocation() << " " << dex_file_pointer;
-    CHECK(DexFile::IsVersionValid(dex_file_pointer)) << GetLocation() << " "  << dex_file_pointer;
+    CHECK(DexFile::IsMagicValid(dex_file_pointer))
+        << file.name() << " for " << GetLocation() << " " << dex_file_pointer;
+    CHECK(DexFile::IsVersionValid(dex_file_pointer))
+        << file.name() << " for " << GetLocation() << " "  << dex_file_pointer;
     const DexFile::Header* header = reinterpret_cast<const DexFile::Header*>(dex_file_pointer);
     const uint32_t* methods_offsets_pointer = reinterpret_cast<const uint32_t*>(oat);
 
     oat += (sizeof(*methods_offsets_pointer) * header->class_defs_size_);
-    CHECK_LE(oat, map->End()) << GetLocation();
+    CHECK_LE(oat, map->End()) << file.name() << " for " << GetLocation();
 
     oat_dex_files_.Put(dex_file_location, new OatDexFile(this,
                                                          dex_file_location,
diff --git a/src/oat_test.cc b/src/oat_test.cc
index 3df831c2f9..64c502d39a 100644
--- a/src/oat_test.cc
+++ b/src/oat_test.cc
@@ -89,6 +89,7 @@ TEST_F(OatTest, WriteRead) {
                                             OatFile::kRelocNone));
   ASSERT_TRUE(oat_file.get() != NULL);
   const OatHeader& oat_header = oat_file->GetOatHeader();
+  ASSERT_TRUE(oat_header.IsValid());
   ASSERT_EQ(1U, oat_header.GetDexFileCount());
   ASSERT_EQ(42U, oat_header.GetImageFileLocationOatChecksum());
   ASSERT_EQ(4096U, oat_header.GetImageFileLocationOatBegin());
@@ -135,4 +136,24 @@ TEST_F(OatTest, OatHeaderSizeCheck) {
 #endif
 }
 
+TEST_F(OatTest, OatHeaderIsValid) {
+    InstructionSet instruction_set = kX86;
+    std::vector<const DexFile*> dex_files;
+    uint32_t image_file_location_oat_checksum = 0;
+    uint32_t image_file_location_oat_begin = 0;
+    const std::string image_file_location;
+    OatHeader oat_header(instruction_set,
+                         &dex_files,
+                         image_file_location_oat_checksum,
+                         image_file_location_oat_begin,
+                         image_file_location);
+    ASSERT_TRUE(oat_header.IsValid());
+
+    char* magic = const_cast<char*>(oat_header.GetMagic());
+    strcpy(magic, "");  // bad magic
+    ASSERT_FALSE(oat_header.IsValid());
+    strcpy(magic, "oat\n000"); // bad version
+    ASSERT_FALSE(oat_header.IsValid());
+}
+
 }  // namespace art