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

[Thomas Stüfe]

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

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