No way of knowing when it's safe to unload a library with a hook

[m417z]

Let's say I load a dll which hooks MessageBoxW. At some point, I need to unload the dll (e.g. to exit or update). SlimDetours offers no way of knowing when it's safe do to so. Given the chain of MessageBoxW calls:

1. app.exe: calls MessageBoxW
2. SlimDetours.dll: MessageBoxW_rbCodeIn (trampoline)
3. hook.dll: MessageBoxW_Hook
4. SlimDetours.dll: MessageBoxW_rbCode (first instructions of the original MessageBoxW function)
5. user32.dll: MessageBoxW
6. hook.dll: MessageBoxW_Hook (continuation of the hook)
7. app.exe: MessageBoxW returns to app.exe

If a thread is running in any of the 1-6 stages, unloading the dll will cause a crash.

One way the library could mitigate this is to use a reference counter, something like:

volatile LONG refCount = 0;

int MessageBoxW_Hook(...) {
    InterlockedIncrement(&refCount);
    // ...
    int result = MessageBoxW_Original(...);
    // ...
    InterlockedDecrement(&refCount);
    return result;
}

void Unload() {
    while (InterlockedCompareExchange(&refCount, 0, 0) > 0) {
        Sleep(10);
    }
    FreeLibraryAndExitThread(hModule, 0);
}

But it puts the burden on the library user. It's also not a fully correct solution, as a thread's instruction pointer could be just before InterlockedIncrement or just after InterlockedDecrement.

One solution I thought of is to perpend code similar to the following to rbCodeIn (pseudo-code):

    if (!trampoline.TLS->originalReturnAddress) {
        pop trampoline.TLS->originalReturnAddress;
        call *trampoline.pbDetour;
        jmp rbCodeOut;
    }
    jmp *trampoline.pbDetour;

rbCodeOut:
    push trampoline.TLS->originalReturnAddress;
    trampoline.TLS->originalReturnAddress = NULL;
    ret;

Then add a new function as follows:

bool SlimDetoursIsSafeToUnload() {
    bool safeToUnload = true;
    SuspendThreads();
    for (auto thread : threads) {
        if (thread->TLS->originalReturnAddress || IsRipInTrampoline(thread->rip)) {
            safeToUnload = false;
            break;
        }
    }
    ResumeThreads();
    return safeToUnload;
}

But it's not a quick fix. Let me know if you have better ideas.

[RatinCN]

Yes indeed, this is an epic puzzle. As far as I know, the most safe way in practice is never unhook and unload, let detour function do nothing instead:

int MessageBoxW_Hook(...) {
    if (g_Enable) { ... }
    int result = MessageBoxW_Original(...);
    if (g_Enable) { ... }
    return result;
}

I thought about this problem before and thought of the same solution as you 🤷‍♂️. And this solution will replace the return address in *RSP, may break some mechanisms (e.g. stack walk, security check, ...). This ticket will be converted to discussion and wait for more ideas.

[RatinCN]

Have you @m417z encountered a scenario where unload is important? This solution is feasible and should be optional, to ensure users who never unhook and unload (this is the most stable and safe way as far as I know) comfortable as before.

[m417z]

may break some mechanisms (e.g. stack walk, security check, ...)

Yes, I also thought about it.

Regarding security and the shadow stack, I updated the pseudo-code to a variant which keeps the call/ret operations consistent with the shadow stack. I didn't test it, but I think it should be compatible. It's a mix of C and assembly, so let me know if the idea isn't clear.

Regarding stack walk, I'm not sure what will be affected except for diagnostic tools such as debuggers, procmon, etc.

Another problem I thought of (maybe you meant it too) is if an exception is thrown in the hook or original, and is handled outside of it, then SlimDetoursIsSafeToUnload will become stuck in returning false. Perhaps it'd be possible to add a finally-like handler to make sure the rbCodeOut part is always executed, but it complicates the solution even more.

Have you @m417z encountered a scenario where unload is important?

Yes, and I encountered such crashes in practice. Two specific examples are the first two projects from my GitHub profile:

• 7 Taskbar Tweaker, Windows Taskbar Customization Tool - It allows customizing the Windows taskbar, and as part of it, several functions are hooked. It can be closed or updated, in which case the hooks are removed. I'm using the consumer refcount mitigation I mentioned.
• Windhawk, the customization marketplace for Windows programs - It allows writing customization mods for Windows, which can place hooks, and can be loaded and unloaded dynamically. Here's a simple example. In this case, it's undesirable to force all mod developers to use their own refcout, and it's prone to mistakes. It'd be much better for Windhawk itself to take care of it safely, and for that, it needs the help of the hooking library it uses.

[namazso]

I made a PoC of using an extra frame, an unwind handler, and reference counting:

https://github.com/namazso/hooktest

Build with clang-cl in release.

A cleaner version of this could be implemented for reliable unhooking

[m417z]

Thanks for sharing. Some thoughts from a library perspective:

