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

[David Holmes]

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