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

[David Holmes]

On Sun, 4 Jul 2021 04:55:56 GMT, Kim Barrett <kbarrett 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

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

Marked as reviewed by dholmes (Reviewer).

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