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