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