RFR(xs): 8205141: runtime/exceptionMsgs/ArrayStoreException/ArrayStoreExceptionTest.java failed with "assert(k->is_objArray_klass()) failed: cast to ObjArrayKlass"

[David Holmes]

Hi Thomas,

On 19/06/2018 7:15 PM, Thomas Stüfe wrote:
> Hi David,
> 
> please find here the new webrev:
> 
> http://cr.openjdk.java.net/~stuefe/webrevs/8205141-ArrayStoreExceptionTest-fails-with-assert/webrev.01/webrev/
> 
> No attempt to handle the callers program error now. I removed the
> offending tests. 

Good.

> For good measure, I also added asserts to
> jni_{S|G}etObjectArrayElement, to make these kind of program errors
> crash a debug VM in a reproducable way.

Might be something more useful (if not already present) in the checked 
JNI implementation. By adding these there's a chance you'll expose 
existing tests that do the wrong thing but pass by "accident". Have you 
run all the JNI tests that use this API?

Style nit:

assert(a->is_objArray() == true, "...");

For bool/boolean values we don't compare against true/false

Cheers,
David
-----

> Cheers, Thomas
> 
> 
> 
> 
> 
> On Tue, Jun 19, 2018 at 9:41 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>> On Tue, Jun 19, 2018 at 9:32 AM, David Holmes <david.holmes at oracle.com> wrote:
>>> Hi Thomas,
>>>
>>> On 19/06/2018 5:26 PM, Thomas Stüfe wrote:
>>>>
>>>> Hi all,
>>>>
>>>> may I have reviews for this fix please:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8205141
>>>>
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8205141-ArrayStoreExceptionTest-fails-with-assert/webrev.00/webrev/
>>>>
>>>> Goetz' new ArrayStoreExceptionTest uncovered a problem if the caller
>>>> erroneously feeds an jobject as jobjectArray parameter to
>>>> jni_SetObjectArrayElement.
>>>>
>>>> The current behavior is to randomly either return a misleading
>>>> ArrayIndexOOBE, or to assert (debug) or to overwrite some memory
>>>> (release) and potentially crash.
>>>>
>>>> I know that feeding an jobject as jobjectArray is of course all wrong
>>>> and UB I still think it makes in this case sense to catch that and
>>>> throw a correct ArrayStoreException instead of the current behavior.
>>>
>>>
>>> Sorry but I disagree. JNI deliberately does not perform this kind of input
>>> parameter checking for performance reasons. It is up to the programmer to
>>> always pass correctly typed arguments.
>>>
>>> https://docs.oracle.com/javase/10/docs/specs/jni/design.html#reporting-programming-errors
>>>
>>> If we were to check this then it would be an IllegalArgumentException IMO
>>> anyway. But we don't check it.
>>>
>>> Thanks,
>>> David
>>> -----
>>
>> Sigh :)
>>
>> Yes, I expected that. Then I will just remove the offending test
>> methods. I'll post an update.
>>
>> ..Thomas
>>
>>>
>>>
>>>> Now, SetObjectArrayElement will throw:
>>>>
>>>> - "xxx is not an array" for types which are not even an array
>>>> - "xxx is not an object array" for primitive arrays.
>>>>
>>>> Thank you,
>>>>
>>>> Thomas
>>>>
>>>