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)

x86_64
Unspecified
enhancement
Not set
normal

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
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)
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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: