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

[David Holmes]

On 25/08/2013 7:21 PM, David Simms wrote:
>
> 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 !

Great! Thanks for checking that out.

David

> /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
>>>
>