RFR (S) 6671508 - JNI GetPrimitiveArrayCritical should not be callable on object arrays

[David Simms]

Refreshed patched: http://cr.openjdk.java.net/~dsimms/6671508/ 
<http://cr.openjdk.java.net/%7Edsimms/6671508/>

On 07/11/2013 08:30 AM, David Holmes wrote:
> On 9/07/2013 11:54 PM, David Simms wrote:
>> Gidday David,
>
> G'day :)
>
>>
>> Inline...
>>
>> On 09/07/13 13:31, David Holmes wrote:
>>> Hi David,
>>>
>>> On 8/07/2013 11:24 PM, David Simms wrote:
>>>>
>>>> Agreed on the use of negative constants is getting more and more
>>>> overcomplicated (especially when it was representing the typed 
>>>> BasicType
>>>> enum). Reorganized as you suggest...(also "Type array": "Primitive 
>>>> type
>>>> array expected..." is more useful).
>>>>
>>>> Refreshed: http://cr.openjdk.java.net/~dsimms/6671508/
>>>> <http://cr.openjdk.java.net/%7Edsimms/6671508/>
>>>
>>> I like this but am unclear on the details of some of the checks. This
>>> old code puzzles me:
>>>
>>> !   if (elementType != -1) {
>>> !     if (aOop->is_typeArray()) {
>>> !       BasicType array_type =
>>> TypeArrayKlass::cast(aOop->klass())->element_type();
>>> !       if (array_type != elementType)
>>> !         ReportJNIFatalError(thr, fatal_element_type_mismatch);
>>> !       } else if (aOop->is_objArray()) {
>>> !         if ( T_OBJECT != elementType)
>>>             ReportJNIFatalError(thr, fatal_object_array_expected);
>>>
>>> Can the last "if" ever be true if we are a typeArray? I would not have
>>> expected that is_typeArray and is _objArray can be true at the same
>>> time. But if they can then your new check_is_obj_array should probably
>>> also be testing that the element type is T_OBJECT, otherwise that part
>>> of the check is being lost.
>>>
>> The old code's formatting is a little misleading I think, should read
>> (with full bracketing):
>
> Doh! My brain kept filling in a brace that wasn't there.
>
>>        if (aOop->is_typeArray()) {
>>           BasicType array_type = 
>> TypeArrayKlass::cast(aOop->klass())->element_type();
>>           if (array_type != elementType) {
>>              ReportJNIFatalError(thr, fatal_element_type_mismatch);
>>           }
>> *       } else if (aOop->is_objArray()) {**
>> **          if (T_OBJECT != elementType) {**
>> **             ReportJNIFatalError(thr, fatal_object_array_expected);**
>> **          }**
>> **       }*  else {
>>           ReportJNIFatalError(thr, fatal_unknown_array_object);
>>        }
>>
>> Or:
>>
>>   * If /is_typeArray/
>>   * else if /is _objArray/
>>   * else /unknown /(currently redundant, but safe)
>>
>> And when I look more closely at klass.hpp, you are right is_objArray()
>> is not the same as:
>>
>> TypeArrayKlass::cast(aOop->klass())->element_type() == T_OBJECT
>>
>> but the old code does actually check that either...it checks
>> is_objArray(), and the supplied elementType is T_OBJECT.
>>
>> I looked at klass.hpp for a while, and even tested "array of array"
>> element_type() == T_OBJECT. Using "element_type() == T_OBJECT" might be
>> slightly more correct than is_objArray() ?
>>
>>> It is also unclear to me that check_array_element_type should only be
>>> expecting primitive arrays ??
>> It's only use is in primitive type array macros to get generate
>> set/get/scalar ops.
>>
>> You mean it should be called check_primitive_array_type for more 
>> clarity ?
>
> Yes please :)
>
> Thanks,
> David
>
>>>
>>> Thanks,
>>> David
>>>
>>>>
>>>> On 08/07/13 14:14, David Holmes wrote:
>>>>> Hi David,
>>>>>
>>>>> Silly question but what the heck is a "typeArray" meant to be? Is 
>>>>> it a
>>>>> primitive array ?? If so we should at least use that terminology in
>>>>> the error message! (I couldn't see a clear definition anywhere other
>>>>> than typeArray is not objArray - which implies it is a primitive
>>>>> array).
>>>>>
>>>>> I must admit I don't like the use of negative elementType values as a
>>>>> means to encode what type of checks to perform, or not perform. I'd
>>>>> much rather see simple clear checks:
>>>>> - check_is_array
>>>>> - check_array_element_type
>>>>> - check_is_obj_array
>>>>> - check_is_primitive_array
>>>>> etc.
>>>>>
>>>>> I had to keep re-reading the code to get the sense of the checks
>>>>> correct.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 8/07/2013 9:19 PM, David Simms wrote:
>>>>>> |Please review a small fix:||
>>>>>>
>>>>>> |http://cr.openjdk.java.net/~dsimms/6671508/
>>>>>> <http://cr.openjdk.java.net/%7Edsimms/6671508/>
>>>>>>
>>>>>> Bug:
>>>>>>
>>>>>> https://jbs.oracle.com/bugs/browse/JDK-6671508||
>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6671508
>>>>>>
>>>>>> |Summary of fix:||
>>>>>> ||
>>>>>>      As noted in the bug, standard use of JNI does not protect the
>>>>>> user
>>>>>> from abusing the API (e.g. supplying incorrect arguments).
>>>>>>      The preferred method to discover such errors is via "checked 
>>>>>> JNI"
>>>>>> (i.e. -Xcheck:jni).
>>>>>>      Added a further constant to "check_array()" to enforce 
>>>>>> primitive
>>>>>> type array arg checking for Get/ReleasePrimativeArrayCritical.
>>>>>>
>>>>>>      coleenp has already reviewed and okayed the fix (but is 
>>>>>> currently
>>>>>> on vacation).
>>>>>> ||
>>>>>> ||Tests:||
>>>>>> ||
>>>>>>      * JPRT
>>>>>>      * UTE "vm.quick.testlist"
>>>>>>      * JCK "vm_jni"
>>>>>> ||
>>>>>> ||Thanks||
>>>>>> ||/David Simms||
>>>>>> ||
>>>>>> |
>>>>
>>

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