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

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Carl Corcoran [:ccorcoran]]

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 1

[Carl Corcoran [:ccorcoran]]

Attachment: Bug 1367899: Add handling for test r/m32, r32 and jne rel8 opcodes;

Review commit: https://reviewboard.mozilla.org/r/142926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142926/

Comment 2

[Carl Corcoran [:ccorcoran]]

Attachment: Bug 1367899: Add handling for call reg opcode;

Review commit: https://reviewboard.mozilla.org/r/142928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142928/

Comment 3

[(Away)]

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.

Comment 4

[(Away)]

Comment on attachment 8871455 [details]
Bug 1367899: Add handling for call reg opcode;

https://reviewboard.mozilla.org/r/142928/#review146670

Comment 5

[Carl Corcoran [:ccorcoran]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142926/diff/1-2/

Comment 6

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8871455 [details]
Bug 1367899: Add handling for call reg opcode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142928/diff/1-2/

Comment 7

[Carl Corcoran [:ccorcoran]]

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 8

[Carl Corcoran [:ccorcoran]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142926/diff/2-3/

Comment 9

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8871455 [details]
Bug 1367899: Add handling for call reg opcode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142928/diff/2-3/

Comment 10

[(Away)]

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+.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5945f26f0ad6
https://hg.mozilla.org/mozilla-central/rev/794a1edad2b4