RFR: 8261395: C1 crash "cannot make java calls from the native compiler"

[David Holmes]

On Fri, 7 May 2021 15:17:38 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> If a nest host and a nest member are associated with different protection domains it can lead to execution of Java code (to validate the "new" protection domain) during a nestmate access check, if nest membership verification has not yet been performed. This will cause assertion or guarantee failures if executed by a JIT compiler thread during access checks.
>> 
>> After much discussion and trying different solutions it was decided that the existing logic for nest membership validation unnecessarily tries to resolve constant-pool entries, when it suffices that the symbolic entry in the constant-pool has the same name as the class being checked. Given this check occurs after we have verified the nest host and the purported member are loaded by the same classloader and in the same runtime package, there can only be one class with the name of the member, and that is the member class. Hence resolution of the constant-pool entry serves no purpose but introduces the complexity of dealing with exceptions and avoiding Java code execution in compiler threads.
>> 
>> @iklam contributed to an earlier version of the fix, and devised the initial testcase approach.
>> @coleenp also contributed to an earlier version of the fix. 
>> 
>> Thanks to both Coleen and Ioi for their insights, discussions and contributions. 
>> 
>> Testing:
>> - the new test
>> - tiers 1-3
>> 
>> Thanks,
>> David
>
> src/hotspot/share/oops/instanceKlass.cpp line 170:
> 
>> 168:   if (_nest_members == NULL || _nest_members == Universe::the_empty_short_array()) {
>> 169:     if (log_is_enabled(Trace, class, nestmates)) {
>> 170:       ResourceMark rm(current);
> 
> Since THREAD is only used for ResourceMark and HandleMark which don't have to be a JavaThread, this parameter can just be Thread* current.  And it should be in the first argument so it's not confused with CHECK.

I can switch the argument order - that is a good suggestion and avoids the need for other changes. But I will still make it a JavaThread as only a JavaThread executes this code.

> src/hotspot/share/oops/instanceKlass.cpp line 251:
> 
>> 249: // are idempotent.
>> 250: InstanceKlass* InstanceKlass::nest_host(TRAPS) {
>> 251:   JavaThread* current = THREAD->as_Java_thread();
> 
> I don't see why this is necessary to do this conversion.

So that we don't use THREAD in the call to has_nest_member as it is not exception related. But if the Thread arg goes first on that call I can switch this back.

> test/hotspot/jtreg/runtime/Nestmates/protectionDomain/TestDifferentProtectionDomains.java line 69:
> 
>> 67:         }
>> 68: 
>> 69:         static byte[] getBytesForClass(String name) throws IOException {
> 
> There's a ClassUnloadCommon library call for this.

Thanks. I copied this from another test. If there's an existing utility I will switch to it.

> test/hotspot/jtreg/runtime/Nestmates/protectionDomain/TestDifferentProtectionDomains.java line 125:
> 
>> 123:         // as any earlier causes security exceptions running the test and we
>> 124:         // don't want to have to set up a policy file etc.
>> 125:         System.setSecurityManager(new SecurityManager());
> 
> You might need to add the option -Djava.security.manager=allow to this test.

Thanks I'll check that out.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3913