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

Karen Kinnear karen.kinnear at oracle.com
Mon Jul 15 07:00:27 PDT 2013


Changeset is great - go and and push.

thanks,
Karen

On Jul 12, 2013, at 9:36 AM, David Simms wrote:

> Hi Karen,
> 
> is_objArray() is wider (it allows for an array of any non primitive type) than the current patch "element_type() == T_OBJECT" (more strict). So that check wouldn't add anything extra.
> 
> Since this is checked JNI I did produce a unit test in the form of a script (similar to "test/runtime/7107135"), since it needs some native code and a switch to the JVM.  Coleen suggested attaching to the bug (which I did) rather than checking it in, due to missing test infrastructure, so we could add it later...you prefer it checked in, or in another form ?
> 
> Cheers
> /David
> 
> On 12/07/13 15:06, Karen Kinnear wrote:
>> 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