On Wed, 25 Oct 2023 10:05:22 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> 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. > > But that just re-introduces the bug? I don't see how we can have both > `ShouldNotReachHere()` in the 'else branch' and return `JNIInvalidRefType` > for unrecognized handles at the same time. > > Not that this function is used to check potentially stale/invalid/corrupt JNI > handles, so the input handle can be complete garbage. Ah, d'uh! Not enough coffee today, sorry. Yes, you are right. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16349#discussion_r1371490180
