On Wed, May 31, 2023 at 3:16 PM Dan Heidinga <heidi...@redhat.com> wrote:
> On Wed, May 31, 2023 at 7:13 AM Jaroslav Bachorík < > jaroslav.bacho...@datadoghq.com> wrote: > >> Dear Team, >> >> I've been investigating the unusual JVM crashes occurring in JVMTI calls >> on a J9 JVM. During my investigation, I scrutinized the `jmethodID` >> definition closely, available here: [jmethodID definition]( >> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#jmethodID >> ). >> >> To paraphrase, the definition suggests that `jmethodID` identifies a Java >> method, initializer, or constructor. These identifiers, once returned by >> JVM TI functions and events, can be safely stored. However, when the class >> is unloaded, they become invalid, rendering them unsuitable for use. >> > > The "typical" usage pattern is to create a JNI reference (local or global) > to the relevant class and keep that reference for the duration of the > jmethodID's life. If the jmethodID needs to exist past the end of the > local method, then it needs to be accompanied in some way by a global > reference to the class. This works great for JNI as you typically need to > look up the class first before looking up methods. JVMTI is more a > challenge here as it's possible to get the jmethodID without already > holding the class - GetMethodDeclaringClass can help here if the jmethodID > is still valid. > Unfortunately, GetMethodDeclaringClass would crash if the jmethodID is not valid any more, so it really can not be used safely as well. > > >> My interpretation is that the JVMTI user should verify the validity of a >> `jmethodID` value before using it to prevent potential crashes. Would you >> agree with this interpretation? >> > > Mostly. I agree that using an invalid jmethodID will cause crashes but > unfortunately, I'm not aware of any spec'd methods to verify its validity. > > >> >> This sounds like a sensible requirement, but its practical application >> remains unclear. As far as I know, methods can be unloaded concurrently to >> the native code executing JVMTI functions. This introduces a potential race >> condition where the JVM unloads the methods during the check->use flow, >> making it only a partial solution. To complicate matters further, no method >> exists to confirm whether a `jmethodID` is valid. >> >> Theoretically, we could monitor the `CompiledMethodUnload` event to track >> the validity state, creating a constantly expanding set of unloaded >> `jmethodID` values or a bloom filter, if one does not care about few >> potential false positives. This strategy, however, doesn't address the >> potential race condition, and it could even exacerbate it due to possible >> event delays. This delay might mistakenly validate a `jmethodID` value that >> has already been unloaded, but for which the event hasn't been delivered >> yet. >> > > And CompileMethodUnloaded isn't the right event either as the jmethodID > could still be valid. If there was a ClassUnloaded event, and a mapping > from class->jmethodID, it would be possible to invalidate the jmethodIDs > but I'm not aware of any spec'd events that provide those details. > Yes, I see that there are more pieces missing here :( > > >> >> Honestly, I don't see a way to use `jmethodID` safely unless the code >> using it suspends the entire JVM and doesn't resume until it's finished >> with that `jmethodID`. Any other approach might lead to JVM crashes, as >> we've observed with J9. >> >> Lastly, it's noteworthy that Hotspot takes meticulous measures to ensure >> that using jmethodIDs for unloaded methods doesn't crash the JVM and even >> provides useful information. This observation has led me to question >> whether the documentation aligns with the Hotspot implementation, >> especially given that following closely the documentation appears to >> increase the risk associated with the use of `jmethodID` values. >> > > I did a pass through the spec after becoming aware of the crashes you were > seeing on J9 and couldn't find a spec-compliant way to prevent the > crashes. It feels like there's some missing methods in the spec to make > your use case safe. > Indeed. This is all even worse when one would use ASGCT as there is not even a theoretical possibility to create a strong reference to a class holding a particular jmethodID - doing so for every single collected frame would be a performance nightmare, not even mentioning the fact that ASGCT is used from a signal handler where any complex JVMTI operations are simply not safe. -JB- > > --Dan > > >> I welcome your thoughts and perspectives on this matter. >> >> Best regards, >> >> Jaroslav >> >
