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

[David Simms]

Hi David,

So I've tested with dtrace both "by hand" with probes enabled and 
scripting and using the test list (faked OOM and NULL return). No 
problems encountered. Assume we are good, thanks for the review !

/Simms

On 08/21/2013 12:47 PM, David Holmes wrote:
> 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
>>