Check memory segment ownership on downcall #22675

AditiS11 · 2025-09-24T17:23:59Z

Fixes: #22573
Ensure that checkValidState() is called for confined
memory segments which are passed as arguments to
the native call when argValue.address is invoked.
Make memArgScopeSet thread local.

With the following change, this is the error produced for the issue:

main result = 150 java.lang.WrongThreadException: Attempted access outside owning thread	at java.base/jdk.internal.foreign.MemorySessionImpl.wrongThread(MemorySessionImpl.java:330)	at java.base/jdk.internal.misc.ScopedMemoryAccess$ScopedAccessError.newRuntimeException(ScopedMemoryAccess.java:114)	at java.base/jdk.internal.foreign.MemorySessionImpl.checkValidState(MemorySessionImpl.java:217)	at java.base/openj9.internal.foreign.abi.InternalDowncallHandler.memSegmtOfPtrToLongArg(InternalDowncallHandler.java:295)	at ConfinedMemorySegmentDowncallTest.makeDownCall(ConfinedMemorySegmentDowncallTest.java:26)	at ConfinedMemorySegmentDowncallTest.lambda$main$0(ConfinedMemorySegmentDowncallTest.java:44)	at java.base/java.lang.Thread.run(Thread.java:1485)

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

tajila · 2025-09-24T21:12:20Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

