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

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ccorcoran, Assigned: ccorcoran)

Tracking

unspecified
mozilla55
x86_64
Unspecified
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8871454 - Flags: review?(aklotz) → review?(dmajor)
Attachment #8871455 - Flags: review?(aklotz) → review?(dmajor)

Comment 3

2 years ago
mozreview-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

::: 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 4

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-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/#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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5945f26f0ad6
https://hg.mozilla.org/mozilla-central/rev/794a1edad2b4
Status: NEW → RESOLVED
Last Resolved: 2 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.