RFR (S): 8022683 : JNI GetStringUTFChars should return NULL on allocation failure not abort the VM

[David Holmes]

On 21/08/2013 8:30 PM, David Simms wrote:
>
> Been a little side-track, reply inline...
>
> On 19/08/13 05:51, David Holmes wrote:
>> Hi David,
>>
>> On 17/08/2013 12:45 AM, David Simms wrote:
>>> Hello all,
>>>
>>> Need reviewers on a JDK8 fix for:
>>> http://bugs.sun.com/view_bug.do?bug_id=8022683
>>>
>>> Found the following functions need to return NULL as suggested by the
>>> JNI spec
>>> (http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html):
>>>
>>>
>>>   * GetStringChars()
>>>   * GetStringUTFChars()
>>>   * Get<PrimitiveType>ArrayElements() family of array getters (same
>>>     generating macro).
>>>
>>> Although the specification suggests OOME may be thrown by certain
>>> functions, the documentation on the aforementioned functions does not
>>> suggest "throws". So they return NULL now without aborting the JVM as
>>> before.
>>
>> There is always come contention with the JNI spec as to whether the
>> intent is as defined by the original JNI spec book, or whether it is
>> reflected in what is considered the "official spec". :)
>>
> Agreed, but let's just take the documentation developers have to go on.
>>> It was assumed the meaning of "isCopy" is undefined given a NULL result,
>>> in keeping with the rest of jni.cpp.
>>
>> Even so I think it preferable to not set it unless the operation
>> succeeds.
>>
> Done
>>> Also discovered allocation.hpp macros missing proper argument bracketing
>>> (e.g. size passed in NEW_C_HEAP_ARRAY_RETURN_NULL()), fixed them up
>>> since I was there (and they caused trouble).
>>
>> I can only see size causing a problem because it is used in an
>> expression. The rest seems somewhat overkill.
>>
> Fine, no need to get too happy, also done.
>
>     Been used to as a matter of practice bracketing anything involving
>     possible expression/calculation, i.e. pointer math.
>
> Updated webrev:
>>> Webrev:
>>>
>>> http://cr.openjdk.java.net/~dsimms/8022683/
>>> <http://cr.openjdk.java.net/%7Edsimms/8022683/>
>>
>> One concern I have is in how the dtrace probes will handle things if
>> buf/result is NULL?
> Previous behavior was to exit the vm with OOM error (i.e.:
> vm_exit_out_of_memory()), which dtrace doesn't handle (so we are good).

My concern is, if the dtrace probes see a NULL will that cause them to 
crash or will they handle the NULL okay?

> Begs the question, how many applications will now simply crash within
> their native code, and does this change reduce the amount of diagnostics
> information. Whilst I prefer to stick the API documentation, wondering
> if this will be practical...  Okay tested with -Xcheck:jni which catches
> the problem (so we are good).

What did Xcheck:jni catch? Subsequent attempted use of the NULL in a JNI 
call? I would hope so. But if the application code doesn't check for 
NULL then it could still crash before the next JNI call.

This is always a risk with making behaviour compliant with spec. I think 
the risk is low because if we have exhausted C heap then the VM is about 
to die at any moment anyway. But we could add a release note to document 
this.

Thanks,
David

>>
>> Thanks,
>> David
>>
>>> Testing:
>>>
>>> Attached "unit test" to bug, naive test program to trigger OOM. It is
>>> not suitable for automated testing. Ideally require hooks into the JVM
>>> to simulate OOM during JNI calls.
>>>
>>> Cheers
>>> /David Simms
>