Closed Bug 1367899 Opened 7 years ago Closed 7 years ago

Add handling of call reg; test r/m32, r32; and jne rel8 opcodes in CreateTrampoline disassembler

Categories

(Core :: General, enhancement)

Product:
Core
Component:
General
Platform:
x86_64
Unspecified
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ccorcoran, Assigned: ccorcoran)

Details

Attachments

(2 files)

Bug 1367899: Add handling for test r/m32, r32 and jne rel8 opcodes;
59 bytes, text/x-review-board-request
away
: review+
Details
Bug 1367899: Add handling for call reg opcode;
59 bytes, text/x-review-board-request
away
: review+
Details 
During the work on bug 1322554, we uncovered a few unhandled opcodes in the x64 CreateTrampoline function:

call reg
test r/m32, r32
jne rel8
Attachment #8871454 - Flags: review?(aklotz) → review?(dmajor)
Attachment #8871455 - Flags: review?(aklotz) → review?(dmajor)
Comment on attachment 8871454 [details]
Bug 1367899: Add handling for test r/m32, r32 and jne rel8 opcodes;

https://reviewboard.mozilla.org/r/142926/#review146660

::: xpcom/build/nsWindowsDllInterceptor.h:1135
(Diff revision 1)
>            return;
>          }
>          COPY_CODES(2 + nModRmSibBytes);
> +      } else if (origBytes[nOrigBytes] == 0x85) {
> +        // test r/m32, r32
> +        COPY_CODES(2);

This is not always two bytes: https://pastebin.mozilla.org/9022745

Maybe we should make it match the other "0x85" case around line ~936? (Although it might be better to rewrite those 0xc0's as kModReg etc.)

::: xpcom/build/nsWindowsDllInterceptor.h:1160
(Diff revision 1)
>          nTrampBytes = jump.GenerateJump(tramp);
>          nOrigBytes += 5;
> +      } else if (origBytes[nOrigBytes] == 0x75) {
> +        // jne rel8
> +        char offset = origBytes[nOrigBytes + 1];
> +        JumpPatch jump(nTrampBytes, (intptr_t)(origBytes + nOrigBytes + 2 +

Nit: I recommend putting the intptr_t(... + offset) on its own line so that it's all together.

Also, (your choice) I wouldn't be opposed to expanding this to cover 0x74/JE as well.
Attachment #8871454 - Flags: review?(dmajor)
Comment on attachment 8871455 [details]
Bug 1367899: Add handling for call reg opcode;

https://reviewboard.mozilla.org/r/142928/#review146670
Attachment #8871455 - Flags: review?(dmajor) → review+
Comment on attachment 8871454 [details]
Bug 1367899: Add handling for test r/m32, r32 and jne rel8 opcodes;

https://reviewboard.mozilla.org/r/142926/#review146660

> This is not always two bytes: https://pastebin.mozilla.org/9022745
> 
> Maybe we should make it match the other "0x85" case around line ~936? (Although it might be better to rewrite those 0xc0's as kModReg etc.)

The whole disassembler design seems a bit unruly; for the moment I'm just slipping in without disrupting too much. I think many opcodes could be unified.

> Nit: I recommend putting the intptr_t(... + offset) on its own line so that it's all together.
> 
> Also, (your choice) I wouldn't be opposed to expanding this to cover 0x74/JE as well.

Done! Good call.
Comment on attachment 8871454 [details]
Bug 1367899: Add handling for test r/m32, r32 and jne rel8 opcodes;

https://reviewboard.mozilla.org/r/142926/#review147894

I have no idea if I'm clicking the right button here but my intent is to mark r+.
Attachment #8871454 - Flags: review?(dmajor) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5945f26f0ad6
Add handling for test r/m32, r32 and jne rel8 opcodes; r=dmajor
https://hg.mozilla.org/integration/autoland/rev/794a1edad2b4
Add handling for call reg opcode; r=dmajor
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5945f26f0ad6
https://hg.mozilla.org/mozilla-central/rev/794a1edad2b4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.