RFR: 8340812: LambdaForm customization via MethodHandle::updateForm is not thread safe

[Tobias Hartmann]

On Tue, 24 Sep 2024 15:11:58 GMT, Chen Liang <liach at openjdk.org> wrote:

>> When investigating an intermittent NPE with an Oracle internal test on AArch64, I noticed that the NPE is actually a SIGSEGV in the code emitted by `MethodHandles::jump_to_lambda_form` when trying to load the `MemberName::method` field because the `MemberName` we retrieved from `LambdaForm::vmentry` is null:
>> 
>> https://github.com/openjdk/jdk/blob/f0374a0bc181d0f2a8c0aa9aa032b07998ffaf60/src/hotspot/cpu/aarch64/methodHandles_aarch64.cpp#L141-L143
>> 
>> The JVM translates SIGSEGVs happening in method handle intrinsics to NPEs, assuming that they are implicit NPEs due to a null receiver:
>> https://github.com/openjdk/jdk/blob/f0374a0bc181d0f2a8c0aa9aa032b07998ffaf60/src/hotspot/share/runtime/sharedRuntime.cpp#L979-L982
>> 
>> After digging around in the MethodHandle implementation that I'm not too familiar with, I found this suspicious code in `MethodHandle::updateForm`:
>> https://github.com/openjdk/jdk/blob/36314a90c15e2ab2a9b32c2e471655c1b07d452c/src/java.base/share/classes/java/lang/invoke/MethodHandle.java#L1881-L1883
>> 
>> The `Unsafe.fullFence` was added by [JDK-8059877](https://bugs.openjdk.org/browse/JDK-8059877) and replaced a `// ISSUE: Should we have a memory fence here?` [comment](https://github.com/openjdk/jdk/commit/224c42ee4d4c3027d1f8f0d8b7ecf6646e9418c3#diff-5a4b2f817a4273eacf07f3ee24782c58c8ff474c6d790f72298e906837c5543aL1442). If the intention was to [prevent publishing a partially initialized object](https://wiki.sei.cmu.edu/confluence/display/java/TSM03-J.+Do+not+publish+partially+initialized+objects), I think it was added to the wrong place (paging @iwanowww).
>> 
>> In the failing scenario, we concurrently trigger LambdaForm customization for a VarHandle invoker:
>> 
>> 	at java.base/java.lang.invoke.MethodHandle.updateForm(MethodHandle.java:1883)
>> 	at java.base/java.lang.invoke.MethodHandle.customize(MethodHandle.java:1856)
>> 	at java.base/java.lang.invoke.MethodHandle.maybeCustomize(MethodHandle.java:1846)
>> 	at java.base/java.lang.invoke.Invokers.maybeCustomize(Invokers.java:632)
>> 	at java.base/java.lang.invoke.Invokers.checkCustomized(Invokers.java:626)
>> 
>> The problem is that another thread can observe `newForm` which via the `MethodHandle::form` field before the store to `vmentry` in `prepare()` (see [here](https://github.com/openjdk/jdk/blob/36314a90c15e2ab2a9b32c2e471655c1b07d452c/src/java.base/share/classes/java/lang/invoke/LambdaForm.java#L821)) is visible and therefore observe null. We need a StoreStore barrier to prevent store...
>
> @TobiHartmann Do we need some test tags like "intermittent" for such a chance-based concurrency test?

Thanks for you review, @liach!

> And seems the interpreter LF code doesn't expect customized forms.

Right, I discussed with @JornVernee off-thread who mentioned the same. I removed that flag from the test, verified that it still triggers the original issue and leave it to the method handle maintainers to address this issue separately.

Running some more testing now.

> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 1:
> 
>> 1: /*
> 
> You can bump the year here if you'd like to.

Thanks! Done.

> test/jdk/java/lang/invoke/TestLambdaFormCustomization.java line 32:
> 
>> 30:   * @bug 8340812
>> 31:   * @summary Verify that LambdaForm customization via MethodHandle::updateForm is thread safe.
>> 32:   * @run driver TestLambdaFormCustomization
> 
> Looks a bit weird that you use `driver` here and `main` on the next line; what's the convention like?

To specify custom arguments, I need to run with `othervm`, right?

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

PR Comment: https://git.openjdk.org/jdk/pull/21160#issuecomment-2371605778
PR Review Comment: https://git.openjdk.org/jdk/pull/21160#discussion_r1773574621
PR Review Comment: https://git.openjdk.org/jdk/pull/21160#discussion_r1773575768