+try {
+return (long) originalFilter.invokeExact(seg);
+} finally {
+sessionImpl.release0();


This wont work as you are releasing ownership of the memory segment before the call is made.

I think the way to solve this is to add a new field to HeapArgInfo called something like MemorySessionImpl[] sessions.

In the memSegmtToLongArg you can acquire the session and also add it to the active info.sessions[]. In runNativeMethod after the native has been called and before info.clear() is called, you can iterate thorugh all the sessions and release them.

Thank you @tajila . I hadn't removed the acquire0 call in SetDependency, I guess thats why the tests passed.
As there is already a call to acquire0 in SetDependency(), can we just call CheckValidState() here to ensure the confined MemorySegment is only accessed by the owner thread?

Thank you @tajila . I hadn't removed the acquire0 call in SetDependency, I guess thats why the tests passed. As there is already a call to acquire0 in SetDependency(), can we just call CheckValidState() here to ensure the confined MemorySegment is only accessed by the owner thread?

I think this could just be a call to CheckValidState or similar since the fields are already being acquired in SetDependency.

Ya good point. I think SetDependency already does the required access/release checks. The one issue with the way its currently done is that memArgScopeSet is not thread local. Perhaps this is the only thing that needs to be changed.

The session for the native call and the session for the args arent guaranteed to be the same.

acquire0 for GlobalSession (the scope that can't be closed) does not do anything.

There is also ImplicitSession which doesnt explicitly close.

acquire0 for GlobalSession (the scope that can't be closed) does not do anything.

My concern is that session(the native call session) can be global, but memArgSession can be confined. Currently the if ((owner == Thread.currentThread()) || (owner == null)) { check is against the memArgSession, but then the closeable action is on the native call session. Unless I've missed something it seems like its possible a confined session on an arg would never be closed unless the arg sessions are explcitly closed after the call.

In runNativeMethod, I see that the downcall is always wrapped in a confined Arena (https://github.com/eclipse-openj9/openj9/blob/master/jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java#L932) so I think the native call session is never global. Are there other contexts where the native call session could be global or implicit?

Right, then my previous concern is not valid. But memArgSession still needs to be thread local.

tajila · 2025-09-29T17:24:54Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

 private long[] offsets;
 private int index;
 private final int size;
+private MemorySessionImpl[] sessions;


Just realized, this can actually be an array list List<MemorySessionImpl> because we dont actually care about the position.We just need to keep track of all of them.

tajila · 2025-09-29T17:25:53Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

 }
+
+void addSession(MemorySessionImpl session) {
+sessions[index - 1] = session;


this can just add to the array list.

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

tajila · 2025-09-30T13:31:05Z

jenkins test sanity alinux64 jdk25

tajila · 2025-09-30T14:18:21Z

jenkins test sanity alinux64 jdk25

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

keithc-ca · 2025-09-30T16:12:06Z

@TobiAjila PR builds may be a little premature; the code as is will not compile for Java 21. See #22675 (comment).

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

tajila · 2025-10-02T13:16:29Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

 */
 /*[IF JAVA_SPEC_VERSION >= 21]*/
-private Set<Scope> memArgScopeSet;
+private final ThreadLocal<Set<Scope>> memArgScopeSet = ThreadLocal.withInitial(HashSet::new);


You'll have the same problem here as last time with recursive calls. This should be added to HeapArgInfo.

Currently, HeapArgInfo is used only for Java versions 22 and above, while memArgScopeSet exists for Java versions 21 and above. If we move memArgScopeSet inside HeapArgInfo, could this cause issues? (Tried the change for Java 25 and it worked)

You can include HeapArgInfo in jdk21 but exlude all the fields that arent used and just keep memArgScopeSet

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

Fixes: eclipse-openj9#22573 Ensure that checkValidState is called for memory segments which are passed as arguments to the native call when argValue.address is invoked. Make memArgScopeSet thread local.

keithc-ca · 2025-10-10T15:24:05Z

Please add a test to validate change.

AditiS11 · 2025-10-10T16:14:48Z

Please add a test to validate change.

Tested the change with the test case mentioned in the issue. It throws the required WrongThreadException now.
Please let me know if there is anything else that needs to be checked.

keithc-ca · 2025-10-10T17:35:58Z

A manual test is fine, but we should have test code in this repository that automatically checks that the issue does not return.

AditiS11 force-pushed the Issue-22573 branch 2 times, most recently from 79a4f0d to 7f35e22 Compare September 24, 2025 17:30

pshipton requested a review from tajila September 24, 2025 18:05

keithc-ca reviewed Sep 24, 2025

View reviewed changes

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java Outdated Show resolved Hide resolved

keithc-ca reviewed Sep 24, 2025

View reviewed changes

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java Outdated Show resolved Hide resolved

tajila reviewed Sep 24, 2025

View reviewed changes

AditiS11 marked this pull request as draft September 25, 2025 09:51

AditiS11 force-pushed the Issue-22573 branch 2 times, most recently from efc1e05 to 982049a Compare September 26, 2025 11:13

AditiS11 marked this pull request as ready for review September 26, 2025 11:15

AditiS11 force-pushed the Issue-22573 branch from 982049a to 67f6393 Compare September 26, 2025 17:04

AditiS11 requested a review from tajila September 27, 2025 01:01

tajila reviewed Sep 29, 2025

View reviewed changes

keithc-ca requested changes Sep 29, 2025

View reviewed changes

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java Show resolved Hide resolved

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java Outdated Show resolved Hide resolved

AditiS11 force-pushed the Issue-22573 branch from 67f6393 to 278cbc0 Compare September 30, 2025 08:27

tajila approved these changes Sep 30, 2025

View reviewed changes

AditiS11 force-pushed the Issue-22573 branch 3 times, most recently from 864edbf to a3796ec Compare September 30, 2025 14:06

keithc-ca requested changes Sep 30, 2025

View reviewed changes

keithc-ca reviewed Oct 1, 2025

View reviewed changes

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java Outdated Show resolved Hide resolved

AditiS11 force-pushed the Issue-22573 branch from a3796ec to 6944f49 Compare October 1, 2025 18:10

keithc-ca reviewed Oct 1, 2025

View reviewed changes

AditiS11 force-pushed the Issue-22573 branch from 6944f49 to 71f7abf Compare October 2, 2025 12:58

tajila reviewed Oct 2, 2025

View reviewed changes

AditiS11 force-pushed the Issue-22573 branch from 71f7abf to c56c04e Compare October 6, 2025 17:04

AditiS11 marked this pull request as draft October 6, 2025 17:06

AditiS11 marked this pull request as ready for review October 7, 2025 16:28

keithc-ca requested changes Oct 7, 2025

View reviewed changes

keithc-ca mentioned this pull request Oct 7, 2025

Add missing source folder for jdk.incubator.foreign #22738

Merged

AditiS11 marked this pull request as draft October 8, 2025 17:46

AditiS11 force-pushed the Issue-22573 branch 2 times, most recently from 26465e4 to a346b4c Compare October 9, 2025 12:17

AditiS11 marked this pull request as ready for review October 9, 2025 12:29

keithc-ca requested changes Oct 9, 2025

View reviewed changes

AditiS11 force-pushed the Issue-22573 branch from a346b4c to 899ed88 Compare October 9, 2025 19:20

keithc-ca requested changes Oct 9, 2025

View reviewed changes

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java Outdated Show resolved Hide resolved

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java Outdated Show resolved Hide resolved

AditiS11 force-pushed the Issue-22573 branch from 899ed88 to 8415c06 Compare October 10, 2025 06:28

Check memory segment ownership on downcall

9ab48fd

Fixes: eclipse-openj9#22573 Ensure that checkValidState is called for memory segments which are passed as arguments to the native call when argValue.address is invoked. Make memArgScopeSet thread local.

AditiS11 force-pushed the Issue-22573 branch from 8415c06 to 9ab48fd Compare October 10, 2025 06:56

Check memory segment ownership on downcall #22675

Are you sure you want to change the base?

Check memory segment ownership on downcall #22675

Uh oh!

Conversation

AditiS11 commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AditiS11 Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tajila Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

AditiS11 Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tajila commented Sep 30, 2025

tajila commented Sep 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithc-ca commented Sep 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithc-ca commented Oct 10, 2025

AditiS11 commented Oct 10, 2025

keithc-ca commented Oct 10, 2025

Labels

4 participants

AditiS11 commented Sep 24, 2025 •

edited

Loading

AditiS11 Sep 25, 2025 •

edited

Loading

tajila Sep 30, 2025 •

edited

Loading

AditiS11 Oct 1, 2025 •

edited

Loading