From 3622c42c1539cf4c8f1109cb49296f583080a91d Mon Sep 17 00:00:00 2001 From: Goetz Lindenmaier Date: Mon, 10 Oct 2022 09:17:44 +0000 Subject: [PATCH] 8293044: C1: Missing access check on non-accessible class Reviewed-by: mdoerr Backport-of: 005b49bb78a468d4e372e6f5fa48bb0db4fd73c2 --- src/hotspot/share/c1/c1_GraphBuilder.cpp | 15 ++-- src/hotspot/share/c1/c1_Runtime1.cpp | 42 +++++++-- src/hotspot/share/ci/ciStreams.cpp | 22 ++++- src/hotspot/share/ci/ciStreams.hpp | 1 + src/hotspot/share/interpreter/bytecode.hpp | 6 +- .../jtreg/compiler/c1/KlassAccessCheck.jasm | 86 +++++++++++++++++++ .../c1/KlassAccessCheckPackagePrivate.jasm | 29 +++++++ .../compiler/c1/KlassAccessCheckTest.java | 61 +++++++++++++ 8 files changed, 244 insertions(+), 18 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/c1/KlassAccessCheck.jasm create mode 100644 test/hotspot/jtreg/compiler/c1/KlassAccessCheckPackagePrivate.jasm create mode 100644 test/hotspot/jtreg/compiler/c1/KlassAccessCheckTest.java diff --git a/src/hotspot/share/c1/c1_GraphBuilder.cpp b/src/hotspot/share/c1/c1_GraphBuilder.cpp index 7ba69db8eb5..2b3ac117b7d 100644 --- a/src/hotspot/share/c1/c1_GraphBuilder.cpp +++ b/src/hotspot/share/c1/c1_GraphBuilder.cpp @@ -2161,8 +2161,7 @@ void GraphBuilder::invoke(Bytecodes::Code code) { void GraphBuilder::new_instance(int klass_index) { ValueStack* state_before = copy_state_exhandling(); - bool will_link; - ciKlass* klass = stream()->get_klass(will_link); + ciKlass* klass = stream()->get_klass(); assert(klass->is_instance_klass(), "must be an instance klass"); NewInstance* new_instance = new NewInstance(klass->as_instance_klass(), state_before, stream()->is_unresolved_klass()); _memory->new_instance(new_instance); @@ -2177,8 +2176,7 @@ void GraphBuilder::new_type_array() { void GraphBuilder::new_object_array() { - bool will_link; - ciKlass* klass = stream()->get_klass(will_link); + ciKlass* klass = stream()->get_klass(); ValueStack* state_before = !klass->is_loaded() || PatchALot ? copy_state_before() : copy_state_exhandling(); NewArray* n = new NewObjectArray(klass, ipop(), state_before); apush(append_split(n)); @@ -2203,8 +2201,7 @@ bool GraphBuilder::direct_compare(ciKlass* k) { void GraphBuilder::check_cast(int klass_index) { - bool will_link; - ciKlass* klass = stream()->get_klass(will_link); + ciKlass* klass = stream()->get_klass(); ValueStack* state_before = !klass->is_loaded() || PatchALot ? copy_state_before() : copy_state_for_exception(); CheckCast* c = new CheckCast(klass, apop(), state_before); apush(append_split(c)); @@ -2224,8 +2221,7 @@ void GraphBuilder::check_cast(int klass_index) { void GraphBuilder::instance_of(int klass_index) { - bool will_link; - ciKlass* klass = stream()->get_klass(will_link); + ciKlass* klass = stream()->get_klass(); ValueStack* state_before = !klass->is_loaded() || PatchALot ? copy_state_before() : copy_state_exhandling(); InstanceOf* i = new InstanceOf(klass, apop(), state_before); ipush(append_split(i)); @@ -2259,8 +2255,7 @@ void GraphBuilder::monitorexit(Value x, int bci) { void GraphBuilder::new_multi_array(int dimensions) { - bool will_link; - ciKlass* klass = stream()->get_klass(will_link); + ciKlass* klass = stream()->get_klass(); ValueStack* state_before = !klass->is_loaded() || PatchALot ? copy_state_before() : copy_state_exhandling(); Values* dims = new Values(dimensions, dimensions, NULL); diff --git a/src/hotspot/share/c1/c1_Runtime1.cpp b/src/hotspot/share/c1/c1_Runtime1.cpp index 807cc407e23..06f18a85efb 100644 --- a/src/hotspot/share/c1/c1_Runtime1.cpp +++ b/src/hotspot/share/c1/c1_Runtime1.cpp @@ -1237,6 +1237,37 @@ JRT_END #else // DEOPTIMIZE_WHEN_PATCHING +static bool is_patching_needed(JavaThread* current, Runtime1::StubID stub_id) { + if (stub_id == Runtime1::load_klass_patching_id || + stub_id == Runtime1::load_mirror_patching_id) { + // last java frame on stack + vframeStream vfst(current, true); + assert(!vfst.at_end(), "Java frame must exist"); + + methodHandle caller_method(current, vfst.method()); + int bci = vfst.bci(); + Bytecodes::Code code = caller_method()->java_code_at(bci); + + switch (code) { + case Bytecodes::_new: + case Bytecodes::_anewarray: + case Bytecodes::_multianewarray: + case Bytecodes::_instanceof: + case Bytecodes::_checkcast: { + Bytecode bc(caller_method(), caller_method->bcp_from(bci)); + constantTag tag = caller_method->constants()->tag_at(bc.get_index_u2(code)); + if (tag.is_unresolved_klass_in_error()) { + return false; // throws resolution error + } + break; + } + + default: break; + } + } + return true; +} + JRT_ENTRY(void, Runtime1::patch_code(JavaThread* thread, Runtime1::StubID stub_id )) RegisterMap reg_map(thread, false); @@ -1248,11 +1279,12 @@ JRT_ENTRY(void, Runtime1::patch_code(JavaThread* thread, Runtime1::StubID stub_i frame runtime_frame = thread->last_frame(); frame caller_frame = runtime_frame.sender(®_map); - // It's possible the nmethod was invalidated in the last - // safepoint, but if it's still alive then make it not_entrant. - nmethod* nm = CodeCache::find_nmethod(caller_frame.pc()); - if (nm != NULL) { - nm->make_not_entrant(); + if (is_patching_needed(thread, stub_id)) { + // Make sure the nmethod is invalidated, i.e. made not entrant. + nmethod* nm = CodeCache::find_nmethod(caller_frame.pc()); + if (nm != NULL) { + nm->make_not_entrant(); + } } Deoptimization::deoptimize_frame(thread, caller_frame.id()); diff --git a/src/hotspot/share/ci/ciStreams.cpp b/src/hotspot/share/ci/ciStreams.cpp index 30ad863c375..06bbd00ebba 100644 --- a/src/hotspot/share/ci/ciStreams.cpp +++ b/src/hotspot/share/ci/ciStreams.cpp @@ -23,9 +23,10 @@ */ #include "precompiled.hpp" -#include "ci/ciCallSite.hpp" #include "ci/ciConstant.hpp" #include "ci/ciField.hpp" +#include "ci/ciKlass.hpp" +#include "ci/ciObjArrayKlass.hpp" #include "ci/ciStreams.hpp" #include "ci/ciUtilities.inline.hpp" #include "runtime/handles.inline.hpp" @@ -190,6 +191,25 @@ ciKlass* ciBytecodeStream::get_klass(bool& will_link) { return CURRENT_ENV->get_klass_by_index(cpool, get_klass_index(), will_link, _holder); } +// ciBytecodeStream::get_klass +// +// If this bytecode is a new, newarray, multianewarray, instanceof, +// or checkcast, get the referenced klass. Retuns an unloaded ciKlass +// if the referenced klass is not accessible. +ciKlass* ciBytecodeStream::get_klass() { + bool will_link; + ciKlass* klass = get_klass(will_link); + if (!will_link && klass->is_loaded()) { // klass not accessible + if (klass->is_array_klass()) { + assert(!klass->is_type_array_klass(), ""); + klass = ciEnv::unloaded_ciobjarrayklass(); + } else { + klass = ciEnv::unloaded_ciinstance_klass(); + } + } + return klass; +} + // ------------------------------------------------------------------ // ciBytecodeStream::get_constant_raw_index // diff --git a/src/hotspot/share/ci/ciStreams.hpp b/src/hotspot/share/ci/ciStreams.hpp index 07e573b5963..6d4de53d35b 100644 --- a/src/hotspot/share/ci/ciStreams.hpp +++ b/src/hotspot/share/ci/ciStreams.hpp @@ -233,6 +233,7 @@ class ciBytecodeStream : StackObj { // If this bytecode is a new, newarray, multianewarray, instanceof, // or checkcast, get the referenced klass. + ciKlass* get_klass(); ciKlass* get_klass(bool& will_link); int get_klass_index() const; diff --git a/src/hotspot/share/interpreter/bytecode.hpp b/src/hotspot/share/interpreter/bytecode.hpp index befdd889d22..552afdf81f2 100644 --- a/src/hotspot/share/interpreter/bytecode.hpp +++ b/src/hotspot/share/interpreter/bytecode.hpp @@ -77,9 +77,11 @@ class Bytecode: public StackObj { int get_index_u2(Bytecodes::Code bc, bool is_wide = false) const { assert_same_format_as(bc, is_wide); assert_index_size(2, bc, is_wide); address p = addr_at(is_wide ? 2 : 1); - if (can_use_native_byte_order(bc, is_wide)) + if (can_use_native_byte_order(bc, is_wide)) { return Bytes::get_native_u2(p); - else return Bytes::get_Java_u2(p); + } else { + return Bytes::get_Java_u2(p); + } } int get_index_u1_cpcache(Bytecodes::Code bc) const { assert_same_format_as(bc); assert_index_size(1, bc); diff --git a/test/hotspot/jtreg/compiler/c1/KlassAccessCheck.jasm b/test/hotspot/jtreg/compiler/c1/KlassAccessCheck.jasm new file mode 100644 index 00000000000..41d9e1998fc --- /dev/null +++ b/test/hotspot/jtreg/compiler/c1/KlassAccessCheck.jasm @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +package compiler/c1; + +super public class KlassAccessCheck + version 51:0 +{ + + public static Method testNewInstance:"()V" + stack 2 locals 0 + { + new class compiler/c1/types/PackagePrivateClass; + return; + } + + + public static Method testNewArray:"()[Ljava/lang/Object;" + stack 1 locals 0 + { + iconst_1; + anewarray class compiler/c1/types/PackagePrivateClass; + areturn; + } + + public static Method testMultiNewArray:"()[[Ljava/lang/Object;" + stack 2 locals 1 + { + iconst_1; + iconst_1; + multianewarray class "[[Lcompiler/c1/types/PackagePrivateClass;", 2; + areturn; + } + + public static Method testCheckCast:"(Ljava/lang/Object;)Ljava/lang/Object;" + stack 1 locals 2 + { + aload_0; + checkcast class compiler/c1/types/PackagePrivateClass; + areturn; + } + + public static Method testCheckCastArr:"(Ljava/lang/Object;)Ljava/lang/Object;" + stack 1 locals 2 + { + aload_0; + checkcast class "[Lcompiler/c1/types/PackagePrivateClass;"; + areturn; + } + + public static Method testInstanceOf:"(Ljava/lang/Object;)Z" + stack 1 locals 2 + { + aload_0; + instanceof class compiler/c1/types/PackagePrivateClass; + ireturn; + } + + public static Method testInstanceOfArr:"(Ljava/lang/Object;)Z" + stack 1 locals 2 + { + aload_0; + instanceof class "[Lcompiler/c1/types/PackagePrivateClass;"; + ireturn; + } +} // end Class KlassAccessCheck diff --git a/test/hotspot/jtreg/compiler/c1/KlassAccessCheckPackagePrivate.jasm b/test/hotspot/jtreg/compiler/c1/KlassAccessCheckPackagePrivate.jasm new file mode 100644 index 00000000000..9c0b46c74fd --- /dev/null +++ b/test/hotspot/jtreg/compiler/c1/KlassAccessCheckPackagePrivate.jasm @@ -0,0 +1,29 @@ +/* + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +package compiler/c1/types; + +super class PackagePrivateClass + version 51:0 +{} + diff --git a/test/hotspot/jtreg/compiler/c1/KlassAccessCheckTest.java b/test/hotspot/jtreg/compiler/c1/KlassAccessCheckTest.java new file mode 100644 index 00000000000..59df674a7a6 --- /dev/null +++ b/test/hotspot/jtreg/compiler/c1/KlassAccessCheckTest.java @@ -0,0 +1,61 @@ +/* + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8293044 + * @requires vm.compiler1.enabled + * @compile KlassAccessCheckPackagePrivate.jasm + * @compile KlassAccessCheck.jasm + * @run main/othervm -Xbatch -XX:TieredStopAtLevel=1 compiler.c1.KlassAccessCheckTest + */ + +package compiler.c1; + +public class KlassAccessCheckTest { + static void test(Runnable r) { + for (int i = 0; i < 1000; ++i) { + try { + r.run(); + throw new AssertionError("No IllegalAccessError thrown"); + } catch (IllegalAccessError e) { + // Expected + } catch (AssertionError e) { + throw e; // rethrow + } catch (Throwable e) { + throw new AssertionError("Wrong exception thrown", e); + } + } + } + + public static void main(String[] args) { + test(() -> KlassAccessCheck.testNewInstance()); + test(() -> KlassAccessCheck.testNewArray()); + test(() -> KlassAccessCheck.testMultiNewArray()); + test(() -> KlassAccessCheck.testCheckCast(42)); + test(() -> KlassAccessCheck.testCheckCastArr(new Integer[0])); + test(() -> KlassAccessCheck.testInstanceOf(42)); + test(() -> KlassAccessCheck.testInstanceOfArr(new Integer[0])); + System.out.println("TEST PASSED"); + } +}