From 12543e037838169098b7624b2dad6c3d6026ea92 Mon Sep 17 00:00:00 2001 From: Sudheer Vinukonda Date: Mon, 18 May 2020 16:35:48 -0700 Subject: [PATCH 1/4] Add TXN_CLOSE hook to CPPAPI TransactionPlugin --- include/tscpp/api/Plugin.h | 10 ++++++++++ src/tscpp/api/utils_internal.cc | 9 +++++++++ 2 files changed, 19 insertions(+) diff --git a/include/tscpp/api/Plugin.h b/include/tscpp/api/Plugin.h index 2f57352cbe0..0b0aa62f7ce 100644 --- a/include/tscpp/api/Plugin.h +++ b/include/tscpp/api/Plugin.h @@ -58,6 +58,7 @@ class Plugin : noncopyable HOOK_READ_REQUEST_HEADERS, /**< This hook will be fired after the request is read. */ HOOK_READ_CACHE_HEADERS, /**< This hook will be fired after the CACHE hdrs. */ HOOK_CACHE_LOOKUP_COMPLETE, /**< This hook will be fired after cache lookup complete. */ + HOOK_TXN_CLOSE, /**< This hook will be fired after send response headers. */ HOOK_SELECT_ALT /**< This hook will be fired after select alt. */ }; @@ -142,6 +143,15 @@ class Plugin : noncopyable transaction.resume(); }; + /** + * This method must be implemented when you hook HOOK_TXN_CLOSE + */ + virtual void + handleTxnClose(Transaction &transaction) + { + transaction.resume(); + }; + /** * This method must be implemented when you hook HOOK_SELECT_ALT */ diff --git a/src/tscpp/api/utils_internal.cc b/src/tscpp/api/utils_internal.cc index 7cb86e0941b..b00627bbe7f 100644 --- a/src/tscpp/api/utils_internal.cc +++ b/src/tscpp/api/utils_internal.cc @@ -141,6 +141,13 @@ void inline invokePluginForEvent(Plugin *plugin, TSHttpTxn ats_txn_handle, TSEve case TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE: plugin->handleReadCacheLookupComplete(transaction); break; + case TS_EVENT_HTTP_TXN_CLOSE: + if (plugin) { + plugin->handleTxnClose(transaction); + } else { + LOG_ERROR("stray event TS_EVENT_HTTP_TXN_CLOSE, no transaction plugin to handle it!"); + } + break; default: assert(false); /* we should never get here */ break; @@ -191,6 +198,8 @@ utils::internal::convertInternalHookToTsHook(Plugin::HookType hooktype) return TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK; case Plugin::HOOK_SELECT_ALT: return TS_HTTP_SELECT_ALT_HOOK; + case Plugin::HOOK_TXN_CLOSE: + return TS_HTTP_TXN_CLOSE_HOOK; default: assert(false); // shouldn't happen, let's catch it early break; From fe47417b4147dcfb378b430998a8f4576e2e29b4 Mon Sep 17 00:00:00 2001 From: Sudheer Vinukonda Date: Mon, 18 May 2020 21:42:14 -0700 Subject: [PATCH 2/4] Clean up TransactionPlugin object and associated Continuation in txn_close --- src/tscpp/api/utils_internal.cc | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/tscpp/api/utils_internal.cc b/src/tscpp/api/utils_internal.cc index b00627bbe7f..61f90446aca 100644 --- a/src/tscpp/api/utils_internal.cc +++ b/src/tscpp/api/utils_internal.cc @@ -49,6 +49,25 @@ resetTransactionHandles(Transaction &transaction, TSEvent event) return; } +void +cleanupTransaction(Transaction &transaction, TSHttpTxn ats_txn_handle) +{ + delete &transaction; + // reset the txn arg to prevent use-after-free + TSUserArgSet(ats_txn_handle, TRANSACTION_STORAGE_INDEX, nullptr); +} + +void +cleanupTransactionPlugin(Plugin *plugin) +{ + TransactionPlugin *transaction_plugin = static_cast(plugin); + std::shared_ptr trans_mutex = utils::internal::getTransactionPluginMutex(*transaction_plugin); + LOG_DEBUG("Locking TransactionPlugin mutex to delete transaction plugin at %p", transaction_plugin); + trans_mutex->lock(); + delete transaction_plugin; + trans_mutex->unlock(); +} + int handleTransactionEvents(TSCont cont, TSEvent event, void *edata) { @@ -77,14 +96,9 @@ handleTransactionEvents(TSCont cont, TSEvent event, void *edata) resetTransactionHandles(transaction, event); const std::list &plugins = utils::internal::getTransactionPlugins(transaction); for (auto plugin : plugins) { - std::shared_ptr trans_mutex = utils::internal::getTransactionPluginMutex(*plugin); - LOG_DEBUG("Locking TransactionPlugin mutex to delete transaction plugin at %p", plugin); - trans_mutex->lock(); - LOG_DEBUG("Locked Mutex...Deleting transaction plugin at %p", plugin); - delete plugin; - trans_mutex->unlock(); + cleanupTransactionPlugin(plugin); } - delete &transaction; + cleanupTransaction(transaction, ats_txn_handle); } break; default: assert(false); /* we should never get here */ @@ -144,9 +158,11 @@ void inline invokePluginForEvent(Plugin *plugin, TSHttpTxn ats_txn_handle, TSEve case TS_EVENT_HTTP_TXN_CLOSE: if (plugin) { plugin->handleTxnClose(transaction); + cleanupTransactionPlugin(plugin); } else { LOG_ERROR("stray event TS_EVENT_HTTP_TXN_CLOSE, no transaction plugin to handle it!"); } + cleanupTransaction(transaction, ats_txn_handle); break; default: assert(false); /* we should never get here */ From f4ef086477e532c46c0dbec1da05b8d474dcb7e9 Mon Sep 17 00:00:00 2001 From: Sudheer Vinukonda Date: Tue, 19 May 2020 12:59:15 -0700 Subject: [PATCH 3/4] Address review comments --- include/tscpp/api/Plugin.h | 4 ++-- src/tscpp/api/GlobalPlugin.cc | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/tscpp/api/Plugin.h b/include/tscpp/api/Plugin.h index 0b0aa62f7ce..f37b8050e17 100644 --- a/include/tscpp/api/Plugin.h +++ b/include/tscpp/api/Plugin.h @@ -58,8 +58,8 @@ class Plugin : noncopyable HOOK_READ_REQUEST_HEADERS, /**< This hook will be fired after the request is read. */ HOOK_READ_CACHE_HEADERS, /**< This hook will be fired after the CACHE hdrs. */ HOOK_CACHE_LOOKUP_COMPLETE, /**< This hook will be fired after cache lookup complete. */ - HOOK_TXN_CLOSE, /**< This hook will be fired after send response headers. */ - HOOK_SELECT_ALT /**< This hook will be fired after select alt. */ + HOOK_TXN_CLOSE, /**< This hook will be fired after send response headers, only for TransactionPlugins::registerHook()!. */ + HOOK_SELECT_ALT /**< This hook will be fired after select alt. */ }; /** diff --git a/src/tscpp/api/GlobalPlugin.cc b/src/tscpp/api/GlobalPlugin.cc index b1be2302a1f..8e5f05c2e7e 100644 --- a/src/tscpp/api/GlobalPlugin.cc +++ b/src/tscpp/api/GlobalPlugin.cc @@ -87,6 +87,7 @@ GlobalPlugin::~GlobalPlugin() void GlobalPlugin::registerHook(Plugin::HookType hook_type) { + assert(hook_type != Plugin::HOOK_TXN_CLOSE); TSHttpHookID hook_id = utils::internal::convertInternalHookToTsHook(hook_type); TSHttpHookAdd(hook_id, state_->cont_); LOG_DEBUG("Registered global plugin %p for hook %s", this, HOOK_TYPE_STRINGS[hook_type].c_str()); From c7f1637afddfaef9446bd435ee7219663dbeb5f8 Mon Sep 17 00:00:00 2001 From: Sudheer Vinukonda Date: Tue, 19 May 2020 14:25:03 -0700 Subject: [PATCH 4/4] More review comments --- include/tscpp/api/TransactionPlugin.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/tscpp/api/TransactionPlugin.h b/include/tscpp/api/TransactionPlugin.h index b34fba01f8b..ce3f1ca4419 100644 --- a/include/tscpp/api/TransactionPlugin.h +++ b/include/tscpp/api/TransactionPlugin.h @@ -93,6 +93,10 @@ class TransactionPlugin : public Plugin * see HookType and Plugin for the correspond HookTypes and callback methods. If you fail to implement the * callback, a default implementation will be used that will only resume the Transaction. * + * \note For automatic destruction, you must either register dynamically allocated instances of + * classes derived from this class with the the corresponding Transaction object (using + * Transaction::addPlugin()), or register HOOK_TXN_CLOSE (but not both). + * * @param HookType the type of hook you wish to register * @see HookType * @see Plugin