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

[Coleen Phillimore]

DavidS,

I think this change looks good.  Going from a vm_exit_out_of_memory() in 
JVM to dtrace (sdt probes on some linuxes) not providing information, 
which it couldn't before with a crash, is still an improvement.

Coleen

On 8/21/2013 6:30 AM, 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).
>
> 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).
>
>>
>> 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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130823/d3c8fe37/attachment-0001.html