Log an error when an app loads duplicate dex files
Creating a class loader with duplicate dex files in its classpath is most
likely an unintended bug. That leads to rejecting any compiled code and
hurts performance by extracting in memory.
Test: run-test gtest
Bug: 149410951
Change-Id: Ieebb69c6bd03acbe95dd8bedb6101d70390b92d8
diff --git a/runtime/class_loader_context.cc b/runtime/class_loader_context.cc
index 98db62e..d2119db 100644
--- a/runtime/class_loader_context.cc
+++ b/runtime/class_loader_context.cc
@@ -1277,6 +1277,43 @@
(std::string_view(path).substr(/*pos*/ path.size() - suffix.size()) == suffix);
}
+// Returns true if the given dex names are mathing, false otherwise.
+static bool AreDexNameMatching(const std::string& actual_dex_name,
+ const std::string& expected_dex_name) {
+ // Compute the dex location that must be compared.
+ // We shouldn't do a naive comparison `actual_dex_name == expected_dex_name`
+ // because even if they refer to the same file, one could be encoded as a relative location
+ // and the other as an absolute one.
+ bool is_dex_name_absolute = IsAbsoluteLocation(actual_dex_name);
+ bool is_expected_dex_name_absolute = IsAbsoluteLocation(expected_dex_name);
+ bool dex_names_match = false;
+
+ if (is_dex_name_absolute == is_expected_dex_name_absolute) {
+ // If both locations are absolute or relative then compare them as they are.
+ // This is usually the case for: shared libraries and secondary dex files.
+ dex_names_match = (actual_dex_name == expected_dex_name);
+ } else if (is_dex_name_absolute) {
+ // The runtime name is absolute but the compiled name (the expected one) is relative.
+ // This is the case for split apks which depend on base or on other splits.
+ dex_names_match =
+ AbsolutePathHasRelativeSuffix(actual_dex_name, expected_dex_name);
+ } else if (is_expected_dex_name_absolute) {
+ // The runtime name is relative but the compiled name is absolute.
+ // There is no expected use case that would end up here as dex files are always loaded
+ // with their absolute location. However, be tolerant and do the best effort (in case
+ // there are unexpected new use case...).
+ dex_names_match =
+ AbsolutePathHasRelativeSuffix(expected_dex_name, actual_dex_name);
+ } else {
+ // Both locations are relative. In this case there's not much we can be sure about
+ // except that the names are the same. The checksum will ensure that the files are
+ // are same. This should not happen outside testing and manual invocations.
+ dex_names_match = (actual_dex_name == expected_dex_name);
+ }
+
+ return dex_names_match;
+}
+
bool ClassLoaderContext::ClassLoaderInfoMatch(
const ClassLoaderInfo& info,
const ClassLoaderInfo& expected_info,
@@ -1305,37 +1342,7 @@
if (verify_names) {
for (size_t k = 0; k < info.classpath.size(); k++) {
- // Compute the dex location that must be compared.
- // We shouldn't do a naive comparison `info.classpath[k] == expected_info.classpath[k]`
- // because even if they refer to the same file, one could be encoded as a relative location
- // and the other as an absolute one.
- bool is_dex_name_absolute = IsAbsoluteLocation(info.classpath[k]);
- bool is_expected_dex_name_absolute = IsAbsoluteLocation(expected_info.classpath[k]);
- bool dex_names_match = false;
-
-
- if (is_dex_name_absolute == is_expected_dex_name_absolute) {
- // If both locations are absolute or relative then compare them as they are.
- // This is usually the case for: shared libraries and secondary dex files.
- dex_names_match = (info.classpath[k] == expected_info.classpath[k]);
- } else if (is_dex_name_absolute) {
- // The runtime name is absolute but the compiled name (the expected one) is relative.
- // This is the case for split apks which depend on base or on other splits.
- dex_names_match =
- AbsolutePathHasRelativeSuffix(info.classpath[k], expected_info.classpath[k]);
- } else if (is_expected_dex_name_absolute) {
- // The runtime name is relative but the compiled name is absolute.
- // There is no expected use case that would end up here as dex files are always loaded
- // with their absolute location. However, be tolerant and do the best effort (in case
- // there are unexpected new use case...).
- dex_names_match =
- AbsolutePathHasRelativeSuffix(expected_info.classpath[k], info.classpath[k]);
- } else {
- // Both locations are relative. In this case there's not much we can be sure about
- // except that the names are the same. The checksum will ensure that the files are
- // are same. This should not happen outside testing and manual invocations.
- dex_names_match = (info.classpath[k] == expected_info.classpath[k]);
- }
+ bool dex_names_match = AreDexNameMatching(info.classpath[k], expected_info.classpath[k]);
// Compare the locations.
if (!dex_names_match) {
@@ -1393,4 +1400,34 @@
}
}
+std::vector<const DexFile*> ClassLoaderContext::CheckForDuplicateDexFiles(
+ const std::vector<const DexFile*>& dex_files_to_check) {
+ DCHECK(dex_files_open_attempted_);
+ DCHECK(dex_files_open_result_);
+
+ std::vector<const DexFile*> result;
+
+ if (special_shared_library_) {
+ return result;
+ }
+
+ std::vector<ClassLoaderInfo*> work_list;
+ work_list.push_back(class_loader_chain_.get());
+ while (!work_list.empty()) {
+ ClassLoaderInfo* info = work_list.back();
+ work_list.pop_back();
+ for (size_t k = 0; k < info->classpath.size(); k++) {
+ for (const DexFile* dex_file : dex_files_to_check) {
+ if (info->checksums[k] == dex_file->GetLocationChecksum()
+ && AreDexNameMatching(info->classpath[k], dex_file->GetLocation())) {
+ result.push_back(dex_file);
+ }
+ }
+ }
+ AddToWorkList(info, work_list);
+ }
+
+ return result;
+}
+
} // namespace art
diff --git a/runtime/class_loader_context.h b/runtime/class_loader_context.h
index 42ecd3d..31fd092 100644
--- a/runtime/class_loader_context.h
+++ b/runtime/class_loader_context.h
@@ -166,6 +166,11 @@
bool verify_names = true,
bool verify_checksums = true) const;
+ // Checks if any of the given dex files is already loaded in the current class loader context.
+ // Returns the list of duplicate dex files (empty if there are no duplicates).
+ std::vector<const DexFile*> CheckForDuplicateDexFiles(
+ const std::vector<const DexFile*>& dex_files);
+
// Creates the class loader context from the given string.
// The format: ClassLoaderType1[ClasspathElem1:ClasspathElem2...];ClassLoaderType2[...]...
// ClassLoaderType is either "PCL" (PathClassLoader) or "DLC" (DelegateLastClassLoader).
diff --git a/runtime/class_loader_context_test.cc b/runtime/class_loader_context_test.cc
index 4d7e390..e2e6075 100644
--- a/runtime/class_loader_context_test.cc
+++ b/runtime/class_loader_context_test.cc
@@ -1637,4 +1637,23 @@
ClassLoaderContext::VerificationResult::kVerifies);
}
+TEST_F(ClassLoaderContextTest, CheckForDuplicateDexFiles) {
+ jobject class_loader_a = LoadDexInPathClassLoader("Main", nullptr);
+ jobject class_loader_b =
+ LoadDexInInMemoryDexClassLoader("MyClass", class_loader_a);
+
+ std::unique_ptr<ClassLoaderContext> context =
+ CreateContextForClassLoader(class_loader_b);
+
+ std::vector<const DexFile*> result = context->CheckForDuplicateDexFiles(
+ std::vector<const DexFile*>());
+ ASSERT_EQ(0u, result.size());
+
+ std::vector<std::unique_ptr<const DexFile>> dex1 = OpenTestDexFiles("Main");
+ std::vector<const DexFile*> dex1_raw = MakeNonOwningPointerVector(dex1);
+ result = context->CheckForDuplicateDexFiles(dex1_raw);
+ ASSERT_EQ(1u, result.size());
+ ASSERT_EQ(dex1_raw[0], result[0]);
+}
+
} // namespace art
diff --git a/runtime/oat_file_manager.cc b/runtime/oat_file_manager.cc
index 22ebc73..a35f8f4 100644
--- a/runtime/oat_file_manager.cc
+++ b/runtime/oat_file_manager.cc
@@ -412,7 +412,7 @@
// Mismatched context, do the actual collision check.
break;
case ClassLoaderContext::VerificationResult::kVerifies:
- return CheckCollisionResult::kNoCollisions;
+ return CheckCollisionResult::kClassLoaderContextMatches;
}
// The class loader context does not match. Perform a full duplicate classes check.
@@ -434,7 +434,8 @@
if (kEnableAppImage && (!runtime->IsJavaDebuggable() || source_oat_file->IsDebuggable())) {
// If we verified the class loader context (skipping due to the special marker doesn't
// count), then also avoid the collision check.
- bool load_image = check_collision_result == CheckCollisionResult::kNoCollisions;
+ bool load_image = check_collision_result == CheckCollisionResult::kNoCollisions
+ || check_collision_result == CheckCollisionResult::kClassLoaderContextMatches;
// If we skipped the collision check, we need to reverify to be sure its OK to load the
// image.
if (!load_image &&
@@ -655,6 +656,41 @@
Runtime::Current()->GetJit()->RegisterDexFiles(dex_files, class_loader);
}
+ // Verify if any of the dex files being loaded is already in the class path.
+ // If so, report an error with the current stack trace.
+ // Most likely the developer didn't intend to do this because it will waste
+ // performance and memory.
+ // We perform the check only if the class loader context match failed or did
+ // not run (e.g. not that we do not run the check if we don't have an oat file).
+ if (context != nullptr
+ && check_collision_result != CheckCollisionResult::kClassLoaderContextMatches) {
+ std::vector<const DexFile*> already_exists_in_classpath =
+ context->CheckForDuplicateDexFiles(MakeNonOwningPointerVector(dex_files));
+ if (!already_exists_in_classpath.empty()) {
+ std::string duplicates = already_exists_in_classpath[0]->GetLocation();
+ for (size_t i = 1; i < already_exists_in_classpath.size(); i++) {
+ duplicates += "," + already_exists_in_classpath[i]->GetLocation();
+ }
+
+ std::ostringstream out;
+ out << "Trying to load dex files which is already loaded in the same ClassLoader hierarchy.\n"
+ << "This is a strong indication of bad ClassLoader construct which leads to poor "
+ << "performance and wastes memory.\n"
+ << "The list of duplicate dex files is: " << duplicates << "\n"
+ << "The current class loader context is: " << context->EncodeContextForOatFile("") << "\n"
+ << "Java stack trace:\n";
+
+ {
+ ScopedObjectAccess soa(self);
+ self->DumpJavaStack(out);
+ }
+
+ // We log this as an ERROR to stress the fact that this is most likely unintended.
+ // Note that ART cannot do anything about it. It is up to the app to fix their logic.
+ // Here we are trying to give a heads up on why the app might have performance issues.
+ LOG(ERROR) << out.str();
+ }
+ }
return dex_files;
}
diff --git a/runtime/oat_file_manager.h b/runtime/oat_file_manager.h
index 71eab16..9c3a38a 100644
--- a/runtime/oat_file_manager.h
+++ b/runtime/oat_file_manager.h
@@ -147,6 +147,7 @@
kSkippedVerificationDisabled,
kNoCollisions,
kPerformedHasCollisions,
+ kClassLoaderContextMatches
};
std::vector<std::unique_ptr<const DexFile>> OpenDexFilesFromOat_Impl(
diff --git a/test/1002-notify-startup/check b/test/1002-notify-startup/check
new file mode 100644
index 0000000..9d8f464
--- /dev/null
+++ b/test/1002-notify-startup/check
@@ -0,0 +1,20 @@
+#!/bin/bash
+#
+# Copyright (C) 2020 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.
+
+# Oat file manager will complain about duplicate dex files. Ignore.
+sed -e '/.*oat_file_manager.*/d' "$2" > "$2.tmp"
+
+diff --strip-trailing-cr -q "$1" "$2.tmp" >/dev/null
diff --git a/test/146-bad-interface/check b/test/146-bad-interface/check
new file mode 100644
index 0000000..9d8f464
--- /dev/null
+++ b/test/146-bad-interface/check
@@ -0,0 +1,20 @@
+#!/bin/bash
+#
+# Copyright (C) 2020 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.
+
+# Oat file manager will complain about duplicate dex files. Ignore.
+sed -e '/.*oat_file_manager.*/d' "$2" > "$2.tmp"
+
+diff --strip-trailing-cr -q "$1" "$2.tmp" >/dev/null
diff --git a/test/688-shared-library/check b/test/688-shared-library/check
index 0b6c9e4..55847cd 100644
--- a/test/688-shared-library/check
+++ b/test/688-shared-library/check
@@ -16,6 +16,7 @@
# Finalizers of DexFile will complain not being able to close
# the main dex file, as it's still open. That's OK to ignore.
-sed -e '/^E\/System/d' "$2" > "$2.tmp"
+# Oat file manager will also complain about duplicate dex files. Ignore.
+sed -e '/^E\/System/d' "$2" | sed -e '/.*oat_file_manager.*/d' > "$2.tmp"
diff --strip-trailing-cr -q "$1" "$2.tmp" >/dev/null
diff --git a/test/726-load-duplicate-dex-files/check b/test/726-load-duplicate-dex-files/check
new file mode 100644
index 0000000..117bd01
--- /dev/null
+++ b/test/726-load-duplicate-dex-files/check
@@ -0,0 +1,28 @@
+#!/bin/bash
+#
+# Copyright (C) 2020 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.
+
+# Content somewhere inside the output.
+grep 'Trying to load dex files which is already loaded in the same ClassLoader hierarchy.' "$2" >/dev/null
+MSG_FOUND=$?
+
+if [[ "$HEAD" != "$EXPECTED_HEAD" || $MSG_FOUND -ne 0 ]] ; then
+ # Print out the log and return with error.
+ cat "$2"
+ exit 1
+fi
+
+# Success.
+exit 0
diff --git a/test/726-load-duplicate-dex-files/expected.txt b/test/726-load-duplicate-dex-files/expected.txt
new file mode 100644
index 0000000..ef3043a
--- /dev/null
+++ b/test/726-load-duplicate-dex-files/expected.txt
@@ -0,0 +1 @@
+Trying to load dex files which is already loaded in the same ClassLoader hierarchy.
diff --git a/test/726-load-duplicate-dex-files/info.txt b/test/726-load-duplicate-dex-files/info.txt
new file mode 100644
index 0000000..c055acf
--- /dev/null
+++ b/test/726-load-duplicate-dex-files/info.txt
@@ -0,0 +1 @@
+Tests that duplicate dex files get reported.
diff --git a/test/726-load-duplicate-dex-files/src/Main.java b/test/726-load-duplicate-dex-files/src/Main.java
new file mode 100644
index 0000000..f156fb9
--- /dev/null
+++ b/test/726-load-duplicate-dex-files/src/Main.java
@@ -0,0 +1,28 @@
+/*
+ * Copyright 2020 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 dalvik.system.PathClassLoader;
+
+class Main {
+ static final String DEX_FILE = System.getenv("DEX_LOCATION")
+ + "/726-load-duplicate-dex-files.jar";
+ static final String DEX_FILES = DEX_FILE + ":" + DEX_FILE;
+
+ public static void main(String[] args) throws Exception {
+ // Adding duplicate dex files to the classpath will trigger a runtime warning.
+ PathClassLoader p = new PathClassLoader(DEX_FILES, Main.class.getClassLoader());
+ }
+}
