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

Fix hook.cpp for insider builds #2

Merged

Conversation

nikitalita
Copy link
Contributor

Hi,

I had attempted to use your current build for windows insider with your new hook fixes, and it didn't work.

Unfortunately, in the original issue mridgers/clink#543 (comment), I had uploaded the wrong screenshot, and it looks like you were working from that. Unfortunately, ReadConsoleW does not have enough cc bytes preceeding it before hitting the end of another function. However, according to Ghidra, this block does not actually get referenced at all, it appears to just be garbage lines:

image

You can see another piece of code jumps into the label, and then makes a RET without calling any of the following lines, and none of those following lines get referenced at all. So, these should be safe to patch.

I've updated hook.cpp to make it so that, if we hit a RET and we have enough room to patch, we just do it anyway.

@nikitalita
Copy link
Contributor Author

Now, obviously, this might break with other functions; If we want to be extra careful, we could just hardcode an exception for those byte sequences, but that might break with every subsequent Windows build. Unfortunately, I'm not that famililar with x86 assembly or how it's patching these jumps to suggest a more sustainable AND safe alternative.

@nikitalita
Copy link
Contributor Author

following our conversation here: mridgers/clink#543 (comment)

I refactored patching to look prior to the hook target, and also after the hook target for a safe set of bytes to patch into. If it hits a RET while looking prior, it breaks instead of continuing to look, and just continues looking after. This should be safer than what I was doing previously and still be sustainable. I've tested it, it works on Windows Insider build 20226.

Copy link
Owner

@chrisant996 chrisant996 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!

@chrisant996
Copy link
Owner

I'm merging the PR. In the next week or two I'll try switching to use Microsoft Detours 4.x for hooking APIs, so that we don't have to mess with this kind of problem anymore moving forward. (It uses the MIT License.)

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

Successfully merging this pull request may close these issues.

None yet

2 participants