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

[David Simms]

Thanks David, will update
Cheers
/D

On 12/07/13 06:34, David Holmes wrote:
> 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||
>>>>>>>> ||
>>>>>>>> |
>>>>>>
>>>>
>>