Closed
Bug 1367899
Opened 8 years ago
Closed 8 years ago
Add handling of call reg; test r/m32, r32; and jne rel8 opcodes in CreateTrampoline disassembler
Categories
(Core :: General, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ccorcoran, Assigned: ccorcoran)
Details
Attachments
(2 files)
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) |
Updated•8 years ago
|
Attachment #8871454 -
Flags: review?(aklotz) → review?(dmajor)
Updated•8 years ago
|
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•8 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•8 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•8 years ago
|
Keywords: checkin-needed
Comment 11•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5945f26f0ad6
https://hg.mozilla.org/mozilla-central/rev/794a1edad2b4
Status: NEW → RESOLVED
Closed: 8 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.
Description
•