Align trampoline to 2 bytes to pass mfp check

[shqke]

Possible follow up to #78.

See 2.3.2 Member Function Pointers:

In the standard representation, a member function pointer for a virtual function is represented with ptr set to 1 plus the function's v-table entry offset (in bytes), converted to a function pointer as if by reinterpret_cast<fnptr_t>(uintfnptr_t(1 + offset)), where uintfnptr_t is an unsigned integer of the same size as fnptr_t.

When dereferencing a member function pointer (MFP) before calling it, the compiler (Clang/GCC) generates a check to verify if the pointer points to a virtual method. This check typically involves inspecting the least significant bit (LSB) of the pointer and handling it differently if the bit is set.

If the trampoline size is not properly aligned, subsequent allocations within the block may cause undefined behavior when calling trampolines using an MFP signature.

Code generation example:
https://godbolt.org/z/c15rrxK4P

I've found no reason to use a bigger value for alignment, but then I am unaware of any other issues that could occur.

[angelfor3v3r]

For me (Windows x86-64, on a x86-32 safetyhook test binary) this makes midhook fail and causes a crash. Seems like it's calling the wrong address. Not sure.

[shqke]

I think I see the problem, assembly data expects requested size:

safetyhook/src/mid_hook.cpp

Lines 136 to 137 in 4b062f6

	store(m_stub.data() + 0x02, m_stub.data() + m_stub.size() - 4);
	store(m_stub.data() + 0x59, m_stub.data() + m_stub.size() - 8);

Then size could be aligned before working with nodes:

diff --git a/src/allocator.cpp b/src/allocator.cpp
index 08504d5..83e673c 100644
--- a/src/allocator.cpp
+++ b/src/allocator.cpp
@@ -86,18 +86,18 @@ std::expected<Allocation, Allocator::Error> Allocator::internal_allocate_near(
     const std::vector<uint8_t*>& desired_addresses, size_t size, size_t max_distance) {
     // Align to 2 bytes to pass MFP virtual method check
     // See https://itanium-cxx-abi.github.io/cxx-abi/abi.html#member-function-pointers
-    size = align_up(size, 2);
+    size_t aligned_size = align_up(size, 2);
 
     // First search through our list of allocations for a free block that is large
     // enough.
     for (const auto& allocation : m_memory) {
-        if (allocation->size < size) {
+        if (allocation->size < aligned_size) {
             continue;
         }
 
         for (auto node = allocation->freelist.get(); node != nullptr; node = node->next.get()) {
             // Enough room?
-            if (static_cast<size_t>(node->end - node->start) < size) {
+            if (static_cast<size_t>(node->end - node->start) < aligned_size) {
                 continue;
             }
 
@@ -108,14 +108,14 @@ std::expected<Allocation, Allocator::Error> Allocator::internal_allocate_near(
                 continue;
             }
 
-            node->start += size;
+            node->start += aligned_size;
 
             return Allocation{shared_from_this(), address, size};
         }
     }
 
     // If we didn't find a free block, we need to allocate a new one.
-    auto allocation_size = align_up(size, system_info().allocation_granularity);
+    auto allocation_size = align_up(aligned_size, system_info().allocation_granularity);
     auto allocation_address = allocate_nearby_memory(desired_addresses, allocation_size, max_distance);
 
     if (!allocation_address) {
@@ -127,13 +127,15 @@ std::expected<Allocation, Allocator::Error> Allocator::internal_allocate_near(
     allocation->address = *allocation_address;
     allocation->size = allocation_size;
     allocation->freelist = std::make_unique<FreeNode>();
-    allocation->freelist->start = *allocation_address + size;
+    allocation->freelist->start = *allocation_address + aligned_size;
     allocation->freelist->end = *allocation_address + allocation_size;
 
     return Allocation{shared_from_this(), *allocation_address, size};
 }
 
 void Allocator::internal_free(uint8_t* address, size_t size) {
+    size = align_up(size, 2);
+
     for (const auto& allocation : m_memory) {
         if (allocation->address > address || allocation->address + allocation->size < address) {
             continue;

I've ran test locally this time.
Let me know if you're okay with this solution.

[angelfor3v3r]

Sure. Commit the changes and we can try CI again and review it. :)

[cursey]

Thanks for the contribution.