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

[David Holmes]

On 11/07/2013 5:46 PM, David Simms wrote:
>
> Refreshed patched: http://cr.openjdk.java.net/~dsimms/6671508/
> <http://cr.openjdk.java.net/%7Edsimms/6671508/>

Thanks David that looks good to me.

BTW can you update your webrev version. The one you are using has a 
broken frames view. Try to find at least vers 23.18-hg-never

David
-----

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