jni-rs: Get array elements error

Get array elements test are failing:

The error is:

ReleasePrimitiveArrayCritical: release array failed bounds check, incorrect pointer returned ? array: 0x00007f4de007bc40 carray: 0x00007f4dd8001260
GuardedMemory(0x00007f4de445c420) base_addr=0x00007f4dd8001240 tag=0x00007f4dd8001720 user_size=24 user_data=0x00007f4dd8001260
  Header guard @0x00007f4dd8001240 is BROKEN
  Trailer guard @0x00007f4dd8001278 is OK
  User data appears to have been freed
FATAL ERROR in native method: ReleasePrimitiveArrayCritical: failed bounds check

About this issue

Original URL
State: closed
Created 4 years ago
Reactions: 1
Comments: 22 (22 by maintainers)

Commits related to this issue

Fix: ReleaseMode struct / methods (#283) — committed to maurolacy/jni-rs by maurolacy 4 years ago
Fix: ReleaseMode struct / methods (#283) — committed to maurolacy/jni-rs by maurolacy 4 years ago
Disable commit() test Test disabled until issue #283 is resolved — committed to maurolacy/jni-rs by maurolacy 4 years ago
Disable commit() test Test disabled until issue #283 is resolved — committed to maurolacy/jni-rs by maurolacy 4 years ago
Add size() helper for Auto*Array (#286) * Add size() helper for Auto*Array * Disable commit() test until issue #283 is resolved — committed to jni-rs/jni-rs by maurolacy 4 years ago

Most upvoted comments

Thanks for the recommendations.

I was analyzing this during the weekend. For what I’ve seen, just reverting the commit you linked (openjdk/jdk@3e904a) would fix the bug. AFAIK, there are no tests in openjdk that validate or depend on this change. So, unless I add a specific test for this, reverting that commit is the only change that is needed to fix this.

My plan is then to fork openjdk and send them a PR, including just that reverted commit. I’ll then write a good description, explaining our use case (that we have -Xcheck:jni enabled for tests), citing references and all. And, if they ask me to open an issue, I’ll do it and follow through from there.

maurolacy on Nov 18, 2020

OK, I’ve decided to take this 😃

My plan is to download OpenJDK sources, fix the bug, confirm / test it’s fixed, and send a patch to the OpenJDK project.

Any advice is welcome.

maurolacy on Nov 15, 2020

Agree, commit in the critical variant is not particularly useful.

BTW, I’ve briefly sampled GH for some valid usages of JNI_COMMIT. This OSS project, for example, creates a global reference to an array; then takes its “elements” (either a pointer to the original or a copy — it targets Android, where both are possible); and once in a while JNI_COMMITs them. I suppose, some code updates the “elements”, commits them, and somehow notifies the Java part about the changes. This saves a potential copy from Java array.

Project Panama, I believe, has already brought a better way to access native memory than direct ByteBuffers (another option here), but I haven’t checked that yet.

dmitry-timofeev on Jan 17, 2021

Hi,

According to the latest comments in the openjdk/jdk#1697 thread, they are not going to change the behaviour of ReleasePrimitiveArrayCritical with JNI_COMMIT and -Xcheck:jni. It’s still open if something will be changed with Release*ArrayElements.

That effectively means commit() is not only buggy, but also useless. So, I propose we remove commit(), at least from AutoPrimitiveArray.

Once the issue with Release*ArrayElements is resolved, we can keep commit(), if it’s not buggy / inconsistent. Or, remove it too from AutoArray.

What do you think?

maurolacy on Dec 22, 2020

FYI: https://github.com/openjdk/jdk/pull/1697.

maurolacy on Dec 8, 2020

Sounds great, that’s a good way to contribute to one of the biggest projects out there!

I’d only recommend getting in touch with the maintainers early, so that they are aware of this problem, and can help getting it integrated. CC-ing people from the original issue and the list discussions could also help get the right people in the loop.

As OpenJDK is a big project with huge impact, they have strict requirements in terms of project documenation, and won’t merge a patch that changes behaviour without a corresponding issue (i.e., if it is not a pure refactoring or document improvements). One of the benefits is that we were able to find by a patch that introduced the bug the issue and the original discussion quickly 😃

OpenJDK Jira doesn’t allow submitting issues directly, so, again, it makes sense to write to the list or submit an issue through the Oracle site, but in this particular case I think the list might work better.

dmitry-timofeev on Nov 17, 2020

If you switch it off, tests pass (even the longer one that does writes after committing, and verifies both results on the Java array). Both the observed behaviour and the implementation (without extra checks) confirm that such scenario does work.

dmitry-timofeev on Nov 4, 2020

The Hotspot implementation supports the commit use-case (which I previously verified in tests without “check:jni” mode): https://github.com/openjdk/jdk/blob/6bc493188b265b1f9776aa62f77c161d75fc591e/src/hotspot/share/prims/jni.cpp#L2623-L2639

JNI check implementation does not distinguish commit from 0, releasing the guarded memory: https://github.com/openjdk/jdk/blob/976acddeb5a8df1e868269787c023306aad3fe4a/src/hotspot/share/prims/jniCheck.cpp#L422-L441

As the mode is named “commit” (write the changes) and not “copy the contents and transfer the buffer ownership to me”; and that the buffer is allocated inside the JNI library (and it is reasonable for that library to be responsible for freeing the buffer to avoid any issues with allocators), I think there is no discrepancy between Hotspot and Android, but a bug in “check:jni” in Hotspot. Here is a commit introducing this bug: https://github.com/openjdk/jdk/commit/3e904a4801b2bf2e988ba096e5cb64a17fd5fce7 , the issue: https://bugs.openjdk.java.net/browse/JDK-8193234 and a discussion on the mailing list: https://mail.openjdk.java.net/pipermail/hotspot-dev/2017-December/029523.html

Probably, the best solution would be to study that discussion, confirm our understanding of the spec on an appropriate mailing list, and if there is a bug in “check:jni” — fix it.

Alternatively, we can:

Keep the test for commit in a separate file that creates a VM without “check:jni”
Remove it.

dmitry-timofeev on Nov 4, 2020

@dmitry-timofeev I turned commit() into a method because my understanding at the time was that it was possible to call ReleaseArrayElements over the array again, later.

I think your original understanding of the spec is actually correct. I don’t see the evidence in the docs that you can commit and forget.

The evidence I have for that is scarce, true:

The name of the JNI method, which starts with Release.
Never seen a code that makes two Release calls.
The Best practices doc above, where a final Release with JNI_COMMIT is done.

I thought you can commit and can keep the pointer, but it doesn’t seem to be true (see for instance the JNI tips for Android — though Dalvik is certainly far from the Hotspot):

Also note that the JNI_COMMIT flag does not release the array, and you will need to call Release again with a different flag eventually.

That comment may be true of the Android VM. But, I’ve never read something like that in the official JNI docs.

We may be hitting some worst case scenario here, in that, or there’s an underlying bug in some implementations, or even worse, different implementations understood JNI_COMMIT differently (like ourselves), and so, it’s impossible to provide a consistent calling pattern accross them.

I think the safest bet, until some particular use case or calling pattern appears, would be to remove this release mode entirely.

maurolacy on Nov 3, 2020