Fix/#215

[acidicoala]

This PR mainly fixes #215, a small bug in the disassembler tests and asmjit warnings.

Fix for #215

The fix consists of:

1. Recognizing the call to a routine that returns dereferenced ESP value (i.e. next instruction after call).
2. Replacing such call instructions with mov reg, imm32 where:
   • reg is the destination register. In real Linux binaries I found out that it can be any GP register, not just eax
   • imm32 is the original address of the instruction that follows call instruction.

I like this approach because it doesn't involve allocating any new global memory. In fact it even makes the hooked function code a bit more performant since instead of 3 instructions (call, mov, ret) CPU will execute just 1 (mov).

Furthermore, sometimes the first instruction we might see in a function is precisely this call to routine reading SP. Hence I also modified the followJmp function to ignore such first calls, since it would be replaced soon-after.

Thanks to this fix, the build-and-test (ubuntu-latest, Debug, 32, clang) CI is now passing successfully. However, the build-and-test (ubuntu-latest, **Release**, 32, clang) is failing due to another issue that is similar in nature, but not the exactly same. I will create another issue detailing it and submit a corresponding PR to fix it once this PR has been approved.

Fix TestDisassembler

Just a minor bug in calculating byte range to disassemble.

Fix asmjit warnings

The asmjit/ashmjit.j header got deprecated, and every build was print this warning. I replaced it with asmjit/x86.h to resolve this warning since Polyhook2 supports only x86/x86_64 architecture.

There are other constant build warnings related to implicit conversion of types, which is not ideal since such conversion might differ depending on the compiler. These warnings will need to be addressed in a future PR as well.

[gemini-code-assist]

Summary of Changes

Hello @acidicoala, I'm Gemini Code Assist¹! 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 delivers crucial fixes to enhance the robustness and correctness of the Polyhook2 library, particularly concerning its disassembler and hooking mechanisms. The primary focus is on resolving issue #215, which involved misinterpreting specific 'call' instructions that read from the stack pointer. By introducing logic to recognize and transform these instructions into direct register moves, the PR ensures accurate instruction processing and improves the efficiency of hooked functions. Additionally, it addresses minor test discrepancies and updates 'asmjit' header imports to eliminate build warnings, contributing to a cleaner and more reliable codebase.

Highlights

• Improved Disassembler Accuracy for Special CALL Routines: The pull request introduces a fix for issue Trampoline calls fail when prologue calls routine reading next instruction #215, enabling the disassembler to correctly identify and handle 'call' instructions that target routines returning the dereferenced stack pointer (ESP). These 'call' instructions are now replaced with 'mov reg, imm32' to ensure proper hooking behavior and improved performance.
• Enhanced followJmp Logic: The 'followJmp' function has been updated to intelligently skip these special 'call' instructions when they appear as the first instruction of a function, preventing incorrect branch following.
• Updated asmjit Header Usage: Deprecation warnings related to 'asmjit/asmjit.h' have been resolved by switching to the more specific 'asmjit/x86.h' header, aligning with Polyhook2's x86/x86_64 architecture focus.
• Minor Disassembler Test Fix: A small bug in the 'TestDisassembler''s byte range calculation was corrected, improving test reliability.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces several fixes, including a clever solution for handling specific call instructions that retrieve the stack pointer, a fix for a disassembler test, and updates to resolve asmjit warnings. The changes are well-structured and address the intended issues. I have a couple of suggestions to improve the code further: one regarding a redundant check which can be simplified, and another concerning the use of a C++20 feature that could be updated to ensure broader compiler compatibility.

[stevemk14ebr]

This is a good approach, we are already re-writing sequences in other places like the trampoline to semantic equivalents so I am fine with this. It is also more general than the approach I had suggested by hooking later. I would prefer log outputs don't mention bug numbers (comments should, but not logs). Very minor though, up to you if you like to fix. Will merge later tonight.

[acidicoala]

Thanks for the swift response! I've changed the log line to Fixing special case [call to routine returning ESP]. The name is a bit verbose, but nothing better comes to my mind, so feel free to change it further if you wish, I don't have any strong opinions on this matter. Looking forward to the merge.

In the meantime I will create another issue for dealing with the failing test with Linux 32-bit Release build configuration.

[stevemk14ebr]

Merged parent #218