forked from martona/mhook
-
Notifications
You must be signed in to change notification settings - Fork 63
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge branch 'fix-remove-max-limit-patch'
- Loading branch information
Showing
4 changed files
with
175 additions
and
1,010 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
e96bed5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BAD CODE, DON'T USE IT, IT NOT WORK IN Mhook_Unhook
e96bed5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RelicOfTesla Could you provide a sample code that shows the bug?
e96bed5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e96bed5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e96bed5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SergiusTheBest @poizan42 martona#2
e96bed5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RelicOfTesla @poizan42 Confirming, unhooking doesn't work in the provided sample:
e96bed5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a patch which makes unhooking work. This was tested on e58a58c via mhook-test.cpp. I was compiling in Cygwin with i686-w64-mingw32-gcc 4.8.3, with a few other minor changes to enable compiling with GCC. Those changes aren't included in the patch and shouldn't affect this issue.
The second change shows why unhooking currently fails. Mhook_Unhook(PVOID _ppHookedFunction) calls TrampolineGet((PBYTE)_ppHookedFunction). In other words, Mhook_Unhook() gets a pointer to a pointer, and it passes the value of that pointer to TrampolineGet(). The value of that pointer points to the trampoline code which jumps to the original function. So, it has to be compared to the location where that trampoline code would be in pCurrent.
The first change fixes a crash while removing the last element from a list of trampolines. In that case pNode->pNextTrampoline is NULL, and so *pListHead becomes NULL above the assert, causing NULL pointer dereferencing when evaluating argument of assert.
The patch contains tabs, and I'm not sure how to make them visible here. They appear in the source of the comment if I edit it, but if I copy from the displayed comment I get spaces.
e96bed5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dreamlayers Looks good. The MessageBoxA sample is working now. Thanks.
e96bed5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a pull request with that unhook fix: martona#3