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

[David Simms]

Thanks for the review !

On 08/23/2013 08:26 PM, Coleen Phillimore wrote:
>
> 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/20130825/bdb4d1f7/attachment.html