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)
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•7 years ago
|
Attachment #8871454 -
Flags: review?(aklotz) → review?(dmajor)
Updated•7 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•7 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•7 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•7 years ago
|
Keywords: checkin-needed
Comment 11•7 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•7 years ago
|
||
bugherder |
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.
Description
•