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

[David Simms]

Gidday David,

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

       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 ?
>
> 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/20130709/0a30aa73/attachment.html