Closed Bug 1495512 Opened 4 years ago Closed 4 years ago
Users report Firefox 64 is no longer opening/starting up when Dr
.Web is installed
Bug 1495512: Part 1 - DLL Interceptor - Add capability to do 10-byte detour patches against ntdll; r?handyman!
47 bytes, text/x-phabricator-request
|Details | Review|
47 bytes, text/x-phabricator-request
|Details | Review|
Hey Adam, can you assist with outreach? We're expecting this 3rd party to address whatever the issue is here. https://company.drweb.com/contacts/moscow/?lng=en
Reaching out to Dr.Web by email.
Hi All, My name is Alex and I present Dr.Web. We find some problems in FireFox patcher. Our hook looks like ntdll!NtMapViewOfSection: 00007ffd`24d3a3e0 b81089743e mov eax,3E748910h 00007ffd`24d3a3e5 4863c0 movsxd rax,eax 00007ffd`24d3a3e8 ffe0 jmp rax 00007ffd`24d3a3ea 250803fe7f and eax,offset SharedUserData+0x308 (00000000`7ffe0308) 00007ffd`24d3a3ef 017503 add dword ptr [rbp+3],esi 00007ffd`24d3a3f2 0f05 syscall 00007ffd`24d3a3f4 c3 ret 00007ffd`24d3a3f5 cd2e int 2Eh Instruction movsxd rax,eax used to reduce size of patch. Function WindowsDllPatcherBase::ResolveRedirectedAddress(FARPROC aOriginalFunction) (firefox \mozglue\misc\interceptor\PatcherBase.h) don't detect redirection to 0x3E748910 that looks like kd> u 3E748910 00000000`3e748910 4c894c2420 mov qword ptr [rsp+20h],r9 00000000`3e748915 4c89442418 mov qword ptr [rsp+18h],r8 00000000`3e74891a 4889542410 mov qword ptr [rsp+10h],rdx 00000000`3e74891f 48894c2408 mov qword ptr [rsp+8],rcx 00000000`3e748924 4883ec78 sub rsp,78h 00000000`3e748928 488b05f9080100 mov rax,qword ptr [00000000`3e759228] 00000000`3e74892f 4889442458 mov qword ptr [rsp+58h],rax 00000000`3e748934 488b442458 mov rax,qword ptr [rsp+58h] In result AddHook decide to patch ntdll!NtMapViewOfSection instead of 0x3E748910. But function CreateTrampoline don't know jmp rax opcode (ffe0) and return error. Distance from NtMapViewOfSection begin to end of jump opcode is only 10 bytes, but CreateTrampoline expected 13 so return error in any case. I suggest to add pattern for our stub to ResolveRedirectedAddress and hook target function. Thanks, Alex
Component: General → Other
Product: Firefox → External Software Affecting Firefox
Version: Trunk → unspecified
OK, so here's an update: As pointed out in Comment 3, there are some incompatibilities between the way the Dr. Web's detour patcher works compared to ours. Theirs does a 10-byte patch by virtue of ensuring that their hook lies in the first 2GB of virtual address space, thus allowing the use of movsxd to expand that address to 64-bits in rax. Our disassembler does not track the values of registers, so ResolveRedirectedAddress cannot follow their hook; instead, our interceptor code attempts to disassemble enough of the patched NtMapViewOfSection to place its own hook. Unfortunately the Dr. Web hook does not write out nop or int 3 instructions to the remaining bytes of the final instruction that they patched, so it throws our disassembler for a loop (since that instruction is now corrupted). The easiest way for us to modify our interceptor code is to add this 10-byte patching capability to our own code.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Priority: P2 → P1
The above try build works with Dr. Web.
Depends on D10285
confirmed can work with Dr.Web
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/17212e7dfe29 Part 1 - DLL Interceptor - Add capability to do 10-byte detour patches against ntdll; r=handyman https://hg.mozilla.org/integration/autoland/rev/78154ca1e2ac Part 2 - Add test for 10-byte DLL interception; r=handyman
Backed out for failing win MinGW builds Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=210961699&revision=78154ca1e2ac4b5f07276ab9969883d517940d9d Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=210961699&repo=autoland&lineNumber=15693 Backout: https://hg.mozilla.org/integration/autoland/rev/bb6f1073c5ce8beb459b69df723ee812ab94a7e8
I still don't understand why we're backing out tier-2 failures.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/7ae82cae37d9 Part 1 - DLL Interceptor - Add capability to do 10-byte detour patches against ntdll; r=handyman https://hg.mozilla.org/integration/autoland/rev/0cd30c3d9b30 Part 2 - Add test for 10-byte DLL interception; r=handyman
Backout by email@example.com: https://hg.mozilla.org/integration/autoland/rev/ac4d133f8fb8 Backed out 2 changesets for Windows build bustages CLOSED TREE
(In reply to Aaron Klotz [:aklotz] from comment #12) > I still don't understand why we're backing out tier-2 failures. Depending on the occurrence rate, we backout tier2 failures as well
(In reply to Pulsebot from comment #15) > Backout by firstname.lastname@example.org: > https://hg.mozilla.org/integration/autoland/rev/ac4d133f8fb8 > Backed out 2 changesets for Windows build bustages CLOSED TREE Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=211523703&revision=0cd30c3d9b303429f3fe2b72a5b1312ab4f53a23 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=211523703&repo=autoland&lineNumber=43558 Backout: https://hg.mozilla.org/integration/autoland/rev/ac4d133f8fb8
Naturally PGO works just fine on my local machine...
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/09573684485b Part 1 - DLL Interceptor - Add capability to do 10-byte detour patches against ntdll; r=handyman https://hg.mozilla.org/integration/autoland/rev/a31b6b37d11c Part 2 - Add test for 10-byte DLL interception; r=handyman
Component: Other → XPCOM
Product: External Software Affecting Firefox → Core
Target Milestone: --- → mozilla65
Hi, I managed to reproduce this issue in older versions of Firefox but I can no longer reproduce this issue using the latest Nightly 65.0a1 (2018-11-28). This issue was tested on: Windows 10 x86 - x64 Windows 7 x86 - x64 Windows 8.1 x86 - x64
Please nominate this for Beta approval when you get a chance.
Beta approval is not necessary. This issue only showed up with the launcher process, which is only enabled on Nightly.
Based on Comment 25 I will mark this issue accordingly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.