RFR: 8195690: JNI GetObjectRefType doesn't handle NULL

[David Holmes]

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

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.

But that all said, what you have is adequate.

Thanks,
David
-----


> The bug was discovered by a failing JCK test; we are asserting (or possibly invoking undefined behavior) without this change.
> 
>> David
>> -----
>>
>> On 23/01/2018 8:24 AM, Kim Barrett wrote:
>>> Please review this fix of GetObjectRefType when applied to a NULL
>>> handle argument.  It should return JNIInvalidRefType, but was instead
>>> asserting in debug builds, and maybe worse things in release builds.
>>> In addition to making GetObjectRefType handle NULL properly, various
>>> functions in JNIHandles and OopStorage now have non-NULL handle argument
>>> preconditions and corresponding assertions.
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8195690
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8195690/open.00/
>>> Testing:
>>> Mach5 {hs,jdk}-tier{1,2,3}
>>> Locally ran JCK vm/jni tests
>>> Locally ran tonga vm.runtime tests for jni tests
> 
>