# HG changeset patch # User Yasumasa Suenaga # Date 1500717887 -32400 # Node ID 354b87b559efaabb1dff9ab4539b40c79c177103 # Parent 1e7c0fb4048a978554905bff2e5d8144cf9222b6 Bug 3424: HeapStats Agent might crash when thread recorder is disabled via realoading configuration Reviewed-by: ykubota https://github.com/HeapStats/heapstats/pull/112 diff -r 1e7c0fb4048a -r 354b87b559ef ChangeLog --- a/ChangeLog Wed Jul 19 17:53:11 2017 +0900 +++ b/ChangeLog Sat Jul 22 19:04:47 2017 +0900 @@ -1,3 +1,7 @@ +2017-07-22 Yasumasa Suenaga + + * Bug 3424: HeapStats Agent might crash when thread recorder is disabled via realoading configuration + 2017-07-19 Yasumasa Suenaga * Bug 3422: [REFACTORING] Stack and base pointer operation diff -r 1e7c0fb4048a -r 354b87b559ef agent/src/heapstats-engines/callbackRegister.hpp --- a/agent/src/heapstats-engines/callbackRegister.hpp Wed Jul 19 17:53:11 2017 +0900 +++ b/agent/src/heapstats-engines/callbackRegister.hpp Sat Jul 22 19:04:47 2017 +0900 @@ -1,7 +1,7 @@ /*! * \file callbackRegister.hpp * \brief Handling JVMTI callback functions. - * Copyright (C) 2015 Yasumasa Suenaga + * Copyright (C) 2015-2017 Yasumasa Suenaga * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -25,6 +25,8 @@ #include #include +#include + #include /*! @@ -38,6 +40,11 @@ */ static std::list callbackList; + /*! + * \brief Read-Write lock for callback container. + */ + static pthread_rwlock_t callbackLock; + public: /*! * \brief Register JVMTI callback function. @@ -45,7 +52,29 @@ * \param callback [in] Callback function. */ static void registerCallback(T callback) { - callbackList.push_back(callback); + pthread_rwlock_wrlock(&callbackLock); + { + callbackList.push_back(callback); + } + pthread_rwlock_unlock(&callbackLock); + }; + + /*! + * \brief Unregister JVMTI callback function. + * \param callback [in] Callback function to unregister. + */ + static void unregisterCallback(T callback) { + pthread_rwlock_wrlock(&callbackLock); + { + for (auto itr = callbackList.cbegin(); + itr != callbackList.cend(); itr++) { + if (*itr == callback) { + callbackList.erase(itr); + break; + } + } + } + pthread_rwlock_unlock(&callbackLock); }; /*! @@ -83,21 +112,36 @@ template std::list TJVMTIEventCallback::callbackList; -#define ITERATE_CALLBACK_CHAIN(type, ...) \ - for (std::list::iterator itr = callbackList.begin(); \ - itr != callbackList.end(); itr++) { \ - (*itr)(__VA_ARGS__); \ - } +/*! + * \brief Read-Write lock for callback container. + */ +template +pthread_rwlock_t TJVMTIEventCallback::callbackLock = + PTHREAD_RWLOCK_INITIALIZER; + +#define ITERATE_CALLBACK_CHAIN(type, ...) \ + pthread_rwlock_rdlock(&callbackLock); \ + { \ + for (std::list::iterator itr = callbackList.begin(); \ + itr != callbackList.end(); itr++) { \ + (*itr)(__VA_ARGS__); \ + } \ + } \ + pthread_rwlock_unlock(&callbackLock); #define DEFINE_MERGE_CALLBACK(callbackName) \ static void mergeCallback(jvmtiEventCallbacks *callbacks) { \ - if (callbackList.size() == 0) { \ - callbacks->callbackName = NULL; \ - } else if (callbackList.size() == 1) { \ - callbacks->callbackName = *callbackList.begin(); \ - } else { \ - callbacks->callbackName = &callbackStub; \ + pthread_rwlock_rdlock(&callbackLock); \ + { \ + if (callbackList.size() == 0) { \ + callbacks->callbackName = NULL; \ + } else if (callbackList.size() == 1) { \ + callbacks->callbackName = *callbackList.begin(); \ + } else { \ + callbacks->callbackName = &callbackStub; \ + } \ } \ + pthread_rwlock_unlock(&callbackLock); \ }; /*! diff -r 1e7c0fb4048a -r 354b87b559ef agent/src/heapstats-engines/jniCallbackRegister.hpp --- a/agent/src/heapstats-engines/jniCallbackRegister.hpp Wed Jul 19 17:53:11 2017 +0900 +++ b/agent/src/heapstats-engines/jniCallbackRegister.hpp Sat Jul 22 19:04:47 2017 +0900 @@ -1,7 +1,7 @@ /*! * \file jniCallbackRegister.hpp * \brief Handling JNI function callback. - * Copyright (C) 2015 Yasumasa Suenaga + * Copyright (C) 2015-2017 Yasumasa Suenaga * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -25,6 +25,8 @@ #include #include +#include + #include /* Type definition for JNI functions. */ @@ -51,6 +53,11 @@ */ static std::list epilogueCallbackList; + /*! + * \brief Read-Write lock for callback container. + */ + static pthread_rwlock_t callbackLock; + public: /*! * \brief Register callbacks for JNI function. @@ -59,13 +66,49 @@ * \param epilogue [in] Callback after calling JNI function. */ static void registerCallback(T prologue, T epilogue) { - if (prologue != NULL) { - prologueCallbackList.push_back(prologue); + pthread_rwlock_wrlock(&callbackLock); + { + if (prologue != NULL) { + prologueCallbackList.push_back(prologue); + } + + if (epilogue != NULL) { + epilogueCallbackList.push_back(epilogue); + } } + pthread_rwlock_unlock(&callbackLock); + }; - if (epilogue != NULL) { - epilogueCallbackList.push_back(epilogue); + /*! + * \brief Unegister callbacks for JNI function. + * + * \param prologue [in] Callback before calling JNI function. + * \param epilogue [in] Callback after calling JNI function. + */ + static void unregisterCallback(T prologue, T epilogue) { + pthread_rwlock_wrlock(&callbackLock); + { + if (prologue != NULL) { + for (auto itr = prologueCallbackList.cbegin(); + itr != prologueCallbackList.cend(); itr++) { + if (prologue == *itr) { + prologueCallbackList.erase(itr); + break; + } + } + } + + if (epilogue != NULL) { + for (auto itr = epilogueCallbackList.cbegin(); + itr != epilogueCallbackList.cend(); itr++) { + if (epilogue == *itr) { + epilogueCallbackList.erase(itr); + break; + } + } + } } + pthread_rwlock_unlock(&callbackLock); }; }; @@ -81,20 +124,35 @@ template std::list TJNICallbackRegister::epilogueCallbackList; +/*! + * \brief Read-Write lock for callback container. + */ +template +pthread_rwlock_t TJNICallbackRegister::callbackLock = + PTHREAD_RWLOCK_INITIALIZER; + #define ITERATE_JNI_CALLBACK_CHAIN(type, originalFunc, ...) \ std::list::iterator itr; \ \ - for (itr = prologueCallbackList.begin(); itr != prologueCallbackList.end(); \ - itr++) { \ - (*itr)(__VA_ARGS__); \ + pthread_rwlock_rdlock(&callbackLock); \ + { \ + for (itr = prologueCallbackList.begin(); \ + itr != prologueCallbackList.end(); itr++) { \ + (*itr)(__VA_ARGS__); \ + } \ } \ + pthread_rwlock_unlock(&callbackLock); \ \ originalFunc(__VA_ARGS__); \ \ - for (itr = epilogueCallbackList.begin(); itr != epilogueCallbackList.end(); \ - itr++) { \ - (*itr)(__VA_ARGS__); \ - } + pthread_rwlock_rdlock(&callbackLock); \ + { \ + for (itr = epilogueCallbackList.begin(); \ + itr != epilogueCallbackList.end(); itr++) { \ + (*itr)(__VA_ARGS__); \ + } \ + } \ + pthread_rwlock_unlock(&callbackLock); /*! * \brief JVM_Sleep callback (java.lang.System#sleep()) diff -r 1e7c0fb4048a -r 354b87b559ef agent/src/heapstats-engines/threadRecorder.cpp --- a/agent/src/heapstats-engines/threadRecorder.cpp Wed Jul 19 17:53:11 2017 +0900 +++ b/agent/src/heapstats-engines/threadRecorder.cpp Sat Jul 22 19:04:47 2017 +0900 @@ -614,17 +614,12 @@ * \param buf_sz [in] Size of ring buffer. */ void TThreadRecorder::initialize(jvmtiEnv *jvmti, JNIEnv *env, size_t buf_sz) { - static bool isRegistered = false; - if (likely(inst == NULL)) { inst = new TThreadRecorder(buf_sz); - if (!isRegistered) { - isRegistered = true; - registerHookPoint(jvmti, env); - registerJNIHookPoint(env); - registerIOTracer(jvmti, env); - } + registerHookPoint(jvmti, env); + registerJNIHookPoint(env); + registerIOTracer(jvmti, env); inst->registerAllThreads(jvmti); } @@ -694,20 +689,39 @@ */ void TThreadRecorder::finalize(jvmtiEnv *jvmti, JNIEnv *env, const char *fname) { - /* Stop to hook JVMTI event */ + /* Stop JVMTI events which are used by ThreadRecorder only. */ TThreadStartCallback::switchEventNotification(jvmti, JVMTI_DISABLE); TThreadEndCallback::switchEventNotification(jvmti, JVMTI_DISABLE); - TMonitorContendedEnterCallback::switchEventNotification(jvmti, JVMTI_DISABLE); TMonitorContendedEnteredCallback::switchEventNotification(jvmti, JVMTI_DISABLE); TMonitorWaitCallback::switchEventNotification(jvmti, JVMTI_DISABLE); TMonitorWaitedCallback::switchEventNotification(jvmti, JVMTI_DISABLE); - /* Stop to hook JNI function */ + /* Stop JNI function hooks. */ TJVMSleepCallback::switchCallback(env, false); TUnsafeParkCallback::switchCallback(env, false); - /* Stop to hook IoTrace */ + /* Unregister JVMTI event callbacks which are used by ThreadRecorder. */ + TThreadStartCallback::unregisterCallback(&OnThreadStart); + TThreadEndCallback::unregisterCallback(&OnThreadEnd); + TMonitorContendedEnterCallback::unregisterCallback( + &OnMonitorContendedEnterForThreadRecording); + TMonitorContendedEnteredCallback::unregisterCallback( + &OnMonitorContendedEnteredForThreadRecording); + TMonitorWaitCallback::unregisterCallback(&OnMonitorWait); + TMonitorWaitedCallback::unregisterCallback(&OnMonitorWaited); + TDataDumpRequestCallback::unregisterCallback( + &OnDataDumpRequestForDumpThreadRecordData); + + /* Refresh JVMTI event callbacks */ + registerJVMTICallbacks(jvmti); + + /* Unregister JNI function hooks which are used by ThreadRecorder. */ + TJVMSleepCallback::unregisterCallback(&JVM_SleepPrologue, &JVM_SleepEpilogue); + TUnsafeParkCallback::unregisterCallback(&UnsafeParkPrologue, + &UnsafeParkEpilogue); + + /* Stop IoTrace hook */ UnregisterIOTracer(env); /* Wait until all tasks are finished. */