Bug 1536762 - Misuse of JNI by libguestfs Java bindings
Summary: Misuse of JNI by libguestfs Java bindings
Keywords:
Status: NEW
Alias: None
Product: Virtualization Tools
Classification: Community
Component: libguestfs
Version: unspecified
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Richard W.M. Jones
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-01-20 22:03 UTC by Demi Marie Obenour
Modified: 2018-07-18 15:44 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed:
Embargoed:


Attachments (Terms of Use)

Description Demi Marie Obenour 2018-01-20 22:03:09 UTC
Description of problem:


The libguestfs Java bindings are buggy: they do not check for Java exceptions after all JNI calls that might throw them, and they do not acquire all needed `jclass`, `jmethodid`, and `jfieldid` objects during `JNI_OnLoad`.  They also leak local references.  Furthermore, potentially large objects are copied between Java and native, instead of using direct `ByteBuffer`s.

Probably the easiest solution is to use `sun.misc.Unsafe`/`jdk.unsupported.misc.Unsafe` and friends to do all marshaling in Java.

Comment 1 Richard W.M. Jones 2018-01-22 16:48:26 UTC
I have to say I don't know very much about JNI.  Do you have suggested
patches or outline changes that need to be made?

Comment 2 Demi Marie Obenour 2018-02-01 23:49:10 UTC
* Firstly, any calls to `FindClass`, `Get*MethodID`, and other such methods (anything that returns a `jclass`, `jmethodid`, or a `jfieldid`) should be done in `JNI_OnLoad` and the `jclass` objects converted to global refs.

* Java exceptions need to be checked (using `ExceptionCheck`) after every operation that might throw them, unless the function return code guarantees that there is no pending exception, or unless one is simply returning.

* Local references must be released with `DeleteLocalRef` when no longer needed.  If the number of local references may exceed 16, it is necessary to call `EnsureLocalCapacity` and check its return value.

* Any data that is not of small and fixed size should be stored in direct `ByteBuffer`s on the Java side and accessed using `GetDirectBufferAddress` or (better yet) the corresponding call from `sun.misc.Unsafe`.  It should be accessed from Java using the corresponding functions of `ByteBuffer` or `Unsafe`.  This is the only way to avoid an O(n) copy on JVMs that do not support pinning (most of them) with native methods that might block (most of them).

* Calling into native from Java is much, much faster than the reverse.  Marshalling code should thus be in Java, rather than in C.


Note You need to log in before you can comment on or make changes to this bug.