RFR: 8313816: Accessing jmethodID might lead to spurious crashes

[Jaroslav Bachorik]

On Tue, 8 Aug 2023 01:51:01 GMT, Andrei Pangin <apangin at openjdk.org> wrote:

>> This is a best effort attempt to harmonize the handling of jmethodIDs when the instanceKlass internal structures are being deallocated (eg. due to `ClassLoaderData::free_deallocate_list`).
>> In all other instances, the jmethodID is NULLed (or set to `_free_method`) before the references Method structures are deallocated. This actually makes possible to query such jmethodIDs without crashing JVM even if the corresponding classes/methods were unloaded.
>> 
>> While it is understandable why JVM can not keep the method metadata around forever it really should be possible to at least assert the validity of a jmethodID without crashing JVM. If nothing else, this makes using the JVMTI functions `GetAllStackTraces` or `GetStackTrace` a risk to use in JVMTI agents - the jmethodids can become invalid the very next moment after the stacktrace is obtained.
>
> src/hotspot/share/oops/instanceKlass.cpp line 2834:
> 
>> 2832:     for (size_t i = 1; i <= count; i++) {
>> 2833:       *((Method**)jmeths[i]) = nullptr;
>> 2834:     }
> 
> The proposed change looks suspicious: the structure being zeroed is released on the very next line.
> So, this change does not seem to fix anything - it only makes a bug (if any) less obvious.
> I would rather do the opposite: poison the memory to make sure any attempt to access freed memory will immediately fail and thus reveal the bug in the code.
> 
> P.S. JVM TI functions do not access this structure; they use jmethodID cache in ClassLoaderData which is cleared but never released, see https://github.com/openjdk/jdk/blob/87b08b6e0192d88025c2275c7dd2c4bdecda58e8/src/hotspot/share/classfile/classLoaderData.cpp#L609C14-L622

Regarding PS - are you sure?
Eg. when I look at the way the stacktrace is obtained at https://github.com/openjdk/jdk/blob/87b08b6e0192d88025c2275c7dd2c4bdecda58e8/src/hotspot/share/prims/jvmtiEnvBase.cpp#L1171, it calls to https://github.com/openjdk/jdk/blob/87b08b6e0192d88025c2275c7dd2c4bdecda58e8/src/hotspot/share/oops/method.cpp#L2196 which in turn calls to https://github.com/openjdk/jdk/blob/87b08b6e0192d88025c2275c7dd2c4bdecda58e8/src/hotspot/share/oops/instanceKlass.cpp#L2247 and then https://github.com/openjdk/jdk/blob/87b08b6e0192d88025c2275c7dd2c4bdecda58e8/src/hotspot/share/oops/instanceKlass.cpp#L2397 

This really seems like the the jmethodID array obtained via `methods_jmethod_ids_acquire()` is the source of truth, at least in this case.

Regarding zeroing the released structure - IIUC, by doing something like `jmethodID mid = jmeths[idx]` the `mid` value (which is in fact a pointer to `Method`) will remain valid even after the holding array has been deallocated.

I have also did a small experiment, just to be sure I am not making a complete fool of myself with following code

mid = jni->GetStaticMethodID(clz, "run", "()V");

if (mid != nullptr) {
  printf("jmethodID points to null: %s\n", *((void**)mid) == nullptr ? "true" : "false");
  jmethodID mid1 = mid;
  *((void**)mid1) = nullptr;
  printf("jmethodID points to null: %s\n", *((void**)mid) == nullptr ? "true" : "false");
}

yielding

jmethodID points to null: false
jmethodID points to null: true


I am running some extra tests now and will switch the PR to draft until I have some results

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15171#discussion_r1286582932