DHooks might crash on plugin reload

[Adrianilloo]

Help us help you

• I have checked that my issue doesn't exist yet.
• I can always reproduce the issue with the provided description below.

Environment

• Operating System version: Ubuntu 20.04.3 LTS
• Game/AppID (with version if applicable): Half-Life 2: Deathmatch / 320
• Current SourceMod version: 1.11
• Current SourceMod snapshot: 6842
• Current Metamod: Source snapshot: 1.11.0-dev+1145

Description

DHooks2 showcases a possible DHooksCallback::plugin_callback NULL pointer de-reference crash sequence when plugins that use it (properly setting hook at OnPluginStart and having double-checked gamedata) get reloaded. Concretely I repeatedly recompile one of them and have it reloaded by restarting map (doing both steps once each new map session), rather than via sm plugins reload (which I didn't test yet). The crash has enough chance to happen each map reload.

Reference plugin.

Recompiled DHooks info

I recompiled DHooks with some debug logging stataments (targetting error logs, for my own convenience) showing relevant related data (callback addresses, offsets, etc.), manually preventing the crash as well, and resulting logs are attached below. Here are the exact testing changes:

+++ b/extensions/dhooks/dynhooks_sourcepawn.cpp
@@ -472,6 +472,8 @@ ReturnAction_t HandleDetour(HookType_t hookType, CHook* pDetour)
 
 CDynamicHooksSourcePawn::CDynamicHooksSourcePawn(HookSetup *setup, CHook *pDetour, IPluginFunction *pCallback, bool post)
 {
+       g_pSM->LogError(myself, "CDynamicHooksSourcePawn called for this = %p with pCallback = %p (plugin callback)",
+               this, pCallback);
        this->params = setup->params;

+++ b/extensions/dhooks/listeners.cpp
@@ -75,6 +75,9 @@ void DHooksEntityListener::CleanupListeners(IPluginContext *pContext)
                IPluginFunction *cb = manager->callback->plugin_callback;
                if (pContext == NULL || (cb && pContext == cb->GetParentRuntime()->GetDefaultContext()))
                {
+                       g_pSM->LogError(myself, "DHooksEntityListener::CleanupListeners cleaning DHooksCallback at %p"
+                               " (plugin_callback = %p; offset = %i)", manager->callback, manager->callback->plugin_callback,
+                               manager->callback->offset);
                        manager->callback->plugin_callback = nullptr;

+++ b/extensions/dhooks/vhook.cpp
@@ -148,7 +148,8 @@ DHooksManager::DHooksManager(HookSetup *setup, void *iface, IPluginFunction *rem
        this->callback->post = post;
        this->callback->hookType = setup->hookType;
        this->callback->params = setup->params;

+       g_pSM->LogError(myself, "DHooksManager constructor of instance %p called! (callback = %p; plugincb = %p; offset = %i)",
+               this, this->callback, plugincb, setup->offset);

@@ -411,6 +412,12 @@ void *Callback(DHooksCallback *dg, void **argStack)
 
        if(dg->thisType == ThisPointer_CBaseEntity || dg->thisType == ThisPointer_Address)
        {
+               if (dg->plugin_callback == NULL)
+               {
+                       g_pSM->LogError(myself, "dg->plugin_callback was NULL!!! (dg = %p; offset = %i)", dg, dg->offset);
+                       return NULL;
+               }
+
                dg->plugin_callback->PushCell(GetThisPtr(g_SHPtr->GetIfacePtr(), dg->thisType));
        }
        if(dg->returnType != ReturnType_Void)

Important: notice how the logs show several map reloads and after one happens, the dg->plugin_callback was NULL!!! log arises, and its dg object has been cleared early at DHooksEntityListener::CleanupListeners.

It seems to me that the calling stack that triggers this is the following:

DHooksManager::DHooksManager -> MakeHandler(ReturnType type) -> GenerateThunk(ReturnType type) -> Callback(DHooksCallback *dg, void **argStack)

Notice that at DHooksManager, the plugin_callback is only assigned after the MakeHandler call.

Logs

errors_20220107.log
crash_j6efozxcjard.zip

[asherkin]

Last time this was discussed you had a 2nd plugin using DHooks loaded with a likely faulty hook setup, and the crash report indicates that plugin was loaded at the time of that crash. Can I just confirm you've reproduce this with only a single, known-good, DHooks-using plugin running?

[Adrianilloo]

Yes, I already tested disabling that plugin (for which I had correctly filled remaining gamedata anyway, here), and, in fact, running with the minimum possible MM and SM plugins + exts and could reproduce same crash. I've just double-checked it (with minimal dependencies, and just this plugin as DHooks one). Identical stacktrace generated.

[peace-maker]

Without looking at the crash dump yet, this commit might be relevant.
peace-maker/DHooks2@aca054f

Hopefully no hook is fired on the same frame after entity removal.. Will have to revisit in that case.

[Adrianilloo]

Ok, here is a very important update.

My server is running SourceTV. If I disable it, the crash vanishes (couldn't reproduce anymore the reload crashes for 40-50 times, ~10 total minutes during same server session). As I guessed initially, TV probably influences it due to its early spawning nature each map restart. This also means sm plugins reload shouldn't trigger the fault, neither.

My TV relevant configs:

autoexec.cfg:

exec tv
tv_enable 1

tv.cfg:

tv_name "AdRiAnIlloO`s TV"

server.cfg:

tv_delay         0
tv_transmitall 1

(There are some file splits for organization reasons, a I run different servers reusing files with symlinks)

[peace-maker]

Could you link a new crash report on crash.limetech.org? It accelerates the debugging process.

[Adrianilloo]

Sorry, deleted last crash dump again accidentally. Here is new one: https://crash.limetech.org/j7nrodanp2lb

crash_j7nrodanp2lb.zip

[Kenzzer]

When #2403 lands this should be fixed.