safetyhook icon indicating copy to clipboard operation
safetyhook copied to clipboard

Test/Thread trapping

Open cursey opened this issue 1 year ago • 3 comments

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.

cursey avatar Feb 10 '24 09:02 cursey

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?

ThirteenAG avatar Mar 10 '24 03:03 ThirteenAG

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.

cursey avatar Mar 10 '24 04:03 cursey

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.

praydog avatar Apr 05 '24 05:04 praydog