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

[Thomas Stüfe]

Hi David, Goetz,

please find here the latest webrev:

http://cr.openjdk.java.net/~stuefe/webrevs/8205141-ArrayStoreExceptionTest-fails-with-assert/webrev.02/webrev/

nothing exciting, I just decided to not assert anything. So this only
removes the tests.

I'll let jdk-submit run again (it hiccuped before) and will push
tomorrow morning.

Thanks, Thomas




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