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

[Kim Barrett]

On Mon, 5 Jul 2021 00:47:09 GMT, David Holmes <dholmes 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.
>
> Hi Kim,
> 
> Based on  your description and previous discussion this all looks good to me.
> 
> Thanks,
> David

Thanks for reviews @dholmes-ora and @tschatzl .

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

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