1328996 - Add support to the DLL Interceptor for RIP-relative displacements

Status: RESOLVED FIXED

(Core :: General, defect)

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

To be more explicit: there is a case in WindowsDllDetourPatcher::CountModRmSib that is currently unsupported on x64 and returns -1. This is because on x64 those 32-bit displacements are RIP-relative. We can easily count those bytes in the instruction, however the displacement in the affected instruction needs to be modified when copied into the trampoline in order to be relative to the new RIP.

Comment 2

[Jim Mathies [:jimm]]

Hey David, you've worked on the dll interceptor recently, we need to add some additional functionality to that to get accessibility turned off on windows 8 touch screen users. Can you pick this up?

Comment 3

[David Parks [:handyman]]

Sure.  It'll probably be late next week before I get to this though.

Comment 4

[Jim Mathies [:jimm]]

(In reply to David Parks [:handyman] from comment #3)
> Sure.  It'll probably be late next week before I get to this though.

sounds good, thanks.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

You won't need Windows 8 to test this, of course. In my case, I could find a reproducible case in tiptsf.dll!ProcessCaretEvents.

Note that tiptsf.dll is located at:
\Program Files\Common Files\Microsoft Shared\Ink

I have modified TestDllInterceptor to work with this. I'll attach that patch here so that you don't have to repeat that work in order to repro this.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: TestDllInterceptor enhancements

Comment 7

[David Parks [:handyman]]

Attachment: 1. Add ability to patch 64-bit MOV instructions to the DLL interceptor

The other patches in this bug are about running very simple tests of the patched functions to make sure that the harness e.g. doesn't immediately crash.  Its a simple sanity check.  Unfortunately, since the signature for ProcessCaretEvents isn't known, the main method this bug references isn't one of a few not included in the test.

Comment 8

[David Parks [:handyman]]

Attachment: 2. Enable tiptsf.dll's ProcessCaretEvents DLL Interceptor test on 64-bit

Comment 9

[David Parks [:handyman]]

Attachment: 3. Add tests that run the intercepted DLL methods

A few notes:

* Obviously, the tests of these methods are not thorough but I figure if we made a mistake in the machine code, its more likely to cause a crash than not.  Since the harness takes 14 bytes, we can be fairly sure we cover the preamble of the method when we run these basic tests.
* The tests are a little wordy.  This could have been done with generics but the result would have been awfully complex.  This initial catch-up is large but adding a new method to the test won't take more than a half-dozen lines.
* The tests have caught a two harness failures (on my Win10 machine).  They are in CreateFileA and SetCapture.  I haven't fixed them because I don't believe the patched methods are ever _used_ and repairs won't be too easy.  Instead, I've added messages about them.  If we decide to go this route, I'll create a new bug for them.
* Two methods, ProcessCaretEvents and ImmSetCandidateWindow, aren't added.  As mentioned above, ProcessCaretEvents isn't tested because the method signature isn't published.  ImmSetCandidateWindow isn't tested because I don't have a good test for it (it seems to crash when given invalid parameters).  Unfortunately, this means the test doesn't check the MOV code added in patch #1.

Comment 10

[David Parks [:handyman]]

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cf92fc113695aa5d75fe84b871c9856096350ac

Comment 11

[David Parks [:handyman]]

Update on SetCapture:

I added that to the DLLInterceptor test in bug 1284897.  There, I added a number of methods that were hooked in the code base but weren't added to the TestDllInterceptor.  Since I wrote that patch, the hook for that method has (apparently) been removed.  So I'll need to remove it from bug 1284897... and from this patch.  So that's one less harness failure.

OTOH, the CreateFileA stub is potentially used here:
https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/plugins/ipc/PluginModuleChild.cpp#1927

Comment 12

[David Parks [:handyman]]

Attachment: 1. Add ability to patch 64-bit MOV instructions to the DLL interceptor

Refreshing the patch

Comment 13

[David Parks [:handyman]]

Attachment: 2. Enable tiptsf.dll's ProcessCaretEvents DLL Interceptor test on 64-bit

Refreshing the patch

Comment 14

[David Parks [:handyman]]

Attachment: 3. Add tests that run the intercepted DLL methods

Refreshing the patch and removing the SetCapture stuff as explained in comment 11.

Comment 15

[David Parks [:handyman]]

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5e5de7ebea50633908391a560d78eaea2d02f80

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8835152 [details] [diff] [review]
1. Add ability to patch 64-bit MOV instructions to the DLL interceptor

Review of attachment 8835152 [details] [diff] [review]:
-----------------------------------------------------------------

r=me when comments are addressed. Thanks!

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +505,5 @@
>  
> +  // Special ModR/M codes.  These indicate operands that cannot be simply
> +  // memcpy-ed.
> +  // Operand is a 64-bit RIP-relative address.
> +  static const int MODRM_OPERAND_64 = -2;

Nit: Can you please name follow the same naming convention that are used for most other constants in this file?

@@ +521,4 @@
>    int CountModRmSib(const BYTE *aModRm, BYTE* aSubOpcode = nullptr)
>    {
>      if (!aModRm) {
> +      MOZ_ASSERT_UNREACHABLE("Missing ModRM byte");

This should just be a regular MOZ_ASSERT, I think.

@@ +899,5 @@
>            COPY_CODES(1);
>            // MOV r/m64, r64 | MOV r64, r/m64
>            int len = CountModRmSib(origBytes + nOrigBytes);
>            if (len < 0) {
> +            switch (len) {

For the sake of readability and consistency with our "Return from errors immediately" coding style, I think we should transform this switch statement into
MOZ_ASSERT(len == MODRM_OPERAND_64);
if (len != MODRM_OPERAND_64) {
  return;
}

// Remaining code goes here

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8835155 [details] [diff] [review]
3. Add tests that run the intercepted DLL methods

Review of attachment 8835155 [details] [diff] [review]:
-----------------------------------------------------------------

r=me when comments addressed.

::: toolkit/xre/test/win/TestDllInterceptor.cpp
@@ +144,5 @@
> +// interceptor.  They check that the patched assembler preamble does
> +// something sane.  The parameter is a pointer to the patched function.
> +bool TestGetWindowInfo(void* aFunc)
> +{
> +  typedef BOOL(WINAPI *GetWindowInfoType)(HWND, PWINDOWINFO);

Can you use decltype for functions that have header declarations? Something like
auto patchedGetWindowInfo = reinterpret_cast<decltype(&GetWindowInfo)>(aFunc)

@@ +149,5 @@
> +  GetWindowInfoType patchedGetWindowInfo =
> +    reinterpret_cast<GetWindowInfoType>(aFunc);
> +  return patchedGetWindowInfo(0, 0) == FALSE;
> +}
> +bool TestSetWindowLongPtr(void* aFunc)

Nit: please add blank lines between the closing brace of the previous function and the beginning of the next function definition.

@@ +365,5 @@
> +}
> +
> +bool TestProcessCaretEvents(void* aFunc)
> +{
> +  printf("TEST-SKIPPED | WindowsDllInterceptor | "

Actually we know this, it is a WinEventProc:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd373885(v=vs.85).aspx

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #17)
> Comment on attachment 8835155 [details] [diff] [review]
> Actually we know this, it is a WinEventProc:
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd373885(v=vs.85).
> aspx

Its function pointer is also typedef'd in Windows headers as WINEVENTPROC, BTW.

Comment 19

[(Away)]

Needinfo for myself to try this with clang-cl when it lands. (Normally I'd ask you to make sure this is green with clang-cl before pushing, but TestDllInterceptor is already busted on that platform, so the burden is on me.)

Comment 20

[David Parks [:handyman]]

Attachment: 1. Add ability to patch 64-bit MOV instructions to the DLL interceptor

> Actually we know this, it is a WinEventProc:
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd373885(v=vs.85).
> aspx

Cool.  I have no idea how you figured that out but it was quite helpful -- it allowed me to run the test and learn that I had ended up completely misinterpreting the problem.  This version of the patch sets up the trampoline correctly.  We only hit the RAX destination register case, for which there is a special opcode we can use, but I also included the non-RAX case for good measure.

Comment 21

[David Parks [:handyman]]

Attachment: 2. Enable tiptsf.dll's ProcessCaretEvents DLL Interceptor test on 64-bit (r=aklotz)

Freshen patch

Comment 22

[David Parks [:handyman]]

Attachment: 3. Add tests that run the intercepted DLL methods (r=aklotz)

> Can you use decltype for functions that have header declarations? Something
> like
> auto patchedGetWindowInfo = reinterpret_cast<decltype(&GetWindowInfo)>(aFunc)

Quite an improvement.  Thanks!

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=657209ff283794a666a2302a78a9368ca2ab0674

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8838673 [details] [diff] [review]
1. Add ability to patch 64-bit MOV instructions to the DLL interceptor

Review of attachment 8838673 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see another iteration on this with respect to the REX prefixes.

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +913,5 @@
> +              reinterpret_cast<int64_t*>(origBytes + nOrigBytes + 4 +
> +                                         *reinterpret_cast<int32_t*>(origBytes + nOrigBytes));
> +            nOrigBytes += 4;
> +
> +            if (reg == 0) {

Let's define constants for the registers up by the mod r/m stuff.
kRegEax = 0 ... kRegEdi = 7

@@ +916,5 @@
> +
> +            if (reg == 0) {
> +              // Destination is RAX.  Encode instruction as MOVABS with a
> +              // 64-bit absolute address as its immediate operand.
> +              tramp[nTrampBytes] = 0xa1;

Don't you need to insert a REX prefix here?

@@ +926,5 @@
> +              // The MOV must be done in two steps.  First, we MOVABS the
> +              // absolute 64-bit address into our target register.
> +              // Then, we MOV from that address into the register
> +              // using register-indirect addressing.
> +              tramp[nTrampBytes] = 0xb8 + reg;

Don't you need a REX prefix here?

@@ +933,5 @@
> +              *trampOperandPtr = absAddr;
> +              nTrampBytes += 8;
> +              tramp[nTrampBytes] = 0x48;
> +              tramp[nTrampBytes+1] = 0x8b;
> +              tramp[nTrampBytes+2] = (reg << 3) | reg;

s/3/kRegFieldShift/

... actually this is a bit too ad-hoc for my liking. Can we add something like:

inline BYTE BuildModRmByte(BYTE aModBits, BYTE aReg, BYTE aRm)
{
  MOZ_ASSERT((aRm & kMaskRm) == aRm);
  MOZ_ASSERT((aModBits & kMaskMod) == aModBits);
  MOZ_ASSERT(((aReg << kRegFieldShift) & kMaskReg) == (aReg << kRegFieldShift));
  return aModBits | (aReg << kRegFieldShift) | aRm;
}

So that we can then write this as:
tramp[nTrampBytes+2] = BuildModRmByte(kModNoRegDisp, reg, reg);

I know this is a bit more work (sorry for that), but I'm really trying to encourage "cleaning up as we go" with respect to the interceptor code.

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

Also don't forget that you will need to propagate the R and B bits from the original instruction's REX prefix as well.

Comment 25

[David Parks [:handyman]]

Attachment: 1. Add ability to patch 64-bit MOV instructions to the DLL interceptor

This addresses all of the nits in comment 23.

> Let's define constants for the registers up by the mod r/m stuff.
> kRegEax = 0 ... kRegEdi = 7

I went with kRegAx...kRegDi just so that things would be valid in 32-bit.  (My case would actually have been kRegRax since I mean 64-bit.)

> Don't you need to insert a REX prefix here?

The ModRM byte is actually (still) copied in line 841 of the parent bug (bug 1284897)'s patch:

https://bugzilla.mozilla.org/attachment.cgi?id=8839770&action=edit

Fun with deeply nested conditionals.  Note that this only allows the bit-combinations W and WR.

Comment 26

[David Parks [:handyman]]

Attachment: 2. Enable tiptsf.dll's ProcessCaretEvents DLL Interceptor test on 64-bit (r=aklotz)

Patch refresh

Comment 27

[David Parks [:handyman]]

Attachment: 3. Add tests that run the intercepted DLL methods (r=aklotz)

Patch refresh

Comment 28

[David Parks [:handyman]]

Updated tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f148e62896634ccd799ac109c01da891c3cd1e8f

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abc0e81104d5
Add ability for 64-bit MOV instructions (those with REX.W bit set) to be patched in the DLL interceptor harness. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ae5d202106
Enable tiptsf.dll's ProcessCaretEvents DLL Interceptor test on 64-bit Windows. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/06c838c29b6f
Add tests that run the intercepted DLL methods, in order to test their harness. r=aklotz

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abc0e81104d5
https://hg.mozilla.org/mozilla-central/rev/d7ae5d202106
https://hg.mozilla.org/mozilla-central/rev/06c838c29b6f

Comment 31

[(No longer employed by Mozilla) Aaron Klotz]

Please request uplift to beta for these patches as well as bug 1330460.

Comment 32

[(Away)]

(In reply to David Major [:dmajor] from comment #19)
> Needinfo for myself to try this with clang-cl when it lands. (Normally I'd
> ask you to make sure this is green with clang-cl before pushing, but
> TestDllInterceptor is already busted on that platform, so the burden is on
> me.)

This looks fine modulo the existing bustage, for which I've filed bug 1343149. Any interest in taking it?

Comment 33

[Jim Mathies [:jimm]]

(In reply to Aaron Klotz [:aklotz] from comment #31)
> Please request uplift to beta for these patches as well as bug 1330460.

Do we need this work up on 53? We have a11y + e10s disabled in release builds, and this in 54 which megres next week to aurora so I think we're ok here.

Comment 34

[(No longer employed by Mozilla) Aaron Klotz]

My request was mainly to give Win64 users parity with Win32 (which is up on 52), but it isn't essential.