PolyHook_2_0 icon indicating copy to clipboard operation
PolyHook_2_0 copied to clipboard

MemoryProtector on pointers created by new()

Open enginelesscc opened this issue 7 months ago • 3 comments

Hi, this screams error:

https://github.com/stevemk14ebr/PolyHook_2_0/blob/f4dbf7b79762e80d464f377654fdb708ebdb9ea1/sources/x64Detour.cpp#L721 https://github.com/stevemk14ebr/PolyHook_2_0/blob/f4dbf7b79762e80d464f377654fdb708ebdb9ea1/sources/x64Detour.cpp#L776

Similarly on x86:

https://github.com/stevemk14ebr/PolyHook_2_0/blob/f4dbf7b79762e80d464f377654fdb708ebdb9ea1/sources/x86Detour.cpp#L124 https://github.com/stevemk14ebr/PolyHook_2_0/blob/f4dbf7b79762e80d464f377654fdb708ebdb9ea1/sources/x86Detour.cpp#L132

We are changing protection on memory we dont own. This has unwanted side effects because it shares pages with other allocs, and may race with other threads or other detours that are done at the same time.

Solution to this is to replace any memory new/delete that has its protection mutated with VirtualAlloc/VirtualFree, given those require a full page at minimum, giving us absolute ownership. At that point we dont need MemoryProtector there anymore, which also fixes the trampoline losing execute permission at the end of scope

And given these are full pages, they should be cached/reused if possible so not every trampoline requires its own page.

enginelesscc avatar Jul 22 '24 12:07 enginelesscc