Program crashes when unhooking

[Nukoooo]

Code to reproduce:

SafetyHookInline hook;

__declspec(noinline) void SayHello(int times)
{
    std::cout << "Hello #" << times << std::endl;
}

void Hooked_SayHello(int times)
{
    hook->call<void, int>(1337);
}

void SayHelloInfinitely()
{
    int count = 0;
    while (true)
    {
        SayHello(count++);
    }
}

int main()
{
    // Starting a thread for SayHello
    std::thread t(SayHelloInfinitely);
    t.detach();

    {
        auto builder = SafetyHookFactory::acquire();
        hook = builder.create_inline(SayHello, Hooked_SayHello);
    }

    std::this_thread::sleep_for(std::chrono::seconds(1));
    hook.reset();
    std::this_thread::sleep_for(std::chrono::seconds(1));

    return 0;
}

Although this can be "fixed" by checking nullptr for hook, it doesn't really live up to the library name

Note: The library used in the code is from Update README.md

[cursey]

I've tried addressing your concerns in #12. It still isn't perfectly safe in my opinion but it is an improvement.

[Nukoooo]

Thank you so much for that! I tried the PR in my production and it partially fixes the issue, because it still crashes on unhooking from time to time. Wouldn't it be better to freeze all the threads while unhooking? I might be missing something but I don't see any thread-freezing related in unhook code while readme says Stops all other threads when creating or deleting hooks

[angelfor3v3r]

When you call unique_ptr.reset then ~InlineHook gets called which will freeze threads. I don't know what your use cases for unhooking is but it's generally very unsafe unless you have full control over when the function you're hooking is being called. No hooking library, from what I know, fixes this.

[cursey]

Wouldn't it be better to freeze all the threads while unhooking? I might be missing something but I don't see any thread-freezing related in unhook code while readme says Stops all other threads when creating or deleting hooks

It freezes the threads when it acquires a builder from the factory:

safetyhook/src/InlineHook.cpp

Lines 287 to 292 in c25ad1a

	void InlineHook::destroy() {
	if (m_trampoline == 0) {
	return;
	}
	auto builder = Factory::acquire();

it still crashes on unhooking from time to time.

Yeah. I think I can further improve the safety by adding a mutex to InlineHook but that would incur a (small) performance penalty when call'ing the original function. If I add this, I'll probably make it opt-in by providing a call_safe method or something that users can choose to use if unhook safety is important for them.

[cursey]

I went ahead and implemented the mutex idea for all the *call methods since safety is a priority for this library I felt I should just keep it as fool-proof as possible (0c2147b).

[Nukoooo]

Thanks for the clarification 👍. I think mutex addresses the issue and don't experience with crashing when unhooking now