Fix incorrect dex-cache clearing with structural redefinition
We were incorrectly clearing the dex-cache in some cases after
structural redefinition. Leaving some entries present that should
have been deleted. This could cause subsequently run code to
resolve static methods or fields incorrectly.
Test: ./test.py --host
Bug: 161846143
Change-Id: Idf23fc21f2e396c347861afd070363c509108096
Merged-In: Idf23fc21f2e396c347861afd070363c509108096
(cherry picked from commit bf6498e3d94cde2abbf99788e68e44f48280846a)
diff --git a/openjdkjvmti/ti_redefine.cc b/openjdkjvmti/ti_redefine.cc
index c457dba..049e0cf 100644
--- a/openjdkjvmti/ti_redefine.cc
+++ b/openjdkjvmti/ti_redefine.cc
@@ -2761,8 +2761,6 @@
art::Locks::mutator_lock_->AssertExclusiveHeld(driver_->self_);
art::ClassLinker* cl = driver_->runtime_->GetClassLinker();
art::ScopedAssertNoThreadSuspension sants(__FUNCTION__);
- art::ObjPtr<art::mirror::Class> orig(holder.GetMirrorClass());
- art::ObjPtr<art::mirror::Class> replacement(holder.GetNewClassObject());
art::ObjPtr<art::mirror::ObjectArray<art::mirror::Class>> new_classes(holder.GetNewClasses());
art::ObjPtr<art::mirror::ObjectArray<art::mirror::Class>> old_classes(holder.GetOldClasses());
// Collect mappings from old to new fields/methods
@@ -2773,8 +2771,6 @@
holder.GetNewInstanceObjects());
art::ObjPtr<art::mirror::ObjectArray<art::mirror::Object>> old_instances(
holder.GetOldInstanceObjects());
- CHECK(!orig.IsNull());
- CHECK(!replacement.IsNull());
// Once we do the ReplaceReferences old_classes will have the new_classes in it. We want to keep
// ahold of the old classes so copy them now.
std::vector<art::ObjPtr<art::mirror::Class>> old_classes_vec(old_classes->Iterate().begin(),
@@ -2848,16 +2844,26 @@
}
}
// We can only shadow things from our superclasses
- if (LIKELY(!field_or_method->GetDeclaringClass()->IsAssignableFrom(orig))) {
+ auto orig_classes_iter = old_classes->Iterate();
+ auto replacement_classes_iter = new_classes->Iterate();
+ art::ObjPtr<art::mirror::Class> f_or_m_class = field_or_method->GetDeclaringClass();
+ if (LIKELY(!f_or_m_class->IsAssignableFrom(holder.GetMirrorClass()) &&
+ std::find(orig_classes_iter.begin(), orig_classes_iter.end(), f_or_m_class) ==
+ orig_classes_iter.end())) {
return false;
}
if constexpr (is_method) {
- auto direct_methods = replacement->GetDirectMethods(art::kRuntimePointerSize);
- return std::find_if(direct_methods.begin(),
- direct_methods.end(),
- [&](art::ArtMethod& m) REQUIRES(art::Locks::mutator_lock_) {
- return UNLIKELY(m.HasSameNameAndSignature(field_or_method));
- }) != direct_methods.end();
+ return std::any_of(
+ replacement_classes_iter.begin(),
+ replacement_classes_iter.end(),
+ [&](art::ObjPtr<art::mirror::Class> cand) REQUIRES(art::Locks::mutator_lock_) {
+ auto direct_methods = cand->GetDirectMethods(art::kRuntimePointerSize);
+ return std::find_if(direct_methods.begin(),
+ direct_methods.end(),
+ [&](art::ArtMethod& m) REQUIRES(art::Locks::mutator_lock_) {
+ return UNLIKELY(m.HasSameNameAndSignature(field_or_method));
+ }) != direct_methods.end();
+ });
} else {
auto pred = [&](art::ArtField& f) REQUIRES(art::Locks::mutator_lock_) {
return std::string_view(f.GetName()) == std::string_view(field_or_method->GetName()) &&
@@ -2865,11 +2871,21 @@
std::string_view(field_or_method->GetTypeDescriptor());
};
if (field_or_method->IsStatic()) {
- auto sfields = replacement->GetSFields();
- return std::find_if(sfields.begin(), sfields.end(), pred) != sfields.end();
+ return std::any_of(
+ replacement_classes_iter.begin(),
+ replacement_classes_iter.end(),
+ [&](art::ObjPtr<art::mirror::Class> cand) REQUIRES(art::Locks::mutator_lock_) {
+ auto sfields = cand->GetSFields();
+ return std::find_if(sfields.begin(), sfields.end(), pred) != sfields.end();
+ });
} else {
- auto ifields = replacement->GetIFields();
- return std::find_if(ifields.begin(), ifields.end(), pred) != ifields.end();
+ return std::any_of(
+ replacement_classes_iter.begin(),
+ replacement_classes_iter.end(),
+ [&](art::ObjPtr<art::mirror::Class> cand) REQUIRES(art::Locks::mutator_lock_) {
+ auto ifields = cand->GetIFields();
+ return std::find_if(ifields.begin(), ifields.end(), pred) != ifields.end();
+ });
}
}
};
@@ -2879,28 +2895,28 @@
[&](art::ArtField* f, const auto& info) REQUIRES(art::Locks::mutator_lock_) {
DCHECK(f != nullptr) << info;
auto it = field_map.find(f);
- if (it != field_map.end()) {
+ if (UNLIKELY(could_change_resolution_of(f, info))) {
+ // Dex-cache Resolution might change. Just clear the resolved value.
+ VLOG(plugin) << "Clearing resolution " << info << " for (field) " << f->PrettyField();
+ return static_cast<art::ArtField*>(nullptr);
+ } else if (it != field_map.end()) {
VLOG(plugin) << "Updating " << info << " object for (field) "
<< it->second->PrettyField();
return it->second;
- } else if (UNLIKELY(could_change_resolution_of(f, info))) {
- // Resolution might change. Just clear the resolved value.
- VLOG(plugin) << "Clearing resolution " << info << " for (field) " << f->PrettyField();
- return static_cast<art::ArtField*>(nullptr);
}
return f;
},
[&](art::ArtMethod* m, const auto& info) REQUIRES(art::Locks::mutator_lock_) {
DCHECK(m != nullptr) << info;
auto it = method_map.find(m);
- if (it != method_map.end()) {
+ if (UNLIKELY(could_change_resolution_of(m, info))) {
+ // Dex-cache Resolution might change. Just clear the resolved value.
+ VLOG(plugin) << "Clearing resolution " << info << " for (method) " << m->PrettyMethod();
+ return static_cast<art::ArtMethod*>(nullptr);
+ } else if (it != method_map.end()) {
VLOG(plugin) << "Updating " << info << " object for (method) "
<< it->second->PrettyMethod();
return it->second;
- } else if (UNLIKELY(could_change_resolution_of(m, info))) {
- // Resolution might change. Just clear the resolved value.
- VLOG(plugin) << "Clearing resolution " << info << " for (method) " << m->PrettyMethod();
- return static_cast<art::ArtMethod*>(nullptr);
}
return m;
});
@@ -2955,18 +2971,25 @@
if (art::kIsDebugBuild) {
// Just make sure we didn't screw up any of the now obsolete methods or fields. We need their
// declaring-class to still be the obolete class
- orig->VisitMethods([&](art::ArtMethod* method) REQUIRES_SHARED(art::Locks::mutator_lock_) {
- if (method->IsCopied()) {
- // Copied methods have interfaces as their declaring class.
- return;
- }
- DCHECK_EQ(method->GetDeclaringClass(), orig) << method->GetDeclaringClass()->PrettyClass()
- << " vs " << orig->PrettyClass();
- }, art::kRuntimePointerSize);
- orig->VisitFields([&](art::ArtField* field) REQUIRES_SHARED(art::Locks::mutator_lock_) {
- DCHECK_EQ(field->GetDeclaringClass(), orig) << field->GetDeclaringClass()->PrettyClass()
- << " vs " << orig->PrettyClass();
- });
+ std::for_each(
+ old_classes_vec.cbegin(),
+ old_classes_vec.cend(),
+ [](art::ObjPtr<art::mirror::Class> orig) REQUIRES_SHARED(art::Locks::mutator_lock_) {
+ orig->VisitMethods(
+ [&](art::ArtMethod* method) REQUIRES_SHARED(art::Locks::mutator_lock_) {
+ if (method->IsCopied()) {
+ // Copied methods have interfaces as their declaring class.
+ return;
+ }
+ DCHECK_EQ(method->GetDeclaringClass(), orig)
+ << method->GetDeclaringClass()->PrettyClass() << " vs " << orig->PrettyClass();
+ },
+ art::kRuntimePointerSize);
+ orig->VisitFields([&](art::ArtField* field) REQUIRES_SHARED(art::Locks::mutator_lock_) {
+ DCHECK_EQ(field->GetDeclaringClass(), orig)
+ << field->GetDeclaringClass()->PrettyClass() << " vs " << orig->PrettyClass();
+ });
+ });
}
}
diff --git a/test/2036-structural-subclass-shadow/expected.txt b/test/2036-structural-subclass-shadow/expected.txt
new file mode 100644
index 0000000..7fb86e3
--- /dev/null
+++ b/test/2036-structural-subclass-shadow/expected.txt
@@ -0,0 +1,8 @@
+value-getter is 42
+value is 42
+static-call is 1337
+static-value is 3.14
+value-getter is 0
+value is 0
+static-call is -559038737
+static-value is 0.0
diff --git a/test/2036-structural-subclass-shadow/info.txt b/test/2036-structural-subclass-shadow/info.txt
new file mode 100644
index 0000000..317dbf8
--- /dev/null
+++ b/test/2036-structural-subclass-shadow/info.txt
@@ -0,0 +1,5 @@
+Tests dex-cache invalidation with structural redefinition.
+
+Regression test for b/161846143. Structural redefinition was incorrectly
+failing to invalidate some dex-cache entries causing incorrect field/method
+resolution in limited cases.
diff --git a/test/2036-structural-subclass-shadow/run b/test/2036-structural-subclass-shadow/run
new file mode 100755
index 0000000..ff387ff
--- /dev/null
+++ b/test/2036-structural-subclass-shadow/run
@@ -0,0 +1,17 @@
+#!/bin/bash
+#
+# 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.
+
+./default-run "$@" --jvmti --runtime-option -Xopaque-jni-ids:true
diff --git a/test/2036-structural-subclass-shadow/src-art/Main.java b/test/2036-structural-subclass-shadow/src-art/Main.java
new file mode 100644
index 0000000..d707701
--- /dev/null
+++ b/test/2036-structural-subclass-shadow/src-art/Main.java
@@ -0,0 +1,21 @@
+/*
+ * 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.
+ */
+
+public class Main {
+ public static void main(String[] args) throws Exception {
+ art.Test2036.run();
+ }
+}
diff --git a/test/2036-structural-subclass-shadow/src-art/art/Redefinition.java b/test/2036-structural-subclass-shadow/src-art/art/Redefinition.java
new file mode 120000
index 0000000..81eaf31
--- /dev/null
+++ b/test/2036-structural-subclass-shadow/src-art/art/Redefinition.java
@@ -0,0 +1 @@
+../../../jvmti-common/Redefinition.java
\ No newline at end of file
diff --git a/test/2036-structural-subclass-shadow/src-art/art/Test2036.java b/test/2036-structural-subclass-shadow/src-art/art/Test2036.java
new file mode 100644
index 0000000..8ca8e2a
--- /dev/null
+++ b/test/2036-structural-subclass-shadow/src-art/art/Test2036.java
@@ -0,0 +1,136 @@
+/*
+ * 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.
+ */
+
+package art;
+
+import java.util.Base64;
+
+public class Test2036 {
+ public static class Transform {
+ public Transform() {}
+ }
+
+ public static class SubTransform extends Transform {
+ public SubTransform() {}
+
+ public static int FooBar() {
+ return 1337;
+ }
+
+ public int foo = 42;
+ public static double STATIC_FOO = 3.14d;
+ }
+
+ public static class SubSubTransform extends SubTransform {
+ public SubSubTransform() {}
+ }
+
+ public static class SubSubSubTransform extends SubSubTransform {
+ public SubSubSubTransform() {}
+
+ public int getFoo() {
+ return this.foo;
+ }
+ }
+ /*
+ * base64 encoded class/dex file for
+ * Base64 generated using:
+ * % javac Test2036.java
+ * % d8 Test2035\$Transform.class
+ * % base64 classes.dex| sed 's:^:":' | sed 's:$:" +:'
+ *
+ * package art;
+ * public static class Transform {
+ * public Transform() {}
+ * public int bar;
+ * }
+ */
+ private static final byte[] DEX_BYTES =
+ Base64.getDecoder()
+ .decode(
+ "ZGV4CjAzNQAZlSz5hewOnpRnplr0AnEqh8+AKNc2qhp0AwAAcAAAAHhWNBIAAAAAAAAAALwCAAAP"
+ + "AAAAcAAAAAcAAACsAAAAAQAAAMgAAAABAAAA1AAAAAIAAADcAAAAAQAAAOwAAABoAgAADAEAACgB"
+ + "AAAwAQAAMwEAAE0BAABdAQAAgQEAAKEBAAC1AQAAxAEAAM8BAADSAQAA3wEAAOQBAADqAQAA8QEA"
+ + "AAEAAAACAAAAAwAAAAQAAAAFAAAABgAAAAkAAAAJAAAABgAAAAAAAAABAAAACwAAAAEAAAAAAAAA"
+ + "BQAAAAAAAAABAAAAAQAAAAUAAAAAAAAABwAAAKwCAACOAgAAAAAAAAEAAQABAAAAJAEAAAQAAABw"
+ + "EAEAAAAOABgADgAGPGluaXQ+AAFJABhMYXJ0L1Rlc3QyMDM2JFRyYW5zZm9ybTsADkxhcnQvVGVz"
+ + "dDIwMzY7ACJMZGFsdmlrL2Fubm90YXRpb24vRW5jbG9zaW5nQ2xhc3M7AB5MZGFsdmlrL2Fubm90"
+ + "YXRpb24vSW5uZXJDbGFzczsAEkxqYXZhL2xhbmcvT2JqZWN0OwANVGVzdDIwMzYuamF2YQAJVHJh"
+ + "bnNmb3JtAAFWAAthY2Nlc3NGbGFncwADYmFyAARuYW1lAAV2YWx1ZQCLAX5+RDh7ImNvbXBpbGF0"
+ + "aW9uLW1vZGUiOiJkZWJ1ZyIsImhhcy1jaGVja3N1bXMiOmZhbHNlLCJtaW4tYXBpIjoxLCJzaGEt"
+ + "MSI6IjJkMDE0MjEyNjVlMjUxMWM5MGUxZTY2NTU0NWEzMzliM2M5OWMwYWYiLCJ2ZXJzaW9uIjoi"
+ + "Mi4yLjMtZGV2In0AAgMBDRgCAgQCCgQJDBcIAAEBAAABAIGABIwCAAAAAAAAAgAAAH8CAACFAgAA"
+ + "oAIAAAAAAAAAAAAAAAAAAA8AAAAAAAAAAQAAAAAAAAABAAAADwAAAHAAAAACAAAABwAAAKwAAAAD"
+ + "AAAAAQAAAMgAAAAEAAAAAQAAANQAAAAFAAAAAgAAANwAAAAGAAAAAQAAAOwAAAABIAAAAQAAAAwB"
+ + "AAADIAAAAQAAACQBAAACIAAADwAAACgBAAAEIAAAAgAAAH8CAAAAIAAAAQAAAI4CAAADEAAAAgAA"
+ + "AJwCAAAGIAAAAQAAAKwCAAAAEAAAAQAAALwCAAA=");
+ /*
+ * base64 encoded class/dex file for
+ * Base64 generated using:
+ * % javac Test2036.java
+ * % d8 Test2035\$SubSubTransform.class
+ * % base64 classes.dex| sed 's:^:":' | sed 's:$:" +:'
+ *
+ * package art;
+ * public static class SubSubTransform extends SubTransform {
+ * public SubTransform() {}
+ * public static int FooBar() { return 0xDEADBEEF; }
+ * public int foo;
+ * public static double STATIC_FOO;
+ * }
+ */
+ private static final byte[] DEX_BYTES_SUB_SUB =
+ Base64.getDecoder()
+ .decode(
+ "ZGV4CjAzNQATpeOoP5d8wSzfcaS99KwXbb1DsEne1tDsAwAAcAAAAHhWNBIAAAAAAAAAADQDAAAS"
+ + "AAAAcAAAAAgAAAC4AAAAAgAAANgAAAACAAAA8AAAAAMAAAAAAQAAAQAAABgBAAC0AgAAOAEAAHAB"
+ + "AAB4AQAAewEAAIMBAACGAQAApgEAAMMBAADTAQAA9wEAABcCAAAjAgAANAIAAEMCAABGAgAAUwIA"
+ + "AFgCAABeAgAAZQIAAAEAAAADAAAABAAAAAUAAAAGAAAABwAAAAgAAAAMAAAAAwAAAAEAAAAAAAAA"
+ + "DAAAAAcAAAAAAAAAAgAAAAkAAAACAAEADgAAAAIAAQAAAAAAAgAAAAIAAAADAAEAAAAAAAIAAAAB"
+ + "AAAAAwAAAAAAAAALAAAAJAMAAAIDAAAAAAAAAQAAAAAAAABoAQAABAAAABQA776t3g8AAQABAAEA"
+ + "AABsAQAABAAAAHAQAgAAAA4AIQAOACAADgAGPGluaXQ+AAFEAAZGb29CYXIAAUkAHkxhcnQvVGVz"
+ + "dDIwMzYkU3ViU3ViVHJhbnNmb3JtOwAbTGFydC9UZXN0MjAzNiRTdWJUcmFuc2Zvcm07AA5MYXJ0"
+ + "L1Rlc3QyMDM2OwAiTGRhbHZpay9hbm5vdGF0aW9uL0VuY2xvc2luZ0NsYXNzOwAeTGRhbHZpay9h"
+ + "bm5vdGF0aW9uL0lubmVyQ2xhc3M7AApTVEFUSUNfRk9PAA9TdWJTdWJUcmFuc2Zvcm0ADVRlc3Qy"
+ + "MDM2LmphdmEAAVYAC2FjY2Vzc0ZsYWdzAANmb28ABG5hbWUABXZhbHVlAIsBfn5EOHsiY29tcGls"
+ + "YXRpb24tbW9kZSI6ImRlYnVnIiwiaGFzLWNoZWNrc3VtcyI6ZmFsc2UsIm1pbi1hcGkiOjEsInNo"
+ + "YS0xIjoiMmQwMTQyMTI2NWUyNTExYzkwZTFlNjY1NTQ1YTMzOWIzYzk5YzBhZiIsInZlcnNpb24i"
+ + "OiIyLjIuMy1kZXYifQACBQEQGAQCBgINBAkPFwoBAQIAAAkBAQCBgATQAgEJuAIAAAAAAgAAAPMC"
+ + "AAD5AgAAGAMAAAAAAAAAAAAAAAAAAA8AAAAAAAAAAQAAAAAAAAABAAAAEgAAAHAAAAACAAAACAAA"
+ + "ALgAAAADAAAAAgAAANgAAAAEAAAAAgAAAPAAAAAFAAAAAwAAAAABAAAGAAAAAQAAABgBAAABIAAA"
+ + "AgAAADgBAAADIAAAAgAAAGgBAAACIAAAEgAAAHABAAAEIAAAAgAAAPMCAAAAIAAAAQAAAAIDAAAD"
+ + "EAAAAgAAABQDAAAGIAAAAQAAACQDAAAAEAAAAQAAADQDAAA=");
+
+ public static void run() throws Exception {
+ Redefinition.setTestConfiguration(Redefinition.Config.COMMON_REDEFINE);
+ doTest();
+ }
+
+ public static void doTest() throws Exception {
+ SubSubSubTransform t = new SubSubSubTransform();
+ System.out.println("value-getter is " + t.getFoo());
+ System.out.println("value is " + t.foo);
+ System.out.println("static-call is " + SubSubSubTransform.FooBar());
+ System.out.println("static-value is " + SubSubSubTransform.STATIC_FOO);
+ Redefinition.doMultiStructuralClassRedefinition(
+ new Redefinition.DexOnlyClassDefinition(Transform.class, DEX_BYTES),
+ new Redefinition.DexOnlyClassDefinition(SubSubTransform.class, DEX_BYTES_SUB_SUB));
+ System.out.println("value-getter is " + t.getFoo());
+ System.out.println("value is " + t.foo);
+ System.out.println("static-call is " + SubSubSubTransform.FooBar());
+ System.out.println("static-value is " + SubSubSubTransform.STATIC_FOO);
+ }
+}
diff --git a/test/2036-structural-subclass-shadow/src/Main.java b/test/2036-structural-subclass-shadow/src/Main.java
new file mode 100644
index 0000000..e0477b0
--- /dev/null
+++ b/test/2036-structural-subclass-shadow/src/Main.java
@@ -0,0 +1,21 @@
+/*
+ * 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.
+ */
+
+public class Main {
+ public static void main(String[] args) throws Exception {
+ System.out.println("FAIL: Test is only for art!");
+ }
+}
diff --git a/test/knownfailures.json b/test/knownfailures.json
index 398f123..3c72608 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -1222,7 +1222,8 @@
"2005-pause-all-redefine-multithreaded",
"2006-virtual-structural-finalizing",
"2007-virtual-structural-finalizable",
- "2035-structural-native-method"],
+ "2035-structural-native-method",
+ "2036-structural-subclass-shadow"],
"variant": "jvm",
"description": ["Doesn't run on RI."]
},