RFR: 8318737: Fallback linker passes bad JNI handle

[Aleksey Shipilev]

On Wed, 25 Oct 2023 09:46:44 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> src/hotspot/share/runtime/jniHandles.cpp line 202:
>> 
>>> 200:       ShouldNotReachHere();
>>> 201:     }
>>> 202:   } else if (is_local_handle(thread, handle) || is_frame_handle(thread, handle)) {
>> 
>> Should we still add `ShouldNotReachHere()` at global `else` branch? This would make the if-else chain exhaustive with the early warning if some handle type is not used. The prior code did this already.
>
> Not sure what you're saying here. As far as I understand the intent of this code is to check whether the handle is of a certain type, and if it's not recognized, return `JNIInvalidRefType`. So, I'm not sure there should be any `ShouldNotReachHere()` in this code.
> 
> We can add an `else` branch that returns `JNIInvalidRefType` though.

The old code would throw `ShouldNotReachHere()` if we did not recognize the handle type:
https://github.com/openjdk/jdk/blob/d2d1592dd94e897fae6fc4098e43b4fffb6d6750/src/hotspot/share/runtime/jniHandles.cpp#L207

I think the new code should still keep it, like so:


 } else if (is_local_handle(thread, handle) || is_frame_handle(thread, handle)) {
   ...
 } else {
   ShouldNotReachHere();
 }


That way, if we ever have another handle type, we would hit the `else` branch instead of silently returning `JNIInvalidRefType`. That is, our condition chain would still be exhaustive, catching unexpected values explicitly.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16349#discussion_r1371481515