Fix #215 & #217

.github/workflows/main.yml

@@ -29,11 +29,11 @@ jobs:
           # Windows
           - os: windows-latest
             build_type: Debug
-            test_bin_path: Debug\\PolyHook_2.exe
+            test_bin_path: Debug/PolyHook_2.exe
 
           - os: windows-latest
             build_type: Release
-            test_bin_path: Release\\PolyHook_2.exe
+            test_bin_path: Release/PolyHook_2.exe
 
           - os: windows-latest
             bitness: 32
@@ -92,7 +92,7 @@ jobs:
         # See https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html?highlight=cmake_build_type
         run: >
           cmake -B ${{ env.BUILD_DIR }}
-          -S ${{ github.workspace }}
+          -DPOLYHOOK_BUILD_DLL="ON"
           -DCMAKE_CXX_COMPILER=${{ matrix.cpp_compiler }}
           -DCMAKE_C_COMPILER=${{ matrix.c_compiler }}
           -DCMAKE_BUILD_TYPE=${{ matrix.build_type }}
@@ -103,15 +103,9 @@ jobs:
         # the default Windows generator is a multi-config generator (Visual Studio generator).
         run: cmake --build ${{ env.BUILD_DIR }} --config ${{ matrix.build_type }}
 
-      # TODO: Deduplicate this command
       - name: Configure CMake for test
         run: >
           cmake -B ${{ env.BUILD_DIR }}
-          -S ${{ github.workspace }}
-          -DCMAKE_CXX_COMPILER=${{ matrix.cpp_compiler }}
-          -DCMAKE_C_COMPILER=${{ matrix.c_compiler }}
-          -DCMAKE_BUILD_TYPE=${{ matrix.build_type }}
-          ${{ matrix.arch }}
           -DPOLYHOOK_BUILD_DLL="OFF"
 
       - name: Build tests

CMakeLists.txt

@@ -389,6 +389,7 @@ endif()
 
 #Tests
 if(NOT POLYHOOK_BUILD_DLL)
