Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add better support for detouring at instruction addresses with DHooks #1969

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

chrb22
Copy link

@chrb22 chrb22 commented Mar 30, 2023

In #1956 I explain the problem that the DHook post-hook causes when detouring at an arbitrary instruction address.

These commits add the new "calling convention" CallConv_INSTRUCTION, which ensures the post-hook is never created. For good measure I set it so you must use ReturnType_Void and ThisPointer_Ignore when setting up the hook as anything else doesn't make much sense. I also made it impossible to use params that aren't custom registers as an instruction detour doesn't have any proper arguments.

Copy link
Member

@peace-maker peace-maker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this!

Since this way of detouring requires more detailed knowledge of how the detouring is done, we should add more documentation into the dhooks.inc file.

There are limitations to this approach users should be aware of:

  • As of now, there are no guarantees of keeping register values in the detouring code, so you have to make sure to preserve all registers the surrounding code still uses after the detour.
  • Detouring right before a jump-target might overwrite the following instruction causing undefined behavior when the function jumps into our inserted jmp instruction.
  • The same for detours near the end of a function. The inserted jmp instruction could reach into the next function causing problems when calling that one.
  • The original instruction where the detour is placed is always executed after your detour callback.

The jump-target problems are less prevalent on the other detouring calling conventions, since the function prologue is usually long enough and there usually are no jumps back into the prologue.

extensions/dhooks/natives.cpp Outdated Show resolved Hide resolved
extensions/dhooks/natives.cpp Outdated Show resolved Hide resolved
plugins/include/dhooks.inc Outdated Show resolved Hide resolved
@@ -85,8 +86,8 @@ CHook::CHook(void* pFunc, ICallingConvention* pConvention)
// Save the trampoline
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful if you could skip instructions similar to the "supercede" mode for function calls.

Imagine a mid-function detour on something like mov eax, 0x1000 where you'd want to change the constant. If you add a detour, change eax, and return, the original instruction would still be executed and your detour had no effect.

I guess you can work around this by detouring after that instruction for now, but that could force you into jump target territory for no reason. I guess we can improve that in an additional PR. This feature is pretty advanced and you should be knowing what you're doing when going this way.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't even consider jump targets. Having a post routine would improve the situation, but it won't solve all cases as you mentioned yourself:

Detouring right before a jump-target might overwrite the following instruction causing undefined behavior when the function jumps into our inserted jmp instruction.

Copy link
Author

@chrb22 chrb22 Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, don't really need a whole post routine for a single instruction, just a supercede implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that would just be conditionally replacing the m_pTrampoline jump with a m_pTrampoline + instruction size jump?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that does seem to work. One problem is that you can't change params and supercede at the same time currently. Should I add MRES_ChangedSupercede = -3 to enum MRESReturn and implement that? Or just have it update params automatically when using MRES_Supercede?

Copy link
Author

@chrb22 chrb22 Apr 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about changing MRESReturn based on what you said about skipping a specified number of bytes

enum MRESReturn
{
	MRES_ChangedHandled = -2,       /**< Deprecated, use `MRES_Handled | MRES_Changed` instead */
	MRES_ChangedOverride = -1,      /**< Deprecated, use `MRES_Override | MRES_Changed` instead */
	MRES_Ignored = 0,               /**< plugin didn't take any action */
	MRES_Handled,                   /**< plugin did something, but real function should still be called */
	MRES_Override,                  /**< call real function, but use my return value */
	MRES_Supercede = 1 << 30,       /**< Skip hooked function/instruction. For functions you need to supply a return value.
	                                     For instructions you can optionally add a number to set the amount of instructions to skip (hooked instruction included in the count),
	                                     For example return `MRES_Supercede + 2` to skip 2 instructions. */
	MRES_Changed = 1 << 31,         /**< Add to return action to replace the params with your own values,
	                                     For example return `MRES_Handled | MRES_Changed` to change the function arguments via a pre-hook */
};

Does returning MRES_Handled actually do something different to returning MRES_Ignored?

@chrb22
Copy link
Author

chrb22 commented Apr 1, 2023

I added instructions on how to avoid overriding jump targets after the hooked instruction. The current documentation says 6 bytes are patched, but isn't it only 5: e9 xx xx xx xx?

// If you need to detour the same function in different plugins make sure to
// wildcard \x2a the first 6 bytes of the signature to accommodate for the patched
// jump introduced by the detour.

Edit: Okay I see the #define JMP_SIZE 6, but couldn't it be 5?

@chrb22 chrb22 requested a review from peace-maker April 8, 2023 11:41
@peace-maker
Copy link
Member

Sorry for the delay, life happened. That warning about two plugins detouring the same function doesn't apply anymore since #1642 for sourcemod plugins/extensions using the gamedata ecosystem. So we can probably leave it out or specify it's only needed when using other VSP or MMS plugins to detour the same functions.

I've envisioned a new parameter to the hooksetup / Functions section allowing you to specify how many bytes to skip instead of only the one instruction. That would require some smarter logic to know when to skip the whole trampoline, but just being able to skip the one instruction is a good start. We can add support for more complex usecases when they pop up. (Was thinking of this inline hook, but I don't plan to port this to sourcepawn :P)

@chrb22
Copy link
Author

chrb22 commented Apr 19, 2023

I was mostly asking about why 6 bytes are used instead of 5.

I was also thinking that it might be possible to add a stack parameter type, so you would be able to define an instruction hook solely using a gameconf. So something like

"stack var 1"
{
    "type"      "float"
    "stack"     "-0x3c"
}

Could be implemented by adding a new entry Stack = -1024 to Register_t so the location would internally be stored in the custom_register variable with a value of Stack - 0x3c. Would also make it possible to hardcode the location in the plugin by just passing DHookRegister_Stack - 0x3c to the hook setup instead of a register.
Haven't looked at converting that to a Dhooks param that can be used in the plugin yet, but maybe it's just modifying CDynamicHooksSourcePawn::GetParamStruct and CDynamicHooksSourcePawn::UpdateParamsFromStruct?

@peace-maker
Copy link
Member

I don't know, public/asm/asm.h defines it as 5 as well. Maybe @Ayuto can chip in? Might just be an oversight.

@Ayuto
Copy link

Ayuto commented Apr 19, 2023

Yes, 5 seems to be correct. Good catch! 6 would be required for a far jump, but DynamicHooks isn't using that.

@chrb22
Copy link
Author

chrb22 commented Apr 20, 2023

I found out that it is also necessary to store the FLAGS register, but that requires me to add new instructions to the assembler from the sourcepawn submodule. I haven't used git a whole lot, so I don't know what the best way to do that is.

@chrb22
Copy link
Author

chrb22 commented Apr 22, 2023

The stack offset idea was surprisingly easy to implement: chrb22@416c5b8. It's a bit weird to put stack offsets together with the register enums, but it was the simplest way I could think of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants