Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heavy concurrency during hook creation causes erratic runtime behaviour #16

Open
goaaats opened this issue Jul 17, 2022 · 1 comment
Open

Comments

@goaaats
Copy link

goaaats commented Jul 17, 2022

Hi! We recently did a big refactor on our project that introduced parallel component/plugin loads, leading to a lot of hooks being created at the same time. This, on fast machines, seemingly leads to erratic runtime behaviour from reloaded-created hooks, such as:

  • Access violations when hooked function is called
  • Detour isn't being called
  • Original function call doesn't respect detoured arguments, is instead being called with original arguments (this one was particularly annoying to identify)

Sadly, none of our devs are running into this, as it seemingly only happens to users on very fast machines, making it very hard to collect more details about the issue. We already placed a lock around Reloaded hook .ctors in order to have them succeed.

Switching to our MinHook-based backend that we have to troubleshoot issues like this, but don't use in production, resolves the issue. Disposing the misbehaving Reloaded hook, then recreating it at idle when no other hooks are being created, resolves the issue.

Is this a known problem/limitation? Is there anything we could do/log that could help with troubleshooting this?

Thanks!

@Sewer56
Copy link
Member

Sewer56 commented Jul 17, 2022

I never wrote Reloaded.Hooks with concurrency in mind as past the initial JIT overhead, the time to hook a function is usually negligible (<1ms).

That said, I can imagine at least two possible points of failure:

  • Native Assembler (Assembler in Utilities class) being used by multiple threads at once.
    • I'd imagine you'd want to mark the Assembler ThreadStatic or use ThreadLocal<T> so you could have an instance per thread.
    • Alternatively abstract it and place a Semaphore/Lock/etc. around it. [I'd actually prefer this in the x86 case as the chance of thread contention is low and I'd prefer to save on limited virtual address space if possible].
  • Hooking the same address before calling .Activate() on the first hook. [The 2nd hook wouldn't register the new function prologue for function because it's not overwritten until you call .Activate()].

This is simply off the top of my head. I didn't look at the code much.
Based on your description, I'd imagine it would probably be the former.

Note: I'm currently on vacation and don't currently have access to a PC most of the time. I'll be home next weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants