Fix #215 & #217 #218
Fix #215 & #217 #218
Conversation
Summary of ChangesHello @acidicoala, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the robustness of the hooking mechanism by addressing two critical issues (#215 and #217) related to stack pointer manipulation during function calls. It introduces intelligent instruction translation for specific Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces fixes for issues #215 and #217 by patching specific instruction patterns, and adds a diagnostics framework to verify these fixes in tests. The changes are well-structured and the addition of synthetic tests for these specific edge cases is a great improvement. My main feedback focuses on improving the error handling in the new instruction patching logic to make it more robust by propagating failures, which will prevent potential crashes if the patching fails at runtime.
polyhook2/Detour/x86Detour.hpp
Outdated
| void fixSpecialCases(insts_t& prologue); | ||
| void fixCallToRoutineReturningSP(Instruction& callInst, const insts_t& routine); | ||
| void fixInlineCallToReadSP(Instruction& callInst); |
There was a problem hiding this comment.
These functions perform instruction patching which can fail (e.g., if asmjit fails to generate code). Returning void hides these failures, which could lead to crashes or undefined behavior later in the hooking process if it continues with unpatched instructions. It would be more robust to change their return type to bool to propagate success or failure.
| void fixSpecialCases(insts_t& prologue); | |
| void fixCallToRoutineReturningSP(Instruction& callInst, const insts_t& routine); | |
| void fixInlineCallToReadSP(Instruction& callInst); | |
| bool fixSpecialCases(insts_t& prologue); | |
| bool fixCallToRoutineReturningSP(Instruction& callInst, const insts_t& routine); | |
| bool fixInlineCallToReadSP(Instruction& callInst); |
There was a problem hiding this comment.
Hmm, this suggestion is not without merit. I can properly implement it, if @stevemk14ebr is OK with it.
sources/x86Detour.cpp
Outdated
| void x86Detour::fixCallToRoutineReturningSP(Instruction& callInst, const insts_t& routine) { | ||
| Log::log( | ||
| "Fixing special case [call to routine returning ESP]:\n" + instsToStr(std::vector{callInst}), | ||
| ErrorLevel::INFO | ||
| ); | ||
|
|
||
| const auto destReg = routine[0].getOperands().substr(0, 3); | ||
| const uint32_t originalAddress = callInst.getAddress(); | ||
| const uint32_t originalNextAddress = originalAddress + callInst.size(); | ||
|
|
||
| // AsmTK parses strings for AsmJit, which generates the binary code. | ||
| asmjit::CodeHolder code; | ||
| asmjit::JitRuntime asmjitRt; | ||
| code.init(asmjitRt.environment()); | ||
|
|
||
| asmjit::x86::Assembler assembler(&code); | ||
| asmtk::AsmParser parser(&assembler); | ||
|
|
||
| // Parse the instructions via AsmTK | ||
| if (const auto error = parser.parse(std::format("mov {}, {:#x}", destReg, originalNextAddress).c_str())) { | ||
| Log::log(std::format("AsmTK error: {}", asmjit::DebugUtils::errorAsString(error)), ErrorLevel::SEV); | ||
| return; | ||
| } | ||
|
|
||
| // Generate the binary code via AsmJit | ||
| uint64_t movAddress = 0; | ||
| if (const auto error = asmjitRt.add(&movAddress, &code)) { | ||
| Log::log(std::format("AsmJIT error: {}", asmjit::DebugUtils::errorAsString(error)), ErrorLevel::SEV); | ||
| return; | ||
| } | ||
|
|
||
| // Replace `call rel32` instruction with `mov reg, imm32`. Both are 5 bytes long. | ||
|
|
||
| callInst = m_disasm.disassemble(movAddress, movAddress, movAddress + callInst.size(), *this)[0]; | ||
| callInst.setAddress(originalAddress); | ||
| } |
There was a problem hiding this comment.
Following the change in the header, this function should return bool to indicate whether the instruction patching was successful. On failure (e.g., from parser.parse or asmjitRt.add), it should return false. On success, it should return true at the end.
bool x86Detour::fixCallToRoutineReturningSP(Instruction& callInst, const insts_t& routine) {
Log::log(
"Fixing special case [call to routine returning ESP]:\n" + instsToStr(std::vector{callInst}),
ErrorLevel::INFO
);
const auto destReg = routine[0].getOperands().substr(0, 3);
const uint32_t originalAddress = callInst.getAddress();
const uint32_t originalNextAddress = originalAddress + callInst.size();
// AsmTK parses strings for AsmJit, which generates the binary code.
asmjit::CodeHolder code;
asmjit::JitRuntime asmjitRt;
code.init(asmjitRt.environment());
asmjit::x86::Assembler assembler(&code);
asmtk::AsmParser parser(&assembler);
// Parse the instructions via AsmTK
if (const auto error = parser.parse(std::format("mov {}, {:#x}", destReg, originalNextAddress).c_str())) {
Log::log(std::format("AsmTK error: {}", asmjit::DebugUtils::errorAsString(error)), ErrorLevel::SEV);
return false;
}
// Generate the binary code via AsmJit
uint64_t movAddress = 0;
if (const auto error = asmjitRt.add(&movAddress, &code)) {
Log::log(std::format("AsmJIT error: {}", asmjit::DebugUtils::errorAsString(error)), ErrorLevel::SEV);
return false;
}
// Replace `call rel32` instruction with `mov reg, imm32`. Both are 5 bytes long.
callInst = m_disasm.disassemble(movAddress, movAddress, movAddress + callInst.size(), *this)[0];
callInst.setAddress(originalAddress);
return true;
}
sources/x86Detour.cpp
Outdated
| void x86Detour::fixInlineCallToReadSP(Instruction& callInst) { | ||
| Log::log( | ||
| "Fixing special case [inline call to to read ESP]:\n" + instsToStr(std::vector{callInst}), | ||
| ErrorLevel::INFO | ||
| ); | ||
|
|
||
| const uint32_t originalAddress = callInst.getAddress(); | ||
| const uint32_t originalNextAddress = originalAddress + callInst.size(); | ||
|
|
||
| // AsmTK parses strings for AsmJit, which generates the binary code. | ||
| asmjit::CodeHolder code; | ||
| asmjit::JitRuntime asmjitRt; | ||
| code.init(asmjitRt.environment()); | ||
|
|
||
| asmjit::x86::Assembler assembler(&code); | ||
| asmtk::AsmParser parser(&assembler); | ||
|
|
||
| // Parse the instructions via AsmTK | ||
| if (const auto error = parser.parse(std::format("push {:#x}", originalNextAddress).c_str())) { | ||
| Log::log(std::format("AsmTK error: {}", asmjit::DebugUtils::errorAsString(error)), ErrorLevel::SEV); | ||
| return; | ||
| } | ||
|
|
||
| // Generate the binary code via AsmJit | ||
| uint64_t pushAddress = 0; | ||
| if (const auto error = asmjitRt.add(&pushAddress, &code)) { | ||
| Log::log(std::format("AsmJIT error: {}", asmjit::DebugUtils::errorAsString(error)), ErrorLevel::SEV); | ||
| return; | ||
| } | ||
|
|
||
| // Replace `call rel32` instruction with `push imm32`. Both are 5 bytes long. | ||
|
|
||
| callInst = m_disasm.disassemble(pushAddress, pushAddress, pushAddress + callInst.size(), *this)[0]; | ||
| callInst.setAddress(originalAddress); | ||
| } |
There was a problem hiding this comment.
This function should return bool to indicate success or failure, for the same reasons as fixCallToRoutineReturningSP. This ensures that any failures during instruction patching are properly handled.
bool x86Detour::fixInlineCallToReadSP(Instruction& callInst) {
Log::log(
"Fixing special case [inline call to to read ESP]:\n" + instsToStr(std::vector{callInst}),
ErrorLevel::INFO
);
const uint32_t originalAddress = callInst.getAddress();
const uint32_t originalNextAddress = originalAddress + callInst.size();
// AsmTK parses strings for AsmJit, which generates the binary code.
asmjit::CodeHolder code;
asmjit::JitRuntime asmjitRt;
code.init(asmjitRt.environment());
asmjit::x86::Assembler assembler(&code);
asmtk::AsmParser parser(&assembler);
// Parse the instructions via AsmTK
if (const auto error = parser.parse(std::format("push {:#x}", originalNextAddress).c_str())) {
Log::log(std::format("AsmTK error: {}", asmjit::DebugUtils::errorAsString(error)), ErrorLevel::SEV);
return false;
}
// Generate the binary code via AsmJit
uint64_t pushAddress = 0;
if (const auto error = asmjitRt.add(&pushAddress, &code)) {
Log::log(std::format("AsmJIT error: {}", asmjit::DebugUtils::errorAsString(error)), ErrorLevel::SEV);
return false;
}
// Replace `call rel32` instruction with `push imm32`. Both are 5 bytes long.
callInst = m_disasm.disassemble(pushAddress, pushAddress, pushAddress + callInst.size(), *this)[0];
callInst.setAddress(originalAddress);
return true;
}
sources/x86Detour.cpp
Outdated
| void x86Detour::fixSpecialCases(insts_t& prologue) { | ||
| for (auto& instruction: prologue) { | ||
| if (const auto routine = getRoutineReturningSP(instruction)) { | ||
| // Fix for #215 https://github.com/stevemk14ebr/PolyHook_2_0/issues/215 | ||
| fixCallToRoutineReturningSP(instruction, *routine); | ||
| PLH_SET_DIAGNOSTIC(Diagnostic::FixedCallToRoutineReadingSP); | ||
| } else if (isInlineCallToReadSP(instruction)) { | ||
| // Fix for #217 https://github.com/stevemk14ebr/PolyHook_2_0/issues/217 | ||
| fixInlineCallToReadSP(instruction); | ||
| PLH_SET_DIAGNOSTIC(Diagnostic::FixedInlineCallToReadSP); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This function should return bool and check the return values of the helper functions (fixCallToRoutineReturningSP and fixInlineCallToReadSP) to propagate any failures up the call stack.
| void x86Detour::fixSpecialCases(insts_t& prologue) { | |
| for (auto& instruction: prologue) { | |
| if (const auto routine = getRoutineReturningSP(instruction)) { | |
| // Fix for #215 https://github.com/stevemk14ebr/PolyHook_2_0/issues/215 | |
| fixCallToRoutineReturningSP(instruction, *routine); | |
| PLH_SET_DIAGNOSTIC(Diagnostic::FixedCallToRoutineReadingSP); | |
| } else if (isInlineCallToReadSP(instruction)) { | |
| // Fix for #217 https://github.com/stevemk14ebr/PolyHook_2_0/issues/217 | |
| fixInlineCallToReadSP(instruction); | |
| PLH_SET_DIAGNOSTIC(Diagnostic::FixedInlineCallToReadSP); | |
| } | |
| } | |
| } | |
| bool x86Detour::fixSpecialCases(insts_t& prologue) { | |
| for (auto& instruction: prologue) { | |
| if (const auto routine = getRoutineReturningSP(instruction)) { | |
| // Fix for #215 https://github.com/stevemk14ebr/PolyHook_2_0/issues/215 | |
| if (!fixCallToRoutineReturningSP(instruction, *routine)) return false; | |
| PLH_SET_DIAGNOSTIC(Diagnostic::FixedCallToRoutineReadingSP); | |
| } else if (isInlineCallToReadSP(instruction)) { | |
| // Fix for #217 https://github.com/stevemk14ebr/PolyHook_2_0/issues/217 | |
| if (!fixInlineCallToReadSP(instruction)) return false; | |
| PLH_SET_DIAGNOSTIC(Diagnostic::FixedInlineCallToReadSP); | |
| } | |
| } | |
| return true; | |
| } |
sources/x86Detour.cpp
Outdated
| bool x86Detour::makeTrampoline(insts_t& prologue, insts_t& trampolineOut) { | ||
| assert(!prologue.empty()); | ||
|
|
||
| fixSpecialCases(prologue); |
There was a problem hiding this comment.
The return value of fixSpecialCases should be checked here. If it fails, makeTrampoline should also fail and return false to prevent the hook from proceeding with potentially corrupted instructions.
| fixSpecialCases(prologue); | |
| if (!fixSpecialCases(prologue)) { | |
| Log::log("Failed to fix special case instructions", ErrorLevel::SEV); | |
| return false; | |
| } |
|
@stevemk14ebr, I've implemented the suggestion related to error checking in fixup functions. I've also took liberty to eliminate build warnings that were bugging me this whole time. This PR is now ready for your review. |
|
Great changes! Really appreciate the recent work for linux compat |
|
Thanks a ton! |
2 participants
While I was on a roll, I decided to create a branch from my branch so that I could work on a fix for #217 and a few other QoL improvements. This branch includes everything from #216 plus additional commits. I did it this way, so that you could chose if how you wish to review those fixes - in isolation or together. If you chose to approve and merge this PR, I will close the #216. If you chose to approve and merge the #216, I will close this PR and create a new one with the fix for #217 specifically.
This PR implements a fix for #217 as it was described in the first post of the issue, by replacing the
call rel32instruction withpush imm32.With those 2 major issues fixed, there were still some minor issues preventing all tests from passing, so they were fixed in this branch as well. Specifically:
Inline call to read SPidiom #217: We can't rely on Linux binaries to test those fixes since those binaries may or may not contain problematic instructions. Instead, inline assembly was used to construct appropriate test cases.Inline call to read SPidiom #217, instruction translations, etc., we need to be able to confirm that said fixes actually took place. Until now this was only possible to do manually by inspecting the output logs. But now when building tests (POLYHOOK_BUILD_DLL OFF), a newPLH_DIAGNOSTICSdefinition will be compiled. When this definition is enabled,IHookclass will contain a newuint32_t m_diagnosticsmember variable, which holds flags for various diagnostics. All flags are defined in theDiagnosticenum class inEnums.hpp(which itself is defined only ifPLH_DIAGNOSTICSis defined). When a special fix gets applied, a corresponding diagnostic flag gets set, and then in tests we can verify that using theIHook::hasDiagnostic(Diagnostic d)helper. For instance, this has allowed me to uncover that no translation was actually taking place in thedlmopeninstruction, so I disabled that test for now. For now these diagnostics are used in tests for Trampoline calls fail when prologue calls routine reading next instruction #215 and Detour fails when prologue containsInline call to read SPidiom #217 where they verify that the expected fixes did in fact take place. I can see other flags being added in the future, making the tests more reliable. Of course in library (non-test) builds, none of the diagnostic functions, enums and variables will be available, so this has no performance impact outside of tests. This design was inspired by the nlohmann/json library, which features a JSON_DIAGNOSTICS flag that works in a similar manner.With all these changes, we should expect all tests to pass from this point forward.