MemoryProtector on pointers created by new()

[enginelesscc]

Hi, this screams error:

PolyHook_2_0/sources/x64Detour.cpp

Line 721 in f4dbf7b

auto tmpTrampoline = (uint64_t) new uint8_t[m_trampolineSz];

PolyHook_2_0/sources/x64Detour.cpp

Line 776 in f4dbf7b

MemoryProtector prot(m_trampoline, m_trampolineSz, ProtFlag::R | ProtFlag::W | ProtFlag::X, *this, false);

Similarly on x86:

PolyHook_2_0/sources/x86Detour.cpp

Line 124 in f4dbf7b

m_trampoline = (uint64_t) new unsigned char[m_trampolineSz];

PolyHook_2_0/sources/x86Detour.cpp

Line 132 in f4dbf7b

MemoryProtector prot(m_trampoline, m_trampolineSz, ProtFlag::R | ProtFlag::W | ProtFlag::X, *this, false);

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.

[stevemk14ebr]

Trampolines will not lose permissions at end of scope as the last parameter to mem protector is set false. You are right though, we should not be stomping permissions of memory we don't own. Using virtualalloc as you suggest would commit one page per hook which is too large. We have a block allocator I think that x64 detour uses already somewhere, we can use this.

Thanks for pointing this out, it's a good fix

[enginelesscc]

Speaking of, why aren't we using asmjit's buffers?
one static asmjit::JitRuntime asmjit_runtime; used for all trampolines

Rough examples:

https://github.com/EngineLessCC/polyhook2/blob/master/polyhook2/Detour/ADetour.hpp#L28
https://github.com/EngineLessCC/polyhook2/blob/master/sources/x86Detour.cpp#L136
https://github.com/EngineLessCC/polyhook2/blob/master/sources/x86Detour.cpp#L144
https://github.com/EngineLessCC/polyhook2/blob/master/sources/x64Detour.cpp#L245

But, if possible, why not replace the manual assembly with asmjit directly? :)

[stevemk14ebr]

This code was written before asmjit was a dependency (it was added for ILCallback). The majority of the trampoline is overwritten code copied to the trampoline from the original function. In this case we read existing instructions into an instruction object, relocate them, then write these directly to the trampoline. We could have done this with asmjit but I wanted to change the instructions as little as possible for reliability and correctness reasons and just do the relocation gymnastics. Asmjit would have involved more re-writing.

I'd prefer fixing this by sticking to the allocation issue. new is not good enough here, I agree with your original comment. We can use the FBAllocator. Reserve a large block with virtualalloc and set permissions X then the FBAllocator can split that up into smaller trampolines.