• As far as I understand, seh_stackalloc instructs to add the region to the PE unwind tables, which is not something that can be done in runtime. The hooking library can prepare a fixed amount of such jmp_target2s, which will impose a limitation on the amount of hooks, or somehow make the lib consumer use their unwind tables, maybe via a macro/template.
• The int _0, int _1, int _2, int _3, int _4, int _5 part is a nice trick, but it's also something that leaks to the lib consumer. Can also be solved with a macro/template perhaps.

I can imagine how the technique you demonstrated can be turned into a hooking library with the help of macros/templates, and it'd be great if such library existed. But I don't see how it's possible to apply it and still keep the current API, which is roughly:

void* original_copied;

void hook_me(int a, int b, int c, int d, int e) {
    printf("%d, %d, %d, %d, %d\n", a, b, c, d, e);
}

void hooked_hook_me(int a, int b, int c, int d, int e) {
    typedef void(*ofn)(int, int, int, int, int);
    ofn o = (ofn)original_copied;
    if (a == 123)
        __debugbreak();
    o(a + 1, b, c, d, e);
}

int main(void) {
    hook_me(1, 2, 3, 4, 5);
    DetoursHook(hook_me, hooked_hook_me, &original_copied);
    hook_me(1, 2, 3, 4, 5);
    __try {
        hook_me(123, 2, 3, 4, 5);
    } __except(EXCEPTION_EXECUTE_HANDLER) {}
    hook_me(1, 2, 3, 4, 5);
    return 0;
}

Even with the help of macros, I hardly believe that it's possible to do something here without introducing new syntax, as what's missing here is having a wrapper for hooked_hook_me on the consumer's side.

My original comment tried to come up with a way to create such a wrapper solely with code that's generated by the hooking library.

So if preserving ABI (or even only source code) is a requirement, I still see no other solution except for the two I mentioned here:

• Storing the ref count in TLS for the outermost call. A complicated solution. No support for cross-hook exceptions. May interfere with stack walking and perhaps with other things such as security features.
• Checking the call stack for all threads. Puts extra burden on the lib consumer. Affects the whole module unless the exact hook function region is known. Stack walking may fail in some cases.

For my current need (Windhawk), the stack walking solution works best given the options, but for a library (given that API is to be preserved) the first option is IMO a better fit, but it's not explored enough to say if it's a viable solution.

[namazso]

Registering unwind tables at runtime is absolutely a thing, see https://learn.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-rtladdfunctiontable

But indeed keeping the ABI is hard without limiting the amount of parameters by copying each one of them. A C++ template is the closest that can be done to at least keep API.

[m417z]

Now that I think about it, if stack walking is reliable (I'm not sure it is in all platforms), maybe it's enough to have the following:

bool IsSafeToUnload(HMODULE* modulesWithHooks, ULONG modulesWithHooksCount) {
    bool safeToUnload = true;
    SuspendThreads();
    // Iterate all threads but the current one.
    for (auto thread : threads) {
        if (IsRipInTrampoline(thread->rip)) {
            safeToUnload = false;
            goto done;
        }
        for (auto address : GetCallStack(thread)) {
            for (HMODULE module : {modulesWithHooks, modulesWithHooksCount}) {
                if (AddressInModule(address, module)) {
                    safeToUnload = false;
                    goto done;
                }
            }
        }
    }
done:
    ResumeThreads();
    return safeToUnload;
}

[m417z]

That approach seemed promising to me, but just yesterday (what are the chances?) Raymond Chen published a blog post demonstrating that it may cause deadlocks:
https://devblogs.microsoft.com/oldnewthing/20250411-00/?p=111066

[RatinCN]

Stack trace may also translates return address to symbol name, if return address is changed, information displayed in "Call Stack" window (VS, WinDbg, ...) may be useless, but I didn't verify this point.

[m417z]

I actually just updated the repo with a solution to the deadlock problem:
https://github.com/m417z/thread-call-stack-scanner
The solution is described in the readme.

if return address is changed, information displayed in "Call Stack" window (VS, WinDbg, ...) may be useless

I assume you're still referring to the previous method we discussed, which stores the return address in TLS and replaces it with another one? This sub-thread suggests another approach. Feel free to take a look at the readme in the repo above, let me know if you have any questions or thoughts in general.

[RatinCN]

I assume you're still referring to the previous method we discussed

Yes, and sorry for my delay, I'm a bit busy so didn't keep up with your progress, I'll keep up as soon as I can.
After a quick glance for comments above, I see:

• The solution will exports a function (e.g. SlimDetoursIsSafeToUnload), there are no changes to the existing interfaces
• Support may be limited on some platforms (x86 mentioned in your README), but I don't think that detracts from the value of the solution

If my understanding is correct, it looks good to me.

[m417z]

Currently, I decided to use the solution as a separate library, since I'm also using it in cases where I don't use SlimDetours. For this reason, I'm also not sure that being part of SlimDetours is the best place for it. If you're interested in integrating it, though, feel free to do so.