RFR: 8195690: JNI GetObjectRefType doesn't handle NULL

[Doerr, Martin]

Hi Kim,

looks good to me as well. The simplification David has requested should be done, but I don't need to see a new webrev for that.
I've also run tests on several machines and they look good.

Best regards,
Martin


-----Original Message-----
From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Kim Barrett
Sent: Dienstag, 23. Januar 2018 03:21
To: David Holmes <david.holmes at oracle.com>
Cc: hotspot-runtime-dev at openjdk.java.net runtime <hotspot-runtime-dev at openjdk.java.net>
Subject: Re: RFR: 8195690: JNI GetObjectRefType doesn't handle NULL

> On Jan 22, 2018, at 8:51 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> On 23/01/2018 11:22 AM, Kim Barrett wrote:
>>> On Jan 22, 2018, at 5:29 PM, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> Hi Kim,
>>> 
>>> As a general rule JNI does not do argument checking. You pass crud then you may crash:
>>> 
>>> "The programmer must not pass illegal pointers or arguments of the wrong type to JNI functions. Doing so could result in arbitrary consequences, including a corrupted system state or VM crash."
>>> 
>>> https://docs.oracle.com/javase/9/docs/specs/jni/design.html#reporting-programming-errors
>> GetObjectRefType is specified to return JNIInvalidRefType when called with a NULL argument:
>> https://docs.oracle.com/javase/9/docs/specs/jni/functions.html#getobjectreftype
> 
> Sorry - yes this specific case is covered and should not crash on NULL.
> 
> The fix looks good. The related asserts etc look good.

Thanks.

> The only comment I have is os.cpp:
> 
> +  // Handle NULL first, so later checks don't need to protect against it.
> +  if (addr == NULL) {
> +    st->print_cr(PTR_FORMAT " is NULL", p2i(addr));
> +    return;
> +  }
> 
> p2i(NULL) will always be zero so this could be simplified.

Yeah, a little too much copy-paste there.  I’ll change that to print the literal string “0x0 is NULL”, or something similar.

> Currently for 0 we would hit:
> 
> 1141   st->print_cr(INTPTR_FORMAT " is an unknown value", p2i(addr));
> 
> which seems reasonable even for the zero case - after all we're not dealing with pointers here but just register values that may have a mapping into memory.

We don’t get that far without problems now, because of the calls to the various JNIHandles predicates that require a non-NULL argument.

> But that all said, what you have is adequate.
> 
> Thanks,
> David
> -----