1625452 - The target.cppunittest.tests fails to hook QueryDosDeviceW from kernelbase.dll for TestDllInterceptor.exe

Status: VERIFIED FIXED

(Core :: mozglue, defect)

Description

[Alexandru Trif, Desktop QA [:atrif]]

Attachment: failed dll.png

Affected versions

• 75.0b10 (20200326191140)

Affected platforms

• Windows 10 x32

Steps to reproduce

1. Download target.cppunittest.tests.zip and target.zip from https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=c6d1ad75d7ca1ab66cf4b269ef73e3020a4abc90 (Fx75.0b10 - Windows 2012 shippable opt Bpgo-B).
2. Unzip the content.
3. Copy mozglue.dll from target.zip in the unzipped file from above.
4. Open the PowerShell window in the folder the content was extracted in the previous step.
5. Type .\TestDllInterceptor.exe and hit Enter.

Expected result

• All checks passed.

Actual result

• TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to hook QueryDosDeviceW from kernelbase.dll First 5 bytes of function: 6A 7C 68 D0 F0

Regression Range

• Reproducible with Firefox 75.0b8, Firefox 75.0b9 and Firefox 75.0b1. Will search for one ASAP.

Notes

• Attached a screenshot with the issue.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 2

[Julien Cristau [:jcristau] (back April 22)]

Toshi, David, can you take a look?

Comment 3

[David Parks [:handyman]]

What happens if you try the test in a regular console (instead of PowerShell)?

Comment 4

[David Parks [:handyman]]

FYI, I am asking because of Bug 1601071.

Comment 5

[Alexandru Trif, Desktop QA [:atrif]]

Attachment: dll_interceptorw10x86.png

(In reply to David Parks (dparks) (he/him) [:handyman] from comment #3)

What happens if you try the test in a regular console (instead of PowerShell)?

Same error with cmd too.

TestDllInterceptorCrossProcess.exe test pass.

Comment 6

[Toshihito Kikuchi [:toshi]]

I think I reproduced this issue. When our detour meets the pattern of 6A 7C 68 D0 F0, it can process the first instruction 6a 7c (= push 7c). After that, its head needs to jump to the 3rd byte 68, which is push imm32, but it jumped to the 4th byte D0. Probably something wrong happened in CountPrefixBytes. Let me continue investigation and find out the root cause..

Comment 7

[Toshihito Kikuchi [:toshi]]

I got the root cause and came up with a simple patch. I'll b e able to start a review by EOD tomorrow.

Comment 8

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1625452 - No need to pass an offset to CountPrefixBytes. r=handyman

When our detour processes instructions, we pass ReadOnlyTargetFunction to
CountPrefixBytes to determine whether a lock prefix exists or not.
In that case, we don't need to pass both ReadOnlyTargetFunction and an offset
as a parameter because ReadOnlyTargetFunction has an offset as a member.

Comment 9

[Julien Cristau [:jcristau] (back April 22)]

What's the likely effect of this issue beyond the failed test? Is it bad enough that we should get the fix in 75 (we're in RC week)?

Comment 10

[Toshihito Kikuchi [:toshi]]

(In reply to Julien Cristau [:jcristau] from comment #9)

What's the likely effect of this issue beyond the failed test? Is it bad enough that we should get the fix in 75 (we're in RC week)?

An immediate impact is our detour fails to hook QueryDosDeviceW here, so ChromiumCDMAdapter may not function correctly. I'm not sure how it emerges, though. Media team can answer that..?

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

I believe the reason we hook this function is so the GMP doesn't need to use the system version, which means we can make the sandbox more restrictive. So I think if we fail to hook we'll run into the sandbox when we're trying to playback premium media (Netflix, Amazon prime, etc.). My preference from the media side would be try and uplift fixes if possible.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

It should be possible to check on a Windows 10 32-bit environment (which doesn't sound like its common) to see if those (Netflix, Amazon Prime, ...) work or not. I'm guessing that will define our urgency in uplifting.

Note that this was found by our own tests, if this had actually been broken in beta this might've been reported?

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Also I'm not clear what regressed this, or if this had been broken in previous versions?

Comment 14

[Julien Cristau [:jcristau] (back April 22)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #12)

It should be possible to check on a Windows 10 32-bit environment (which doesn't sound like its common) to see if those (Netflix, Amazon Prime, ...) work or not. I'm guessing that will define our urgency in uplifting.

Alexandru, can you test this?

Note that this was found by our own tests, if this had actually been broken in beta this might've been reported?

I'd hope so, but probably better to make sure... Thanks :)

Comment 15

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Wonder if this is the cause of bug 1496607.

Comment 16

[Toshihito Kikuchi [:toshi]]

(In reply to Bryce Seager van Dyk (:bryce) from comment #15)

Wonder if this is the cause of bug 1496607.

I believe it is. As shown below (that's from my local system where no repro), KernelBase!QueryDosDeviceW starts with push 7Ch, followed by a code pushing an immediate value. This detour failure occurs depending on a number of that immediate value, which means this issue could easily disappear/reappear release by release.

KernelBase!QueryDosDeviceW:
1012e5e0 6a7c            push    7Ch
1012e5e2 6898321b10      push    offset KernelBase!HviIsAnyHypervisorPresent+0xcdc (101b3298)

My proposed patch affects only 32bit binaries (i.e. 32bit Windows or WOW64), and the change is simple. The regression risk from uplift is low.

Comment 17

[Alexandru Trif, Desktop QA [:atrif]]

Attachment: console-export-2020-4-3_9-13-38.txt

(In reply to Julien Cristau [:jcristau] from comment #14)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #12)

It should be possible to check on a Windows 10 32-bit environment (which doesn't sound like its common) to see if those (Netflix, Amazon Prime, ...) work or not. I'm guessing that will define our urgency in uplifting.

Alexandru, can you test this?

I get the attached console errors when trying to play videos on Netflix or amazon prime. Tested with two machines. On one machine Netflix was working after trying to update the widevine plugin (even if no updates were installed) and playing another video (I could not check the other machine due to boot errors) but amazon prime was not at all on neither on the machines. On the other machine, Netflix was not working. These errors were presented in Firefox 75.0 (20200331175109) win 10x86. If more information is needed please let me know.

Comment 18

[Alexandru Trif, Desktop QA [:atrif]]

So I retested on another win10x86 machine. It seems that Netflix is working after a browser restart.

Comment 19

[Catalin Sasca, Desktop QA [:csasca]]

Tested on Amazon Prime too and from the first try, an error message is shown that the stream cannot be played. After restarting the PC, and accessing Amazon Prime again, movies/TV series loaded as expected. This happened on a Win 10 x86 system with Firefox 75.0.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc31229fd23f
No need to pass an offset to CountPrefixBytes.  r=handyman

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/bc31229fd23f

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Given that this is Windows 10 32-bit only (which is likely not very common?), restarting can make it work and it's been there for a while, it probably does not make sense to try to uplift it aggressively to a dot release.

Comment 23

[Catalin Sasca, Desktop QA [:csasca]]

Attachment: Win 10 x86 dll.png

Verified the fix on 76 (beta - treeherder) under Windows 10 32-bit and all tests are passed.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Part of me thinks we should take this on ESR68 since this breaks potentially-important functionality, but the other part of me says we've made it this far into it's life cycle without anybody complaining and I wonder who will notice. Calling this wontfix for now but I'm open to arguments for why we should if someone feels strongly about it.