1489391 - The target.cppunittest.tests fails to hook SetWindowLongPtrA from user32.dll for TestDllInterceptorCrossProcess.exe

Status: VERIFIED FIXED

(Core :: mozglue, defect, P1)

Description

[Cristian Fogel [:cfogel]]

Attachment: 63dll interceptor.png

[Affected versions]:
- 63.0b3(20180904170936)

[Affected platforms]: 
- Windows 8 64/32bit 
- Windows 8.1 64/32bit 

[Steps to reproduce]:
1. Download target.cppunittest.tests.zip from https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=026931928157
(look for Windows 2012 x64 opt N if using Windows 64bit and the Windows 2012 opt N if using Windows 32bit)
2. Unzip its content
3. Open the Command Prompt (in the folder created in step 2),  type in TestDllInterceptorCrossProcess.exe and press Enter. 
   - per bug 1461654, the missing dll error is triggered, but it can be avoided if taking the missing files from the target.zip) 

[Expected result]:
- Per https://bugzilla.mozilla.org/show_bug.cgi?id=1451511#c16, all of output displayed after running the TestDllInterceptorCrossProcess.exe file is: TEST-PASS

[Actual result]:
-  "TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to add hook" is triggered for SetWindowLongPtrA; 
- after bug 1467798

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Can you please provide the version numbers for user32.dll on those platforms?

Just right-click on user32.dll in explorer, click properties, and then go to the Version tab.

Comment 2

[Cristian Fogel [:cfogel]]

Win_8:   6.2.9200.17044
Win_8.1: 6.3.9600.18535

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

I'm looking for somebody to take this.

Comment 4

[David Parks [:handyman]]

Attachment: wip.patch

Comment 5

[David Parks [:handyman]]

Some additional info:

• The issue is with TestDllInterceptor.exe (not TestDllInterceptorCrossProcess.exe).
• You can run the TestDllInterceptor.exe in dist/bin by copying mozglue.dll from dist/mozglue to dist/bin and installing VCRuntime from [1].
• The disease has spread since this report. Using Windows 8.1 x64, I am seeing additional failures. Remembering that the trampoline needs 13 bytes, the offenders and their x64 machine codes are:
  • CloseHandle: ff 25 ca 21 12 00 90 90 90 90 90 90 ff 25 1e 1a
    • The second rip-relative jmp starts at byte 13. CloseHandle could work as a 10-byte trampoline. I don't see another safe solution at this point.
  • SetWindowLongPtrA: 41 b9 01 00 00 00 e9 a5 f8 ff ff 41 b8 ff ff 00
    • The issue again is instructions in the first 13 bytes that follow a jmp.
    • The disassembly is:
      0x0000000000000000: 41 B9 01 00 00 00 mov r9d, 1
      0x0000000000000006: E9 A5 F8 FF FF jmp 0xfffffffffffff8b0
      [...]
      so the "41 b8" bytes follow the jmp and are in the trampoline space. Again, this could work with a 10-byte trampoline.
  • SetWindowLongPtrW: 45 33 c9 e9 48 ff ff ff b2 01 e8 71 fe ff ff 48
    • Disassembly:
      0x0000000000000000: 45 33 C9 xor r9d, r9d
      0x0000000000000003: E9 48 FF FF FF jmp 0xffffffffffffff50
      0x0000000000000008: B2 01 mov dl, 1
      0x000000000000000a: E8 71 FE FF FF call 0xfffffffffffffe80
    • I'm at a loss as to what to do here. We can only handle the first 8 bytes so even the 10-byte patch would fail. Presumably something jmps to the mov at 0x8 or else it would be worthless, so its not ok to overwrite it.
    • The usage is for crash-protecting Flash WndProc.

---

I'm attaching a patch that forces 10-byte patches for CloseHandle and SetWindowLongPtrA. I think this is the right solution in those cases; it fixes them in the test. For SetWindowLongPtrW, I need some advice -- NI to Aaron.

---

[1] https://www.microsoft.com/en-us/download/details.aspx?id=52685

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