+	target_compile_definitions(${PROJECT_NAME} PRIVATE PLH_DIAGNOSTICS)
 	target_sources(${PROJECT_NAME} PRIVATE
 		${PROJECT_SOURCE_DIR}/MainTests.cpp
 		${PROJECT_SOURCE_DIR}/UnitTests/${POLYHOOK_OS}/TestDisassembler.cpp

UnitTests/TestUtils.hpp

@@ -25,7 +25,7 @@
         noexcept(noexcept(std::declval<decltype(&FUNC)>()(std::declval<Args>()...))) -> auto { \
         PLH::StackCanary canary; \
         PLH_STOP_OPTIMIZATIONS(); \
-		effects.PeakEffect().trigger(); \
+        effects.PeakEffect().trigger(); \
         __VA_ARGS__ \
         return PLH::FnCast(TRMP, &FUNC)($args...); \
     }
@@ -47,8 +47,8 @@
  * or ReleaseWithDebInfo mode (ReleaseWithDebInfo optimizes _slightly_ less).
  */
 #define PLH_STOP_OPTIMIZATIONS()                                                                                       \
-	volatile int i = 0;                                                                                                \
-	PH_UNUSED(i)
+    volatile int i = 0;                                                                                                \
+    PH_UNUSED(i)
 
 namespace PLH::test {
 

UnitTests/linux/TestDetourTranslationx64.cpp

@@ -23,10 +23,13 @@ PLH_TEST_DETOUR_CALLBACK(dlmopen, {
 TEST_CASE("Testing Detours with Translations", "[Translation][ADetour]") {
 	PLH::test::registerTestLogger();
 
+// dlmopen may or may not have instructions requiring translations.
+// Hence, this test was disabled. We need to construct reliable synthetic tests instead.
+#if 0
 	SECTION("dlmopen (INPLACE)") {
 		PLH::StackCanary canary;
 
-		const auto *resultBefore = dlmopen(LM_ID_BASE, LIBM_SO, RTLD_NOW);
+		const auto* handleBefore = dlmopen(LM_ID_BASE, LIBM_SO, RTLD_NOW);
 
 		PLH::x64Detour detour((uint64_t)dlmopen, (uint64_t)dlmopen_hooked, &dlmopen_trmp);
 		// Only INPLACE creates conditions for translation, since
@@ -36,11 +39,13 @@ TEST_CASE("Testing Detours with Translations", "[Translation][ADetour]") {
 		REQUIRE(detour.hook());
 
 		effects.PushEffect();
-		const auto *resultAfter = dlmopen(LM_ID_BASE, LIBM_SO, RTLD_NOW);
+		const auto* handleAfter = dlmopen(LM_ID_BASE, LIBM_SO, RTLD_NOW);
 
+		REQUIRE(detour.hasDiagnostic(PLH::Diagnostic::TranslatedInstructions));
 		REQUIRE(effects.PopEffect().didExecute());
-		REQUIRE(resultAfter == resultBefore);
+		REQUIRE(handleAfter == handleBefore);
 
 		REQUIRE(detour.unHook());
 	}
+#endif
 }

UnitTests/linux/TestDetourx64.cpp

@@ -70,7 +70,7 @@ unsigned char hookMe5[] = {
 	0x48, 0xFF, 0xA0, 0x20, 0x01, 0x00, 0x00 // jmp qword ptr[rax+120h]
 };
 
-uint64_t nullTramp = NULL;
+uint64_t nullTramp = 0;
 NOINLINE void h_nullstub() {
 	PLH::StackCanary canary;
 	PLH_STOP_OPTIMIZATIONS();

UnitTests/linux/TestDetourx86.cpp

@@ -14,6 +14,8 @@ namespace {
 
 EffectTracker effects;
 
+constexpr uint8_t JUMP_SIZE = 5;
+
 }
 
 NOINLINE int __cdecl hookMe1() {
@@ -97,6 +99,31 @@ NOINLINE void PH_ATTR_NAKED hookMeLoop() {
 		"ret;");
 }
 
+extern "C" NOINLINE uintptr_t PH_ATTR_NAKED returnESP() {
+	__asm__ __volatile__ (
+		"movl (%esp), %eax\n"
+		"ret"
+	);
+}
+
+NOINLINE uintptr_t PH_ATTR_NAKED readESP() {
+	__asm__ __volatile__ (
+		"call returnESP\n"
+		"ret"
+	);
+}
+PLH_TEST_DETOUR_CALLBACK(readESP);
+
+NOINLINE uintptr_t PH_ATTR_NAKED inlineReadESP() {
+	__asm__ __volatile__ (
+		"call 0f\n"
+		"0: pop %%eax\n"
+		"ret"
+		::: "eax"
+	);
+}
+PLH_TEST_DETOUR_CALLBACK(inlineReadESP);
+
 PLH_TEST_DETOUR_CALLBACK(hookMeLoop);
 
 // PLH_TEST_DETOUR_CALLBACK doesn't support variadic functions yet.
@@ -114,7 +141,7 @@ NOINLINE int h_hookPrintf(const char *format, ...) {
 	return PLH::FnCast(hookPrintfTramp, &printf)("INTERCEPTED YO:%s", message.c_str());
 }
 
-// must specify specific overload of std::pow by assiging to pFn of type
+// must specify specific overload of std::pow by assigning to pFn of type
 const auto &pow_double = std::pow<double, double>;
 PLH_TEST_DETOUR_CALLBACK(pow_double);
 
@@ -186,6 +213,35 @@ TEST_CASE("Testing x86 detours", "[x86Detour][ADetour]") {
 		REQUIRE(detour.unHook() == true);
 	}
 
+	SECTION("Test #215 (call to routine returning ESP)") {
+		PLH::x86Detour PLH_TEST_DETOUR(readESP);
+		REQUIRE(detour.hook() == true);
+
+		effects.PushEffect();
+		const auto esp = readESP();
+		REQUIRE(esp == (uintptr_t)readESP + JUMP_SIZE);
+		REQUIRE(detour.hasDiagnostic(PLH::Diagnostic::FixedCallToRoutineReadingSP));
+		REQUIRE(effects.PopEffect().didExecute());
+		REQUIRE(detour.unHook() == true);
+	}
+
+	SECTION("Test #217 (inline call to read ESP)") {
+		PLH::x86Detour PLH_TEST_DETOUR(inlineReadESP);
+		REQUIRE(detour.hook() == true);
+
+		effects.PushEffect();
+		const auto esp = inlineReadESP();
+		REQUIRE(esp == (uintptr_t)inlineReadESP + JUMP_SIZE);
+		REQUIRE(detour.hasDiagnostic(PLH::Diagnostic::FixedInlineCallToReadSP));
+		REQUIRE(effects.PopEffect().didExecute());
+		REQUIRE(detour.unHook() == true);
+	}
+
+#ifndef NDEBUG
+	// This test is disabled in Release builds due to aggressive optimization of compilers.
+	// Specifically with clang on Linux the std::pow function is always inlined.
+	// Hence, the hooked function is never called.
+
 	// it's a pun...
 	// ^ what pun? nothing found on the web >.<
 	SECTION("hook pow") {
@@ -199,6 +255,7 @@ TEST_CASE("Testing x86 detours", "[x86Detour][ADetour]") {
 		detour.unHook();
 		REQUIRE(effects.PopEffect().didExecute());
 	}
+#endif
 
 	SECTION("hook malloc") {
 		PLH::x86Detour PLH_TEST_DETOUR(malloc);
@@ -216,8 +273,6 @@ TEST_CASE("Testing x86 detours", "[x86Detour][ADetour]") {
 		REQUIRE(detour.hook() == true);
 	}
 
-	// TODO: Fix this. when making jmpToProl, relative displacement can only encode offset up to 0x7FFFFFFF
-	//       But during tests, distance between prologue and trampoline was larger than that, leading to incorrect jump.
 	SECTION("hook printf") {
 		PLH::x86Detour detour((uint64_t)&printf, (uint64_t)h_hookPrintf, &hookPrintfTramp);
 		REQUIRE(detour.hook() == true);

UnitTests/linux/TestDisassembler.cpp

@@ -263,7 +263,7 @@ TEMPLATE_TEST_CASE("Test Disassemblers x86 FF25", "[ZydisDisassembler]", PLH::Zy
 	auto Instructions = disasm.disassemble(
 		(uint64_t)x86ASM_FF25.data(),
 		(uint64_t)x86ASM_FF25.data(),
-		(uint64_t)(x86ASM_FF25.data() + address_length),
+		(uint64_t)(jmp_address_ptr + address_length),
 		PLH::MemAccessor()
 	);
 

UnitTests/windows/TestDetourx64.cpp

@@ -9,7 +9,7 @@
 
 #include "polyhook2/PolyHookOsIncludes.hpp"
 
-#include <asmjit/asmjit.h>
+#include <asmjit/x86.h>
 
 #define WIN32_LEAN_AND_MEAN
 #include <Windows.h>

polyhook2/Detour/ADetour.hpp

@@ -65,6 +65,30 @@ class Detour : public PLH::IHook {
 
     void setIsFollowCallOnFnAddress(bool value);
 
+
+    /**
+     * @return instructions of a routine if the provided instruction is a call instruction that calls a routine
+     * which returns an address stored SP register, which refers to the address of the next instruction following
+     * the call instruction. This is used to fix issue #215.
+     * An example routine looks like this (eax could be any general-purpose register):
+     * <code>
+     * 8B 04 24 | mov eax, dword ptr [esp]
+     * C3       | ret
+     * </code>
+     */
+    std::optional<insts_t> getRoutineReturningSP(const Instruction& callInst);
+
+    /**
+     * @return true if the provided instruction is a call instruction which has next address being its destination
+     * and which is followed by a `pop reg` instruction. This is used to fix issue #217.
+     * An example inline routine looks like this (ebx could be any general-purpose register):
+     * <code>
+     * 0x59d57b24 | e8 00 00 00 00 | call 0x59D57B29 -> 59d57b29
+     * 0x59d57b29 | 5b             | pop ebx
+     * </code>
+     */
+    bool isCallInlineReturningSP(const Instruction& callInst);
+
 protected:
     uint64_t m_fnAddress;
     uint64_t m_fnCallback;