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

[David Holmes]

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.

It is also unclear to me that check_array_element_type should only be 
expecting primitive arrays ??

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