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

Karen Kinnear karen.kinnear at oracle.com
Fri Jul 12 06:06:37 PDT 2013


David,

The code looks good - in fact it looks great - a huge improvement!
Can you make a small test to check in with it please?

The only piece I don't understand is the is_objArray test - which is now missing - is
there a way to get an array with T_OBJECT that doesn't pass the is_objArray test?
Since people can pass complete garbage to jni calls - perhaps worth adding that
check back as well? Or not?

thanks so much!
Karen

On Jul 12, 2013, at 12:34 AM, 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||
>>>>>>>> ||
>>>>>>>> |
>>>>>> 
>>>> 
>> 



More information about the hotspot-runtime-dev mailing list