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

[David Holmes]

On 19/06/2018 9:47 PM, Thomas Stüfe wrote:
> 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?

When your run with -Xcheck:jni the JNI entry points in jniChecked.cpp 
are used and these perform additional sanity checking and good usage 
verification.

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

Very hard to easily see what coverage that gives compared to a grep of 
all tests that use these JNI functions, unfortunately.

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

If a test fails in tier 2 or later in the CI testing then I'll probably 
have to fix it :)

I'll leave it up to you, but personally I wouldn't make the change at 
this time.

Thanks,
David (signing off for the night)

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