[jdk17] RFR: 8269661: JNI_GetStringCritical does not lock char array

[Kim Barrett]

On Mon, 5 Jul 2021 07:51:52 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Please review this fix to jni_GetStringCritical.  When object pinning is
>> being used it pins/unpins the String argument, but that's the wrong object.
>> It should be pinning the associated byte array (when !latin1), as it is a
>> reference into that array that will be returned and must be imobilized.
>> 
>> Presently only Shenandoah is affected by this bug, as it is the only GC that
>> uses object pinning (at the region level) rather than the GC locker.  But
>> both G1 and ZGC plan to use object pinning in the future.  But with region
>> pinning the problem is still going to be rare because the String and its
>> value array are likely to be allocated in the same region.
>> 
>> In addition, if pinning the String's value array, we also need to prevent
>> string deduplication from changing the array while in the critical section.
>> Otherwise, the unpin when the critical section is released will use the
>> wrong object for the unpin operation.  We accomplish this by marking the
>> String as no longer subject to string deduplication.  As that state is
>> sticky, when pinning is used a String can't be deduplicated after being used
>> in a critical section.  We could add a critical section counter to String,
>> but for now we're going to guess that isn't worth the effort.  (String has
>> at least 2 bytes available for the two string deduplication flags plus such
>> a counter (more in some configurations, but always at least 2 at present)).
>> 
>> As part of the restructuring to accomplish these changes, it proved to be
>> easy to avoid using the GC locker (or pinning) for the latin1 case, where a
>> copy of the value array will be used.  Hence this change also addresses
>> JDK-8269650.
>> 
>> Testing:
>> Existing tests for GetStringCritical are in various vmTestbase/gc/lock
>> tests and vmTestbase/nsk/stress/jni/jnistress004.java.
>> 
>> Ran mach5 tier1, tier5.  Tier5 is where the vmTestbase/gc/lock and
>> vmTestbase/nsk/stress/jni tests are run.
>> 
>> Locally (linux-x64) ran those tests with Shenandoah, with and without
>> string deduplication enabled.
>
> src/hotspot/share/prims/jni.cpp line 2894:
> 
>> 2892:       ret[s_len] = 0;
>> 2893:     }
>> 2894:     if (isCopy != NULL) *isCopy = JNI_TRUE;
> 
> Pre-existing: This also returns `JNI_TRUE` for `isCopy` if the return value is `NULL`. It probably does not matter, but seems strange. Not sure if something should be done about this (separately).

I noticed that, but decided against making any change to that behavior. I think it's arguable whether it's correct usage, but I can imagine a caller that used a JNI_FALSE isCopy result to indicate no copy was attempted so no OOME could have happened so the NULL check of the result isn't needed.  The specification doesn't say what the isCopy value should be in the OOME case.

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

PR: https://git.openjdk.java.net/jdk17/pull/209