For SetWindowLongPtrW, I need some advice -- NI to Aaron.

For however long this hasn't been working, are we seeing a significant number of crashes in the wild due to our inability to set this hook? If not, maybe we should just skip this test on Win8.1.

Otherwise my thought is that we leverage some of the stuff I wrote for ARM64 where we find a memory region within the range of an int32 displacement and set up a veneer, however that seems like a bit much work for something that might not matter all that much.

Comment 7

[David Parks [:handyman]]

I'm going to dig into why we are hooking both SetWindowLongPtrW and SetWindowLongPtrA to try to find a way to avoid SetWindowLongPtrW. Maybe we'll get lucky and find that Flash isn't using it anymore.

Comment 8

[David Parks [:handyman]]

I've spent a day looking at this with Flash and I have not found any uses of SetWindowLongPtrW (or SetWindowLongPtrA for that matter). I've tried with/without async rendering, normal and fullscreen, wmode as unspecified and as "window" [1]. While its possible that some untested behavior triggers this, I am nearly certain that its just part of the old windowed handlers. If that is the case then we can stop hooking both of these -- but if we don't want to find out I'm wrong about this, we could just cut the hooks in Win 8. The rest tests fine now and should get dumped soon anyway.

This is the relevant hook [2]. NI to jimm to see if he can confirm that the crash-protected window handler is not used e.g. for invisible Flash windows. Unless jimm thinks it is in use, I'm going to suggest we skip this in Win 8 only and hook the other 2 with 10-byte patches as in the wip above.

--

[1] https://wiki.mozilla.org/Plugins/Async_Drawing
[2] https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/dom/plugins/ipc/PluginInstanceChild.cpp#1825

Comment 9

[David Parks [:handyman]]

(If jimm is certain that we don't use them then lets totally rip them out.)

Comment 10

[Jim Mathies [:jimm]]

(In reply to David Parks (dparks) [:handyman] from comment #7)

I'm going to dig into why we are hooking both SetWindowLongPtrW and SetWindowLongPtrA to try to find a way to avoid SetWindowLongPtrW. Maybe we'll get lucky and find that Flash isn't using it anymore.

We removed all the windowing modes, so I think we can rip all of this subclass code out (or maybe just disable it). The SetWindowLongPtrA calls we're for an older version of flash as well, which is blocked. So we don't need those hooks either.

Comment 11

[David Parks [:handyman]]

That's the same conclusion I came to. I'll post a patch today.

Comment 12

[David Parks [:handyman]]

Attachment: Bug 1489391: Part 1 - Use 10-byte patch in DLL interceptor for CloseHandle on Win8/8.1 r=aklotz!

CloseHandle has a jump followed by enough nops to fit a 10-byte patch but not enough to fit the default 13-byte patch when running Windows 8 or 8.1. This patch tells the interceptor to use a 10-byte patch on those OSs.

Comment 13

[David Parks [:handyman]]

Attachment: Bug 1489391: Part 2 - Remove SetWindowLong*/SetWindowLongPtr* from TestDllInterceptor r=aklotz!

SetWindowLong*/SetWindowLongPtr* was being intercepted so that we could override windowprocs in windowed plugins on Windows. We no longer support windowed plugins so these functions are never intercepted.

Depends on D55535

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdffefea6e9a
Part 1 - Use 10-byte patch in DLL interceptor for CloseHandle on Win8/8.1 r=aklotz
https://hg.mozilla.org/integration/autoland/rev/33fef95ff8cf
Part 2 - Remove SetWindowLong*/SetWindowLongPtr* from TestDllInterceptor r=aklotz

Comment 15

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/bdffefea6e9a
https://hg.mozilla.org/mozilla-central/rev/33fef95ff8cf

Comment 16

[Cristian Fogel [:cfogel]]

With our routine verification on the DLL Interceptors, starting with 69.0b16-build1 this issue was no longer encountered and reported.
A more detailed report can be found here.

Marking the issue as verified with 73.0b1-build2, as our latest verified version.