Skip to content

Commit

Permalink
[INLINEHOOK] Update thread PC correctly when unhooking
Browse files Browse the repository at this point in the history 
This change from the bug found by @hakujitsu7 in Microsoft Detours PR microsoft#331:
microsoft#331, but matches rbCode region explicitly.
  • Loading branch information
@RatinCN
RatinCN committed Feb 3, 2025
1 parent 98f45e6 commit 35aa2ca
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions Source/SlimDetours/Thread.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@ detour_thread_update(
bUpdateContext = FALSE;
if (o->fIsRemove)
{
if (cxt.CONTEXT_PC >= (ULONG_PTR)o->pTrampoline &&
cxt.CONTEXT_PC < ((ULONG_PTR)o->pTrampoline + sizeof(o->pTrampoline)))
if (cxt.CONTEXT_PC >= (ULONG_PTR)o->pTrampoline->rbCode &&
cxt.CONTEXT_PC < ((ULONG_PTR)o->pTrampoline->rbCode + RTL_FIELD_SIZE(DETOUR_TRAMPOLINE, rbCode)))
{
cxt.CONTEXT_PC = (ULONG_PTR)o->pbTarget +
detour_align_from_trampoline(o->pTrampoline, (BYTE)(cxt.CONTEXT_PC - (ULONG_PTR)o->pTrampoline));
Expand Down

2 comments on commit 35aa2ca

@hakujitsu7
Copy link

Choose a reason for hiding this comment

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

Great improvement! This fix ensures that the thread's program counter is correctly checked against rbCode, making the detour removal process more robust. However, on the x64 architecture, the PC may also point to instructions within rbCodeIn, not just rbCode. To fully cover this case, it might be worth extending the condition to include rbCodeIn as well. This would help prevent potential edge cases where the PC is not properly updated during unhooking.

@RatinCN
Copy link
Member Author

@RatinCN RatinCN commented on 35aa2ca Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Great improvement! This fix ensures that the thread's program counter is correctly checked against rbCode, making the detour removal process more robust. However, on the x64 architecture, the PC may also point to instructions within rbCodeIn, not just rbCode. To fully cover this case, it might be worth extending the condition to include rbCodeIn as well. This would help prevent potential edge cases where the PC is not properly updated during unhooking.

PR #2 is opened for covering this case, and invite you @hakujitsu7 to review it.

Please sign in to comment.