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

[Lindenmaier, Goetz]

Hi Thomas, 

Thanks for fixing this issue introduced by me. Apologies
for breaking this.

I'm fine with your change, even more if you leave out the 
asserts and just remove the test.

No new webrev needed.

Best regards,
  Goetz. 

> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of David Holmes
> Sent: Dienstag, 19. Juni 2018 14:57
> To: Thomas Stüfe <thomas.stuefe at gmail.com>
> Cc: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
> Subject: Re: RFR(xs): 8205141:
> runtime/exceptionMsgs/ArrayStoreException/ArrayStoreExceptionTest.java
> failed with "assert(k->is_objArray_klass()) failed: cast to ObjArrayKlass"
> 
> 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
> >>>>>>
> >>>>>
> >>