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

Test/Thread trapping #63

Merged
merged 7 commits into from
May 15, 2024
Merged

Test/Thread trapping #63

merged 7 commits into from
May 15, 2024

Conversation

cursey
Copy link
Owner

@cursey cursey commented Feb 10, 2024

This is a POC/WIP PR that replaces thread freezing with a technique I'm calling thread trapping. The idea is to remove execute access from the pages of memory we're modifying and trap the threads that throw access violations on them until we're done writing our changes to them. We then fix up the IP from within the exception handler itself.

This is intended to be a safer/more reliable/more performant alternative to freezing threads.

@ThirteenAG
Copy link

Just wanted to let you know that I've started using this PR in my releases, so far so good. Hope it gets merged soon. Perhaps there's no reason to remove execute_while_frozen, and it should be left as an extra?

@cursey
Copy link
Owner Author

cursey commented Mar 10, 2024

Just wanted to let you know that I've started using this PR in my releases, so far so good. Hope it gets merged soon. Perhaps there's no reason to remove execute_while_frozen, and it should be left as an extra?

Happy to hear that. I don't necessarily want to maintain execute_while_frozen if it is no longer being used by the library. Luckily the functionality is self contained and can easily be ripped out if someone needs it.

There are a few things holding up landing this PR that I still need to address.

  • This branch needs to be rebased on main where a number of changes have since taken place so conflicts need to be resolved.
  • I have some concern that this isn't entirely thread safe if a DLL using safetyhook is unloaded. Maybe this is a good enough usecase to keep execute_while_frozen around.
  • Optional Thread trapping should be compatible with Linux by using a signal handler.

Feedback that this system is working reliably is really great though and I'll try prioritize landing it (or at least bringing it up-to-date with main). Thanks.

@praydog
Copy link
Collaborator

praydog commented Apr 5, 2024

Just wanted to let you know that I've started using this PR in my releases, so far so good. Hope it gets merged soon. Perhaps there's no reason to remove execute_while_frozen, and it should be left as an extra?

I would highly recommend using the latest commit that @cursey just pushed. I was having some major issues with it in a multi-threaded context where trying to execute code in the protected trampoline page would not be caught and handled correctly by safetyhook's exception handler.

I was hooking a really busy function, and while the initial hook worked, randomly during the hooking phase of another hook, the trampoline page would be protected, and the exception would not be handled correctly if the original busy function jmped to the trampoline which was now not execute permissible.

@cursey cursey marked this pull request as ready for review May 15, 2024 05:52
@cursey cursey merged commit b046e12 into main May 15, 2024
5 checks passed
@cursey cursey deleted the test/thread-trapping branch May 15, 2024 05:57
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

Successfully merging this pull request may close these issues.

4 participants