Superceding in one function prevents execution of post-callbacks up the call chain

[nosoop]

ℹ️ This should no longer be an issue if you're using @peace-maker's version with detour support, but the issue will remain open as long as the issue is valid upstream.

---

Versions installed:

• SourceMod version: 1.10.0.6452
• Metamod:Source version: 1.11.0-dev+1127
• DHooks version: 2.2.0-detours7 (does affect mainline dhooks iirc)
• Platform: Linux

Hard to explain, so here's an example case.

Attached below is a plugin that uses a virtual hook on CBaseCombatWeapon::PrimaryAttack() and a post-hook on PostThink via SDKHooks. If the hook on ::PrimaryAttack() returns MRES_Supercede, the PostThinkPost callback never executes.

To reproduce, install the plugin, set test_supercede_bug to 1, and shoot gun. The expected result is that the SourceMod continues to call PostThink with the latest game time while attacking; the current behavior is that no messages are displayed.

Only TF2 gamedata is provided, but the calls aren't specific to TF2.

test_dhook_think_func.zip

[peace-maker]

This might be related to 44289ce which was reverted in 69b433c for some reason.

iirc @BotoX debugged and reported some issue which lead to the first commit. Don't know what crash it was causing when it was removed again.

[BotoX]

The "fix crash" commit was causing some hooks to leak memory like crazy.
Napalm lag fix for example.
It'd fill up some linked list in SourceHook to infinity causing huuuge lagspikes on unhook (traversing a linked list with a million entries..)

[peace-maker]

It's weird though. EndContext should by called by the hookmanager generator in sourcehook. Maybe there's a bug in SourceHook or we're using it wrong, but I don't think calling EndContext ourself is the right fix?

https://github.com/alliedmodders/metamod-source/blob/31be74561d8a022bb69ea1b22bffaaa138e7fb23/core/sourcehook/sourcehook_hookmangen.cpp#L1312

[BotoX]

I think you might be right, I was only seeing the memory leak with the gamerules radiushurt (napalm lag fix) hook.
All other dhooks weren't leaking, so clearly something was calling EndContext there.
Either way I'd rather call it twice than have to traverse a linked list with hundred thousand entries on every unhook 🔥

[peace-maker]

But popping the stack more than you pushed it won't work.

[BotoX]

True, EndContext calls: m_ContextStack.pop(); which does --m_UsedSize;.
And m_UsedSize is a size_t.
So if it was calling EndContext too much we'd definitely see crashes or other broken behavior.
But everything is fine for me with all of those EndContext enabled.
Absolutely no issue with this since 9 months: https://git.botox.bz/CSSZombieEscape/sm-ext-dhooks2/commit/0e6936a7ee41a48c0abd9e5eb52071536b6999a4
Also before that, because the precompiled dhooks we were using before switching to SM 1.10 had that code enabled.
As the issue only appeared after upgrading to SM 1.10 and compiling dhooks ourself.

[peace-maker]

You're talking about this plugin which caused the context stack to explode on your server, right?

I think the issue nosoop is describing comes from the context stack not being popped and the MRES_SUPERCEDE return getting reused by the parent hook. It's weird, that it skips the post callback even after calling the original function that way. I guess only some nasty debugging will shed some light here. I don't have any time for this right now.

[BotoX]

You're talking about this plugin which caused the context stack to explode on your server, right?

Yeah, we had a good laugh too because a "Lag Fix" plugin was causing extreme server lag :^)

@nosoop can you compile dhooks yourself?
Although I have no clue about this bug you could test this repo: https://git.botox.bz/CSSZombieEscape/sm-ext-dhooks2/

[nosoop]

@BotoX Can do. Tried your repo compiled against 1.10 with the development instance described in the initial post (with a large number of dhooks/detour-using plugins) and it immediately crashes on player spawn, accelerator dumps 1, 2.

2.2.0-detours14a doesn't crash on spawn, but I can verify that it is also affected by the supercede bug.

That's all from a cursory test; if you'd like any further actions taken let me know and I can look into it more when time permits.

[peace-maker]

@nosoop Can you test with only the virtual hook plugin attached in the first comment loaded? That crash is from a detour which uses a completely different system from SourceHook which is used by sdkhooks and dhooks alike.

[nosoop]

In hindsight I should've tested that sooner, yeah.

It looks like I indeed can't reproduce the supercede issue on a build from that repo, nor can I reproduce it on detours14a with an extra commit to revert 69b433c.

The latter also works on my plugin-filled development instance, though I'm not thoroughly testing for regressions there.

[peace-maker]

This was fixed in peace-maker@314f926. It appears to be a misuse of SourceHook's API on our side. We're pushing an extra context onto the context stack when we shouldn't on supercede of non vector or float returns. So adding a bunch of EndContext calls accidentally fixed the issue, but caused problems on every other case of using dhooks while not superceding the original function.

[nosoop]

Closing this since it's fixed in the version of DHooks bundled with SourceMod.