diff --git a/core/metamod_console.cpp b/core/metamod_console.cpp index 6b789b5..b747628 100644 --- a/core/metamod_console.cpp +++ b/core/metamod_console.cpp @@ -428,6 +428,12 @@ bool Command_Meta(IMetamodSourceCommandInfo *info) bool already; CPluginManager::CPlugin *pl; + // If we've recently tried to unload plugins, they might still + // be in the unload queue. Force them out now. This is not + // lowered to CPluginManager because it's not strictly safe + // there. + g_SourceHook.ResolvePendingUnloads(true); + PluginId id = g_PluginMngr.Load(full_path, Pl_Console, already, error, sizeof(error)); pl = g_PluginMngr.FindById(id); if (!pl || id < Pl_MinId || (pl->m_Status < Pl_Paused)) diff --git a/core/metamod_plugins.cpp b/core/metamod_plugins.cpp index a70b7be..20b9212 100644 --- a/core/metamod_plugins.cpp +++ b/core/metamod_plugins.cpp @@ -382,6 +382,36 @@ const char *CPluginManager::GetStatusText(CPlugin *pl) } } +struct Unloader : public SourceHook::Impl::UnloadListener +{ + CPluginManager::CPlugin *plugin_; + bool destroy_; + + Unloader(CPluginManager::CPlugin *plugin, bool destroy) + : plugin_(plugin), destroy_(destroy) + { } + + void ReadyToUnload(SourceHook::Plugin plug) + { + if (plugin_->m_UnloadFn != NULL) + plugin_->m_UnloadFn(); + + dlclose(plugin_->m_Lib); + + if (destroy_) + { + delete plugin_; + } + else + { + plugin_->m_Lib = NULL; + plugin_->m_API = NULL; + } + + delete this; + } +}; + CPluginManager::CPlugin *CPluginManager::_Load(const char *file, PluginId source, char *error, size_t maxlen) { FILE *fp; @@ -552,17 +582,8 @@ CPluginManager::CPlugin *CPluginManager::_Load(const char *file, PluginId source if (pl->m_Lib && (pl->m_Status < Pl_Paused)) { pl->m_Events.clear(); - g_SourceHook.UnloadPlugin(pl->m_Id); UnregAllConCmds(pl); - - if (pl->m_UnloadFn != NULL) - { - pl->m_UnloadFn(); - } - - dlclose(pl->m_Lib); - pl->m_Lib = NULL; - pl->m_API = NULL; + g_SourceHook.UnloadPlugin(pl->m_Id, new Unloader(pl, false)); } return pl; @@ -580,23 +601,9 @@ bool CPluginManager::_Unload(CPluginManager::CPlugin *pl, bool force, char *erro //Note, we'll always tell the plugin it will be unloading... if (pl->m_API->Unload(error, maxlen) || force) { - //Make sure to detach it from sourcehook! - g_SourceHook.UnloadPlugin(pl->m_Id); - pl->m_Events.clear(); - UnregAllConCmds(pl); - if (pl->m_UnloadFn != NULL) - { - pl->m_UnloadFn(); - } - - //Clean up the DLL - dlclose(pl->m_Lib); - pl->m_Lib = NULL; - pl->m_API = NULL; - //Remove the plugin from the list PluginIter i; for (i=m_Plugins.begin(); i!=m_Plugins.end(); i++) @@ -607,9 +614,9 @@ bool CPluginManager::_Unload(CPluginManager::CPlugin *pl, bool force, char *erro break; } } - //Free its memory - delete pl; + //Make sure to detach it from sourcehook! + g_SourceHook.UnloadPlugin(pl->m_Id, new Unloader(pl, true)); return true; } } else { diff --git a/core/sourcehook/sourcehook.cpp b/core/sourcehook/sourcehook.cpp index 58f8086..8b66b2c 100644 --- a/core/sourcehook/sourcehook.cpp +++ b/core/sourcehook/sourcehook.cpp @@ -1,4 +1,5 @@ /* ======== SourceHook ======== +* vim: set ts=4 sw=4 tw=99 noet: * Copyright (C) 2004-2010 Metamod:Source Development Team * No warranties of any kind * @@ -357,7 +358,7 @@ namespace SourceHook return m_ContextStack.front().pOverrideRet; } - void CSourceHookImpl::UnloadPlugin(Plugin plug) + void CSourceHookImpl::UnloadPlugin(Plugin plug, UnloadListener *listener) { // 1) Remove all hooks by this plugin @@ -375,6 +376,20 @@ namespace SourceHook else ++iter; } + + // Fix for bug 5034. It's pretty tricky to find whether hookmanagers owned by this + // plugin are active. We could change how hook managers are tracked in SH, and lazily + // free them as the context stack drops to 0, or we could change the pubfunc API to + // know whether it's active or not. Rather than deal with this extra complexity, we + // just conservatively wait until the context stack hits 0 before unloading. + if (m_ContextStack.size() == 0) + { + listener->ReadyToUnload(plug); + } + else + { + m_PendingUnloads.push_back(new PendingUnload(listener, plug)); + } } CHookManList::iterator CSourceHookImpl::RemoveHookManager(CHookManList::iterator hookman_iter) @@ -571,12 +586,41 @@ namespace SourceHook return pCtx; } + void CSourceHookImpl::ResolvePendingUnloads(bool force) + { + List::iterator iter = m_PendingUnloads.begin(); + while (iter != m_PendingUnloads.end()) + { + PendingUnload *unload = *iter; + + if (!force && !unload->deactivated()) + { + // Unless being forced, wait one drop of the context stack + // before actually unloading. Otherwise, we'll still return + // to unloaded memory. + unload->deactivate(); + iter++; + } + else + { + unload->listener()->ReadyToUnload(unload->plugin()); + delete unload; + iter = m_PendingUnloads.erase(iter); + } + } + } + void CSourceHookImpl::EndContext(IHookContext *pCtx) { // Do clean up task, if any is associated with this context m_ContextStack.front().DoCleanupTaskAndDeleteIt(); // Then remove it m_ContextStack.pop(); + + // If we've reached 0 contexts and there are pending unloads, + // resolve them now. + if (m_ContextStack.size() == 0 && m_PendingUnloads.size() != 0) + ResolvePendingUnloads(); } void CSourceHookImpl::CompleteShutdown() diff --git a/core/sourcehook/sourcehook_impl.h b/core/sourcehook/sourcehook_impl.h index cf5579e..f9ef068 100644 --- a/core/sourcehook/sourcehook_impl.h +++ b/core/sourcehook/sourcehook_impl.h @@ -1,4 +1,5 @@ /* ======== SourceHook ======== +* vim: set ts=4 sw=4 tw=99 noet: * Copyright (C) 2004-2010 Metamod:Source Development Team * No warranties of any kind * @@ -254,7 +255,43 @@ namespace SourceHook }; typedef CStack HookContextStack; - + + class UnloadListener + { + public: + virtual void ReadyToUnload(Plugin plug) = 0; + }; + + class PendingUnload + { + UnloadListener *listener_; + Plugin plug_; + bool deactivated_; + + public: + PendingUnload(UnloadListener *listener, Plugin plug) + : listener_(listener), plug_(plug), deactivated_(false) + { } + + Plugin plugin() const + { + return plug_; + } + + UnloadListener *listener() const + { + return listener_; + } + + void deactivate() + { + deactivated_ = true; + } + bool deactivated() const + { + return deactivated_; + } + }; class CSourceHookImpl : public ISourceHook { @@ -263,6 +300,7 @@ namespace SourceHook CVfnPtrList m_VfnPtrs; CHookIDManager m_HookIDMan; HookContextStack m_ContextStack; + List m_PendingUnloads; bool SetHookPaused(int hookid, bool paused); CHookManList::iterator RemoveHookManager(CHookManList::iterator iter); @@ -304,10 +342,16 @@ namespace SourceHook void *GetOverrideRetPtr(); //!< Used for setting the override return value - /** - * @brief Make sure that a plugin is not used by any other plugins anymore, and unregister all its hook managers - */ - void UnloadPlugin(Plugin plug); + /* + * @brief Make sure that a plugin is not used by any + * other plugins anymore, and unregister all its hook + * managers. If any hooks owned by this plugin are + * still on the callstack, defers notifying the listener + * until the count has dropped to 0. + */ + void UnloadPlugin(Plugin plug, UnloadListener *listener); + + void ResolvePendingUnloads(bool force = false); void RemoveHookManager(Plugin plug, HookManagerPubFunc pubFunc);