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

RESOLVED FIXED in mozilla9


8 years ago
a year ago


(Reporter: armenzg, Assigned: glandium)


Windows Server 2008
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(4 attachments, 1 obsolete attachment)

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

See bug 606473 as well.


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

Comment 5

8 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).

Comment 6

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

Comment 7

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

Comment 8

8 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)

Comment 9

8 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)


8 years ago
Attachment #555423 - Attachment is obsolete: true

Comment 10

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

Comment 13

8 years ago
Backed out, because it traded the win64 orange with a win debug one :(

Comment 14

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

Comment 15

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

This adds support for jumps on x86
Attachment #556790 - Flags: review?(vladimir)
(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.


8 years ago
Whiteboard: fixed-in-bs → [inbound]
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Duplicate of this bug: 653361
Depends on: 972671


a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.