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

[Chen Liang]

On Tue, 24 Sep 2024 14:18:58 GMT, Tobias Hartmann <thartmann 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 stores that initialize the newForm from being r...

Looks good; the form field is final so it should already have loadLoadFence at read. The full fence seems redundant, as the old and new forms must be functionally the same.

Marked as reviewed by liach (Reviewer).

For the test failure: LambdaForm goes from generic to specific; we prefer to compile lambda forms before customizing them (as you see, all our customization eagerly compiles the customized result). And seems the interpreter LF code doesn't expect customized forms.

Maybe we can add some checks to ensure the customization threshold is not less than the compilation threshold.

@TobiHartmann Do we need some test tags like "intermittent" for such a chance-based concurrency test?

src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 1:

> 1: /*

You can bump the year here if you'd like to.

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?

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

Marked as reviewed by liach (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21160#pullrequestreview-2325572020
PR Review: https://git.openjdk.org/jdk/pull/21160#pullrequestreview-2325650013
PR Comment: https://git.openjdk.org/jdk/pull/21160#issuecomment-2371583482
PR Comment: https://git.openjdk.org/jdk/pull/21160#issuecomment-2371592225
PR Review Comment: https://git.openjdk.org/jdk/pull/21160#discussion_r1773566766
PR Review Comment: https://git.openjdk.org/jdk/pull/21160#discussion_r1773565669