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

[David Simms]

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/20130821/0a1d5337/attachment.html