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

[Thomas Stüfe]

Hi David,

On Tue, Jun 19, 2018 at 12:17 PM, David Holmes <david.holmes at oracle.com> wrote:
> 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.

Can you elaborate?

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

I ran jdk-submit tests.

Not sure if I think the risk is very high. assert(array==NULL) would
crash today too, and the chance of a test accidentally feed something
else than a jobjectArray into one of these functions are rather small,
imho.

Still, if you think I should test run other tests, I'll do it. But
maybe alternatively I would leave out the asserts for another day &
fix, since the bug is P2 and I would like to close it.

Thanks, Thomas

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