ART: Simple structural class check
Adds a simple check to class-loading when the embedded dex file in
an oat file and the dex file on the class path where we found the
class do not match.
We require that the number of methods and fields do not change, as
that will almost certainly mean that quickened and other compiled
offsets are wrong now. This is a reasonably lightweight change, but
we should investigate a full comparison including name and type of
members.
Bug: 17937814
Bug: 18708951
Change-Id: Icb9638bebd369ab23822817f4a97c8dd8625fea5
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index cd1a93a..702e901 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -4507,6 +4507,171 @@
return true;
}
+static void CountMethodsAndFields(ClassDataItemIterator& dex_data,
+ size_t* virtual_methods,
+ size_t* direct_methods,
+ size_t* static_fields,
+ size_t* instance_fields) {
+ *virtual_methods = *direct_methods = *static_fields = *instance_fields = 0;
+
+ while (dex_data.HasNextStaticField()) {
+ dex_data.Next();
+ (*static_fields)++;
+ }
+ while (dex_data.HasNextInstanceField()) {
+ dex_data.Next();
+ (*instance_fields)++;
+ }
+ while (dex_data.HasNextDirectMethod()) {
+ (*direct_methods)++;
+ dex_data.Next();
+ }
+ while (dex_data.HasNextVirtualMethod()) {
+ (*virtual_methods)++;
+ dex_data.Next();
+ }
+ DCHECK(!dex_data.HasNext());
+}
+
+static void DumpClass(std::ostream& os,
+ const DexFile& dex_file, const DexFile::ClassDef& dex_class_def,
+ const char* suffix) {
+ ClassDataItemIterator dex_data(dex_file, dex_file.GetClassData(dex_class_def));
+ os << dex_file.GetClassDescriptor(dex_class_def) << suffix << ":\n";
+ os << " Static fields:\n";
+ while (dex_data.HasNextStaticField()) {
+ const DexFile::FieldId& id = dex_file.GetFieldId(dex_data.GetMemberIndex());
+ os << " " << dex_file.GetFieldTypeDescriptor(id) << " " << dex_file.GetFieldName(id) << "\n";
+ dex_data.Next();
+ }
+ os << " Instance fields:\n";
+ while (dex_data.HasNextInstanceField()) {
+ const DexFile::FieldId& id = dex_file.GetFieldId(dex_data.GetMemberIndex());
+ os << " " << dex_file.GetFieldTypeDescriptor(id) << " " << dex_file.GetFieldName(id) << "\n";
+ dex_data.Next();
+ }
+ os << " Direct methods:\n";
+ while (dex_data.HasNextDirectMethod()) {
+ const DexFile::MethodId& id = dex_file.GetMethodId(dex_data.GetMemberIndex());
+ os << " " << dex_file.GetMethodName(id) << dex_file.GetMethodSignature(id).ToString() << "\n";
+ dex_data.Next();
+ }
+ os << " Virtual methods:\n";
+ while (dex_data.HasNextVirtualMethod()) {
+ const DexFile::MethodId& id = dex_file.GetMethodId(dex_data.GetMemberIndex());
+ os << " " << dex_file.GetMethodName(id) << dex_file.GetMethodSignature(id).ToString() << "\n";
+ dex_data.Next();
+ }
+}
+
+static std::string DumpClasses(const DexFile& dex_file1, const DexFile::ClassDef& dex_class_def1,
+ const DexFile& dex_file2, const DexFile::ClassDef& dex_class_def2) {
+ std::ostringstream os;
+ DumpClass(os, dex_file1, dex_class_def1, " (Compile time)");
+ DumpClass(os, dex_file2, dex_class_def2, " (Runtime)");
+ return os.str();
+}
+
+
+// Very simple structural check on whether the classes match. Only compares the number of
+// methods and fields.
+static bool SimpleStructuralCheck(const DexFile& dex_file1, const DexFile::ClassDef& dex_class_def1,
+ const DexFile& dex_file2, const DexFile::ClassDef& dex_class_def2,
+ std::string* error_msg) {
+ ClassDataItemIterator dex_data1(dex_file1, dex_file1.GetClassData(dex_class_def1));
+ ClassDataItemIterator dex_data2(dex_file2, dex_file2.GetClassData(dex_class_def2));
+
+ // Counters for current dex file.
+ size_t dex_virtual_methods1, dex_direct_methods1, dex_static_fields1, dex_instance_fields1;
+ CountMethodsAndFields(dex_data1, &dex_virtual_methods1, &dex_direct_methods1, &dex_static_fields1,
+ &dex_instance_fields1);
+ // Counters for compile-time dex file.
+ size_t dex_virtual_methods2, dex_direct_methods2, dex_static_fields2, dex_instance_fields2;
+ CountMethodsAndFields(dex_data2, &dex_virtual_methods2, &dex_direct_methods2, &dex_static_fields2,
+ &dex_instance_fields2);
+
+ if (dex_virtual_methods1 != dex_virtual_methods2) {
+ std::string class_dump = DumpClasses(dex_file1, dex_class_def1, dex_file2, dex_class_def2);
+ *error_msg = StringPrintf("Virtual method count off: %zu vs %zu\n%s", dex_virtual_methods1,
+ dex_virtual_methods2, class_dump.c_str());
+ return false;
+ }
+ if (dex_direct_methods1 != dex_direct_methods2) {
+ std::string class_dump = DumpClasses(dex_file1, dex_class_def1, dex_file2, dex_class_def2);
+ *error_msg = StringPrintf("Direct method count off: %zu vs %zu\n%s", dex_direct_methods1,
+ dex_direct_methods2, class_dump.c_str());
+ return false;
+ }
+ if (dex_static_fields1 != dex_static_fields2) {
+ std::string class_dump = DumpClasses(dex_file1, dex_class_def1, dex_file2, dex_class_def2);
+ *error_msg = StringPrintf("Static field count off: %zu vs %zu\n%s", dex_static_fields1,
+ dex_static_fields2, class_dump.c_str());
+ return false;
+ }
+ if (dex_instance_fields1 != dex_instance_fields2) {
+ std::string class_dump = DumpClasses(dex_file1, dex_class_def1, dex_file2, dex_class_def2);
+ *error_msg = StringPrintf("Instance field count off: %zu vs %zu\n%s", dex_instance_fields1,
+ dex_instance_fields2, class_dump.c_str());
+ return false;
+ }
+
+ return true;
+}
+
+// Checks whether a the super-class changed from what we had at compile-time. This would
+// invalidate quickening.
+static bool CheckSuperClassChange(Handle<mirror::Class> klass,
+ const DexFile& dex_file,
+ const DexFile::ClassDef& class_def,
+ mirror::Class* super_class)
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+ // Check for unexpected changes in the superclass.
+ // Quick check 1) is the super_class class-loader the boot class loader? This always has
+ // precedence.
+ if (super_class->GetClassLoader() != nullptr &&
+ // Quick check 2) different dex cache? Breaks can only occur for different dex files,
+ // which is implied by different dex cache.
+ klass->GetDexCache() != super_class->GetDexCache()) {
+ // Now comes the expensive part: things can be broken if (a) the klass' dex file has a
+ // definition for the super-class, and (b) the files are in separate oat files. The oat files
+ // are referenced from the dex file, so do (b) first. Only relevant if we have oat files.
+ const OatFile* class_oat_file = dex_file.GetOatFile();
+ if (class_oat_file != nullptr) {
+ const OatFile* loaded_super_oat_file = super_class->GetDexFile().GetOatFile();
+ if (loaded_super_oat_file != nullptr && class_oat_file != loaded_super_oat_file) {
+ // Now check (a).
+ const DexFile::ClassDef* super_class_def = dex_file.FindClassDef(class_def.superclass_idx_);
+ if (super_class_def != nullptr) {
+ // Uh-oh, we found something. Do our check.
+ std::string error_msg;
+ if (!SimpleStructuralCheck(dex_file, *super_class_def,
+ super_class->GetDexFile(), *super_class->GetClassDef(),
+ &error_msg)) {
+ // Print a warning to the log. This exception might be caught, e.g., as common in test
+ // drivers. When the class is later tried to be used, we re-throw a new instance, as we
+ // only save the type of the exception.
+ LOG(WARNING) << "Incompatible structural change detected: " <<
+ StringPrintf(
+ "Structural change of %s is hazardous (%s at compile time, %s at runtime): %s",
+ PrettyType(super_class_def->class_idx_, dex_file).c_str(),
+ class_oat_file->GetLocation().c_str(),
+ loaded_super_oat_file->GetLocation().c_str(),
+ error_msg.c_str());
+ ThrowIncompatibleClassChangeError(klass.Get(),
+ "Structural change of %s is hazardous (%s at compile time, %s at runtime): %s",
+ PrettyType(super_class_def->class_idx_, dex_file).c_str(),
+ class_oat_file->GetLocation().c_str(),
+ loaded_super_oat_file->GetLocation().c_str(),
+ error_msg.c_str());
+ return false;
+ }
+ }
+ }
+ }
+ }
+ return true;
+}
+
bool ClassLinker::LoadSuperAndInterfaces(Handle<mirror::Class> klass, const DexFile& dex_file) {
CHECK_EQ(mirror::Class::kStatusIdx, klass->GetStatus());
const DexFile::ClassDef& class_def = dex_file.GetClassDef(klass->GetDexClassDefIndex());
@@ -4526,6 +4691,11 @@
}
CHECK(super_class->IsResolved());
klass->SetSuperClass(super_class);
+
+ if (!CheckSuperClassChange(klass, dex_file, class_def, super_class)) {
+ DCHECK(Thread::Current()->IsExceptionPending());
+ return false;
+ }
}
const DexFile::TypeList* interfaces = dex_file.GetInterfacesList(class_def);
if (interfaces != nullptr) {
diff --git a/runtime/dex_file.cc b/runtime/dex_file.cc
index 31d79bf..9d835f4 100644
--- a/runtime/dex_file.cc
+++ b/runtime/dex_file.cc
@@ -241,6 +241,7 @@
location,
location_checksum,
mem_map,
+ nullptr,
error_msg);
}
@@ -326,9 +327,12 @@
size_t size,
const std::string& location,
uint32_t location_checksum,
- MemMap* mem_map, std::string* error_msg) {
+ MemMap* mem_map,
+ const OatFile* oat_file,
+ std::string* error_msg) {
CHECK_ALIGNED(base, 4); // various dex file structures must be word aligned
- std::unique_ptr<DexFile> dex_file(new DexFile(base, size, location, location_checksum, mem_map));
+ std::unique_ptr<DexFile> dex_file(
+ new DexFile(base, size, location, location_checksum, mem_map, oat_file));
if (!dex_file->Init(error_msg)) {
return nullptr;
} else {
@@ -339,7 +343,8 @@
DexFile::DexFile(const byte* base, size_t size,
const std::string& location,
uint32_t location_checksum,
- MemMap* mem_map)
+ MemMap* mem_map,
+ const OatFile* oat_file)
: begin_(base),
size_(size),
location_(location),
@@ -354,7 +359,8 @@
class_defs_(reinterpret_cast<const ClassDef*>(base + header_->class_defs_off_)),
find_class_def_misses_(0),
class_def_index_(nullptr),
- build_class_def_index_mutex_("DexFile index creation mutex") {
+ build_class_def_index_mutex_("DexFile index creation mutex"),
+ oat_file_(oat_file) {
CHECK(begin_ != NULL) << GetLocation();
CHECK_GT(size_, 0U) << GetLocation();
}
diff --git a/runtime/dex_file.h b/runtime/dex_file.h
index 54c4b09..060562b 100644
--- a/runtime/dex_file.h
+++ b/runtime/dex_file.h
@@ -43,6 +43,7 @@
} // namespace mirror
class ClassLinker;
class MemMap;
+class OatFile;
class Signature;
template<class T> class Handle;
class StringPiece;
@@ -390,8 +391,9 @@
static const DexFile* Open(const uint8_t* base, size_t size,
const std::string& location,
uint32_t location_checksum,
+ const OatFile* oat_file,
std::string* error_msg) {
- return OpenMemory(base, size, location, location_checksum, NULL, error_msg);
+ return OpenMemory(base, size, location, location_checksum, NULL, oat_file, error_msg);
}
// Open all classesXXX.dex files from a zip archive.
@@ -889,6 +891,10 @@
// the dex_location where it's file name part has been made canonical.
static std::string GetDexCanonicalLocation(const char* dex_location);
+ const OatFile* GetOatFile() const {
+ return oat_file_;
+ }
+
private:
// Opens a .dex file
static const DexFile* OpenFile(int fd, const char* location, bool verify, std::string* error_msg);
@@ -924,12 +930,14 @@
const std::string& location,
uint32_t location_checksum,
MemMap* mem_map,
+ const OatFile* oat_file,
std::string* error_msg);
DexFile(const byte* base, size_t size,
const std::string& location,
uint32_t location_checksum,
- MemMap* mem_map);
+ MemMap* mem_map,
+ const OatFile* oat_file);
// Top-level initializer that calls other Init methods.
bool Init(std::string* error_msg);
@@ -1013,6 +1021,10 @@
typedef HashMap<const char*, const ClassDef*, UTF16EmptyFn, UTF16HashCmp, UTF16HashCmp> Index;
mutable Atomic<Index*> class_def_index_;
mutable Mutex build_class_def_index_mutex_ DEFAULT_MUTEX_ACQUIRED_AFTER;
+
+ // The oat file this dex file was loaded from. May be null in case the dex file is not coming
+ // from an oat file, e.g., directly from an apk.
+ const OatFile* oat_file_;
};
std::ostream& operator<<(std::ostream& os, const DexFile& dex_file);
diff --git a/runtime/oat_file.cc b/runtime/oat_file.cc
index 9c06ebe..8bf7ad4 100644
--- a/runtime/oat_file.cc
+++ b/runtime/oat_file.cc
@@ -458,7 +458,7 @@
const DexFile* OatFile::OatDexFile::OpenDexFile(std::string* error_msg) const {
return DexFile::Open(dex_file_pointer_, FileSize(), dex_file_location_,
- dex_file_location_checksum_, error_msg);
+ dex_file_location_checksum_, GetOatFile(), error_msg);
}
uint32_t OatFile::OatDexFile::GetOatClassOffset(uint16_t class_def_index) const {
diff --git a/test/131-structural-change/build b/test/131-structural-change/build
new file mode 100755
index 0000000..7ddc81d
--- /dev/null
+++ b/test/131-structural-change/build
@@ -0,0 +1,31 @@
+#!/bin/bash
+#
+# Copyright (C) 2015 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Stop if something fails.
+set -e
+
+mkdir classes
+${JAVAC} -d classes `find src -name '*.java'`
+
+mkdir classes-ex
+${JAVAC} -d classes-ex `find src-ex -name '*.java'`
+
+if [ ${NEED_DEX} = "true" ]; then
+ ${DX} -JXmx256m --debug --dex --dump-to=classes.lst --output=classes.dex --dump-width=1000 classes
+ zip $TEST_NAME.jar classes.dex
+ ${DX} -JXmx256m --debug --dex --dump-to=classes-ex.lst --output=classes.dex --dump-width=1000 classes-ex
+ zip ${TEST_NAME}-ex.jar classes.dex
+fi
diff --git a/test/131-structural-change/expected.txt b/test/131-structural-change/expected.txt
new file mode 100644
index 0000000..79facd5
--- /dev/null
+++ b/test/131-structural-change/expected.txt
@@ -0,0 +1,2 @@
+Should really reach here.
+Got expected error.
diff --git a/test/131-structural-change/info.txt b/test/131-structural-change/info.txt
new file mode 100644
index 0000000..6d5817b
--- /dev/null
+++ b/test/131-structural-change/info.txt
@@ -0,0 +1 @@
+Check whether a structural change in a (non-native) multi-dex scenario is detected.
diff --git a/test/131-structural-change/run b/test/131-structural-change/run
new file mode 100755
index 0000000..63fdb8c
--- /dev/null
+++ b/test/131-structural-change/run
@@ -0,0 +1,18 @@
+#!/bin/bash
+#
+# Copyright (C) 2015 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Use secondary switch to add secondary dex file to class path.
+exec ${RUN} "${@}" --secondary
diff --git a/test/131-structural-change/src-ex/A.java b/test/131-structural-change/src-ex/A.java
new file mode 100644
index 0000000..800347b
--- /dev/null
+++ b/test/131-structural-change/src-ex/A.java
@@ -0,0 +1,20 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class A {
+ public void bar() {
+ }
+}
diff --git a/test/131-structural-change/src-ex/B.java b/test/131-structural-change/src-ex/B.java
new file mode 100644
index 0000000..61369db
--- /dev/null
+++ b/test/131-structural-change/src-ex/B.java
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class B extends A {
+ public void test() {
+ System.out.println("Should not reach this!");
+ }
+}
diff --git a/test/131-structural-change/src/A.java b/test/131-structural-change/src/A.java
new file mode 100644
index 0000000..b07de58
--- /dev/null
+++ b/test/131-structural-change/src/A.java
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class A {
+ public void foo() {
+ }
+
+ public void bar() {
+ }
+
+ public void baz() {
+ }
+}
diff --git a/test/131-structural-change/src/Main.java b/test/131-structural-change/src/Main.java
new file mode 100644
index 0000000..c94843e
--- /dev/null
+++ b/test/131-structural-change/src/Main.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.io.File;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+
+/**
+ * Structural hazard test.
+ */
+public class Main {
+ public static void main(String[] args) {
+ new Main().run();
+ }
+
+ private void run() {
+ try {
+ Class<?> bClass = getClass().getClassLoader().loadClass("A");
+ System.out.println("Should really reach here.");
+ } catch (Exception e) {
+ e.printStackTrace(System.out);
+ }
+
+ try {
+ Class<?> bClass = getClass().getClassLoader().loadClass("B");
+ System.out.println("Should not reach here.");
+ } catch (IncompatibleClassChangeError icce) {
+ System.out.println("Got expected error.");
+ } catch (Exception e) {
+ e.printStackTrace(System.out);
+ }
+
+ }
+
+}
diff --git a/test/run-test b/test/run-test
index e48681a..3582628 100755
--- a/test/run-test
+++ b/test/run-test
@@ -420,6 +420,7 @@
good="yes"
fi
fi
+ exit
elif [ "$update_mode" = "yes" ]; then
"./${build}" >"$build_output" 2>&1
build_exit="$?"
