TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to add hook

RESOLVED FIXED in mozilla9

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: armenzg, Assigned: glandium)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla9
x86_64
Windows Server 2008
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Bug 680818 got fixed and we can now pass the compilation step and reach this make check failure.

See bug 606473 as well.

From:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1314087809.1314099252.14141.gz#err0

Running TestDllInterceptor tests
NEXT ERROR TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to add hook
make[4]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit/xre/test/win'
NEXT ERROR make[4]: *** [check] Error 1
make[3]: *** [check] Error 2
make[3]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit/xre/test'
make[2]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit/xre'
make[2]: *** [check] Error 2
make[1]: *** [check] Error 2
make[1]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit'
make: *** [check] Error 2
program finished with exit code 2
elapsedTime=301.292000
TinderboxPrint: check<br/>12518/<em class="testfail">1</em>
Unknown Error: command finished with exit code: 2
GetVersionExA entry has too complex MOV.  (mov r64, [rel32])

We hook LdrLoadDll, SetWindowLongPtr (SetWindowLong on Win32), and etc, but we don't inject GetVersionEx.  I believe that this isn't good test case for injection.

We should turn off this test or modify test case.
Modifying the test case to the functions we actually hook sounds like a good way to go.
(Reporter)

Comment 3

6 years ago
How long will that take (roughly)? days or more?
Could we disable the test while it gets fixed?

In the next few days I hope to have try support in case fixing the test will be easier that way.

You probably know but just to be explicit, this is the last bug that I need to be fixed before I can show the win64 builds on tbpl.
CCing the test author.
(Assignee)

Comment 5

6 years ago
The problem is finding a function that can be verified to have been called successfully when hooked (not the hook, the original function).
(Assignee)

Comment 6

6 years ago
We hook:
- GetWindowInfo
- SetWindowLongA/SetWindowLongPtrA
- SetWindowLongW/SetWindowLongPtrW
- TrackPopupMenu
- LdrLoadDll

None of these seem to fit comment 5, except the first one, which requires a window...
(Assignee)

Comment 7

6 years ago
Created attachment 555423 [details] [diff] [review]
Try intercepting a function inside the executable in TestDllInterceptor

This works for me, but merely by luck. I doubt it works on x64. We'd have to find the right contents for the fillPreload function such that the generated code is supported by the interceptor code. We could go as far as actually writing well-suited assembly.
(Assignee)

Comment 8

6 years ago
Created attachment 555704 [details] [diff] [review]
part 0 - Support a few more opcodes and fix x86 mov r, [r] test

My x86 machine code is a bit rusty, but this should be correct.

Mainly, I needed sub r, imm8 and mov r, [r+imm8] on x86 ; rex.wr and mov r, [r] on x64. These are the most common instructions I was getting when trying various functions for the test case, with or without optimization.
The x64 part could probably be refactored further, and share with x86, but I didn't feel like going much further.

I also spotted a bug in the test for mov r, [r] on x86, that tested REG instead of R/M to filter out the [SIB] and disp32  cases that take more bytes.
Attachment #555704 - Flags: review?(vladimir)
(Assignee)

Comment 9

6 years ago
Created attachment 555705 [details] [diff] [review]
part 1 - Intercept a function from the executable in TestDllInterceptor

This makes the test hook a custom function instead of GetVersionEx. It's using UINT64 values mainly because the x64 dll interceptor doesn't support mov with 32 bit registers.
I tested with MSVC8 on x86 and x64, with and without optimization in both cases, and it passed.
Attachment #555705 - Flags: review?(vladimir)
(Assignee)

Updated

6 years ago
Attachment #555423 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Created attachment 555706 [details] [diff] [review]
part 2 - Test WindowsDllInterceptor on functions we intercept in our codebase

This adds a test to verify that we can indeed hook the functions that our codebase hooks and that are listed in comment 6.
Attachment #555706 - Flags: review?(vladimir)
Comment on attachment 555704 [details] [diff] [review]
part 0 - Support a few more opcodes and fix x86 mov r, [r] test

I'm going to assume the asm decoding is correct :)
Attachment #555704 - Flags: review?(vladimir) → review+
Attachment #555705 - Flags: review?(vladimir) → review+
Attachment #555706 - Flags: review?(vladimir) → review+
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/76e73aad0fab
http://hg.mozilla.org/integration/mozilla-inbound/rev/be03169f5635
http://hg.mozilla.org/integration/mozilla-inbound/rev/1aed3d723632
Whiteboard: [inbound]
(Assignee)

Comment 13

6 years ago
Backed out, because it traded the win64 orange with a win debug one :(
http://hg.mozilla.org/integration/mozilla-inbound/rev/9f7ccdc68e3f
(Assignee)

Comment 14

6 years ago
So, what is happening on win debug is that the function GetProcAddress returns is actually the ILT (import lookup table) entry for the function, and that's only a jump to the real function. We don't support jumps in the interceptor on x86. This is due to -DEBUG turning off -OPT:REF, which actually gets rid of these. So building the executable with -DEBUG -OPT:REF works, but this doesn't sound very robust: there's no guarantee that -OPT:REF will continue to reliably remove these ILT entries. The other option is to add support for jumps in the interceptor.
(Assignee)

Comment 15

6 years ago
Created attachment 556790 [details] [diff] [review]
Addition to part 0

This adds support for jumps on x86
Attachment #556790 - Flags: review?(vladimir)
M-i merge of landing:
http://hg.mozilla.org/mozilla-central/rev/76e73aad0fab
http://hg.mozilla.org/mozilla-central/rev/be03169f5635
http://hg.mozilla.org/mozilla-central/rev/1aed3d723632

M-i merge of subsequent backout:
http://hg.mozilla.org/mozilla-central/rev/9f7ccdc68e3f
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Whiteboard: [inbound]
(Reporter)

Comment 17

6 years ago
I pushed the change to try to see how it behaves:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=62214c5e8cf8

For comparison I will look at:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=76e73aad0fab
(Reporter)

Comment 18

6 years ago
(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #17)
> I pushed the change to try to see how it behaves:
> http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=62214c5e8cf8
> 
> For comparison I will look at:
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=76e73aad0fab

The results are satisfactory.
Attachment #556790 - Flags: review?(vladimir) → review+
(Assignee)

Comment 19

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/5dda9668493b
http://hg.mozilla.org/integration/mozilla-inbound/rev/38bbf1e8ae5d
http://hg.mozilla.org/integration/mozilla-inbound/rev/ce43a8644bc0
Whiteboard: fixed-in-bs
(Assignee)

Updated

6 years ago
Whiteboard: fixed-in-bs → [inbound]
http://hg.mozilla.org/mozilla-central/rev/5dda9668493b
http://hg.mozilla.org/mozilla-central/rev/38bbf1e8ae5d
http://hg.mozilla.org/mozilla-central/rev/ce43a8644bc0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9

Updated

6 years ago
Duplicate of this bug: 653361

Updated

4 years ago
Depends on: 972671
You need to log in before you can comment on or make changes to this bug.