Reland "Remove obsolete code in the verifier."
This reverts commit 7572e76a368e56da139648a5f2e0a92b25a581e2.
Bug: 176960283
Bug: 178731135
Reason for revert: Fixed access checks of mterp on array allocations
Change-Id: I5c2e321e392714bb6921f3b02e32a79ebfd4e22a
diff --git a/runtime/interpreter/mterp/mterp.cc b/runtime/interpreter/mterp/mterp.cc
index 2a1e974..7e31184 100644
--- a/runtime/interpreter/mterp/mterp.cc
+++ b/runtime/interpreter/mterp/mterp.cc
@@ -432,8 +432,24 @@
Thread* self)
REQUIRES_SHARED(Locks::mutator_lock_) {
const Instruction* inst = Instruction::At(dex_pc_ptr);
- return DoFilledNewArray<false, false, false>(inst, *shadow_frame, self,
- shadow_frame->GetResultRegister()) ? 1u : 0u;
+ JValue* result_register = shadow_frame->GetResultRegister();
+ bool res = false;
+ if (shadow_frame->GetMethod()->SkipAccessChecks()) {
+ res = DoFilledNewArray</*is_range=*/false,
+ /*do_access_check=*/false,
+ /*transaction_active=*/false>(inst,
+ *shadow_frame,
+ self,
+ result_register);
+ } else {
+ res = DoFilledNewArray</*is_range=*/false,
+ /*do_access_check=*/true,
+ /*transaction_active=*/false>(inst,
+ *shadow_frame,
+ self,
+ result_register);
+ }
+ return res ? 1u : 0u;
}
extern "C" size_t MterpFilledNewArrayRange(ShadowFrame* shadow_frame,
@@ -441,8 +457,24 @@
Thread* self)
REQUIRES_SHARED(Locks::mutator_lock_) {
const Instruction* inst = Instruction::At(dex_pc_ptr);
- return DoFilledNewArray<true, false, false>(inst, *shadow_frame, self,
- shadow_frame->GetResultRegister()) ? 1u : 0u;
+ JValue* result_register = shadow_frame->GetResultRegister();
+ bool res = false;
+ if (shadow_frame->GetMethod()->SkipAccessChecks()) {
+ res = DoFilledNewArray</*is_range=*/true,
+ /*do_access_check=*/false,
+ /*transaction_active=*/false>(inst,
+ *shadow_frame,
+ self,
+ result_register);
+ } else {
+ res = DoFilledNewArray</*is_range=*/true,
+ /*do_access_check=*/true,
+ /*transaction_active=*/false>(inst,
+ *shadow_frame,
+ self,
+ result_register);
+ }
+ return res ? 1u : 0u;
}
extern "C" size_t MterpNewArray(ShadowFrame* shadow_frame,
@@ -451,11 +483,23 @@
REQUIRES_SHARED(Locks::mutator_lock_) {
const Instruction* inst = Instruction::At(dex_pc_ptr);
int32_t length = shadow_frame->GetVReg(inst->VRegB_22c(inst_data));
- ObjPtr<mirror::Object> obj = AllocArrayFromCode</*kAccessCheck=*/ false>(
- dex::TypeIndex(inst->VRegC_22c()), length, shadow_frame->GetMethod(), self,
- Runtime::Current()->GetHeap()->GetCurrentAllocator());
+ gc::AllocatorType allocator = Runtime::Current()->GetHeap()->GetCurrentAllocator();
+ ObjPtr<mirror::Object> obj;
+ if (shadow_frame->GetMethod()->SkipAccessChecks()) {
+ obj = AllocArrayFromCode</*kAccessCheck=*/ false>(dex::TypeIndex(inst->VRegC_22c()),
+ length,
+ shadow_frame->GetMethod(),
+ self,
+ allocator);
+ } else {
+ obj = AllocArrayFromCode</*kAccessCheck=*/ true>(dex::TypeIndex(inst->VRegC_22c()),
+ length,
+ shadow_frame->GetMethod(),
+ self,
+ allocator);
+ }
if (UNLIKELY(obj == nullptr)) {
- return 0u;
+ return 0u;
}
shadow_frame->SetVRegReference(inst->VRegA_22c(inst_data), obj);
return 1u;
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 9c6798d..017846b 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -5538,27 +5538,6 @@
case VERIFY_ERROR_ACCESS_METHOD:
case VERIFY_ERROR_INSTANTIATION:
case VERIFY_ERROR_CLASS_CHANGE:
- case VERIFY_ERROR_FORCE_INTERPRETER:
- case VERIFY_ERROR_LOCKING:
- if (IsAotMode() || !can_load_classes_) {
- if (error != VERIFY_ERROR_ACCESS_CLASS &&
- error != VERIFY_ERROR_ACCESS_FIELD &&
- error != VERIFY_ERROR_NO_METHOD &&
- error != VERIFY_ERROR_ACCESS_METHOD) {
- // If we're optimistically running verification at compile time, turn NO_xxx,
- // class change and instantiation errors into soft verification errors so that we
- // re-verify at runtime. We may fail to find or to agree on access because of not yet
- // available class loaders, or class loaders that will differ at runtime. In these
- // cases, we don't want to affect the soundness of the code being compiled. Instead, the
- // generated code runs "slow paths" that dynamically perform the verification and cause
- // the behavior to be that akin to an interpreter.
- error = VERIFY_ERROR_BAD_CLASS_SOFT;
- }
- } else {
- // If we fail again at runtime, mark that this instruction would throw and force this
- // method to be executed using the interpreter with checks.
- flags_.have_pending_runtime_throw_failure_ = true;
- }
// How to handle runtime failures for instructions that are not flagged kThrow.
//
// The verifier may fail before we touch any instruction, for the signature of a method. So
@@ -5581,6 +5560,11 @@
}
break;
+ case VERIFY_ERROR_FORCE_INTERPRETER:
+ case VERIFY_ERROR_LOCKING:
+ // This will be reported to the runtime as a soft failure.
+ break;
+
// Indication that verification should be retried at runtime.
case VERIFY_ERROR_BAD_CLASS_SOFT:
if (!allow_soft_failures_) {
@@ -5588,8 +5572,8 @@
}
break;
- // Hard verification failures at compile time will still fail at runtime, so the class is
- // marked as rejected to prevent it from being compiled.
+ // Hard verification failures at compile time will still fail at runtime, so the class is
+ // marked as rejected to prevent it from being compiled.
case VERIFY_ERROR_BAD_CLASS_HARD: {
flags_.have_pending_hard_failure_ = true;
break;
diff --git a/test/816-illegal-new-array/expected-stderr.txt b/test/816-illegal-new-array/expected-stderr.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/816-illegal-new-array/expected-stderr.txt
diff --git a/test/816-illegal-new-array/expected-stdout.txt b/test/816-illegal-new-array/expected-stdout.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/816-illegal-new-array/expected-stdout.txt
diff --git a/test/816-illegal-new-array/info.txt b/test/816-illegal-new-array/info.txt
new file mode 100644
index 0000000..b5eaf3e
--- /dev/null
+++ b/test/816-illegal-new-array/info.txt
@@ -0,0 +1,2 @@
+Regression test for mterp which used to not do access checks on array
+allocation opcodes.
diff --git a/test/816-illegal-new-array/smali/TestCase.smali b/test/816-illegal-new-array/smali/TestCase.smali
new file mode 100644
index 0000000..e0dbbf3
--- /dev/null
+++ b/test/816-illegal-new-array/smali/TestCase.smali
@@ -0,0 +1,47 @@
+# Copyright (C) 2021 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.
+
+.class public LTestCase;
+.super Ljava/lang/Object;
+
+
+.method public constructor <init>()V
+.registers 1
+ invoke-direct {v0}, Ljava/lang/Object;-><init>()V
+ return-void
+.end method
+
+.method public static filledNewArray()[Ljava/lang/Object;
+.registers 1
+ const v0, 0
+ filled-new-array {v0}, [Lp/Foo;
+ move-result-object v0
+ return-object v0
+.end method
+
+.method public static filledNewArrayRange()[Ljava/lang/Object;
+.registers 2
+ const v0, 0
+ const v1, 0
+ filled-new-array/range {v0..v1}, [Lp/Foo;
+ move-result-object v0
+ return-object v0
+.end method
+
+.method public static newArray()V
+.registers 1
+ const v0, 0
+ new-array v0, v0, [Lp/Foo;
+ return-void
+.end method
diff --git a/test/816-illegal-new-array/src/Main.java b/test/816-illegal-new-array/src/Main.java
new file mode 100644
index 0000000..2d48c12
--- /dev/null
+++ b/test/816-illegal-new-array/src/Main.java
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2021 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.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+public class Main {
+
+ public static void main(String[] args) throws Exception {
+ Class<?> c = Class.forName("TestCase");
+ Method m = c.getMethod("filledNewArray");
+ try {
+ m.invoke(null);
+ throw new Error("Expected IllegalAccessError");
+ } catch (InvocationTargetException e) {
+ if (!(e.getCause() instanceof IllegalAccessError)) {
+ throw new Error("Expected IllegalAccessError, got " + e.getCause());
+ }
+ }
+
+ m = c.getMethod("filledNewArrayRange");
+ try {
+ m.invoke(null);
+ throw new Error("Expected IllegalAccessError");
+ } catch (InvocationTargetException e) {
+ if (!(e.getCause() instanceof IllegalAccessError)) {
+ throw new Error("Expected IllegalAccessError, got " + e.getCause());
+ }
+ }
+
+ m = c.getMethod("newArray");
+ try {
+ m.invoke(null);
+ throw new Error("Expected IllegalAccessError");
+ } catch (InvocationTargetException e) {
+ if (!(e.getCause() instanceof IllegalAccessError)) {
+ throw new Error("Expected IllegalAccessError, got " + e.getCause());
+ }
+ }
+ }
+}
+
diff --git a/test/816-illegal-new-array/src/p/Foo.java b/test/816-illegal-new-array/src/p/Foo.java
new file mode 100644
index 0000000..8d018e0
--- /dev/null
+++ b/test/816-illegal-new-array/src/p/Foo.java
@@ -0,0 +1,20 @@
+/*
+ * Copyright (C) 2021 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 p;
+
+class Foo {
+}
diff --git a/test/knownfailures.json b/test/knownfailures.json
index 6df3ae7..507ebb8 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -1010,6 +1010,7 @@
"801-VoidCheckCast",
"802-deoptimization",
"804-class-extends-itself",
+ "816-illegal-new-array",
"900-hello-plugin",
"901-hello-ti-agent",
"903-hello-tagging",