1665486 - BBC Flash audio/video player freezes, crashing Firefox without a FFx crash dump

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Laurence "GreenReaper" Parry]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:82.0) Gecko/20100101 Firefox/82.0

Steps to reproduce:

I loaded the page https://www.bbc.co.uk/news/av/world-europe-22149846
I clicked play. It told me I needed Flash enabled to play.
I enabled Flash from the plugin icon dropdown on the bar and reloaded the page.
I clicked play.

Actual results:

The player appeared to load, then freeze; then the browser was silently terminated.

While I did not get a crash report, here is an older report to show my configuration:
https://crash-stats.mozilla.org/report/index/07e4062c-2a92-4f78-a942-eaeaa0200911

This reported problem may however correlate with this system crash report:
Problem signature
Problem Event Name: BEX64
Application Name: firefox.exe
Application Version: 82.0.0.7564
Application Timestamp: 5f61f9f3
Fault Module Name: ntdll.dll
Fault Module Version: 10.0.18362.1049
Fault Module Timestamp: b5beef21
Exception Offset: 00000000000a1170
Exception Code: c0000409
Exception Data: 000000000000000a
OS Version: 10.0.18363.2.0.0.768.101
Locale ID: 1033
Additional Information 1: 0ba9
Additional Information 2: 0ba9c20510fd9284d04bb07218b2efa3
Additional Information 3: c1a9
Additional Information 4: c1a9e0882d81b2304764e6c0cf51373c

Extra information about the problem
Bucket ID: 5ab0fe2cbdf5831ff9b4b0a6d28badf0 (1852299577299545584)

Expected results:

The player should have loaded and played the audio. It does on Google Chrome.
There should also have been a crash dump created since the browser crashed.

I used Firefox Nightly 82.0a1 x64 20200916095656, using Flash 32.0.0.433 on Win 10 Home 1909 18363.1082 on an AMD E-350 with Radeon HD 6310 15.201.1151.0 8-21-2015.

Comment 1

[Laurence "GreenReaper" Parry]

Attachment: Windows debug !analyze and exception report

I found a .dmp file for the crash and !analyze -v in windbg suggested "an overrun of a stack-based buffer".

Comment 2

[Andrei Purice]

Hello,
I was able to reproduce this on 82.0a1 (2020-09-20) following the steps provided and indeed it makes the browser crash.
I could not reproduce it on the beta version 81.0 nor the release one 80.0.1 .
Setting a component for this issuein order to get the dev team involved.
If you feel it's an incorrect one please feel free to change it to a more appropriate one.

Comment 3

[Laurence "GreenReaper" Parry]

This player also handles video and thus may have more visibility than I originally expected.

Today I ran across a link in an article posted today that led to coverage of the Japanese Tsunami of 2011:
https://www.bbc.co.uk/news/world-asia-pacific-12709598
The video on this page froze and terminated my browser as before. A WER file was generated, which I will attach. Looks like the same BEX error.

I am not sure if this is related in any way to Bug 1665351 about CFG (or the clang 11 update in Bug 1660340 which triggered it). The former bug appears to have been addressed, but perhaps only for 32-bit builds. I am currently on 83.0a1 (2020-09-22) (64-bit) build 20200922094538.

Yesterday I updated to Windows 10 Build 2004, so it appears this is unrelated (i.e. it crashes on old and new Windows semi-annual builds).

Comment 4

[Laurence "GreenReaper" Parry]

Attachment: Windows Error Reporting report of crash on Windows 10 May Update

Comment 5

[Laurence "GreenReaper" Parry]

Bug 1666484 also exhibits a similar termination and a small regression range has been identified which including the clang 11 update.

Perhaps David would be able to help, or else advise on who might be well-placed to investigate/address this issue?

Comment 6

[(Away)]

This stems from a clang bug, I filed https://bugs.llvm.org/show_bug.cgi?id=47617.

The delayload stub that plays the role of &InternetOpenA in our hook isn't getting marked as CFG-approved.

Since I'm not too familiar with the hooks in dom/plugins/ipc/, I'll need to defer to :handyman -- is this something that we can work around by suppressing CFG on various RunFunction() methods? Or is the use of these functions so pervasive that this would require broader changes?

Comment 7

[Laurence "GreenReaper" Parry]

...and it looks like you beat me to it, but here's what I had typed up, in case any of the members are useful.

The error occurred in the function broker thread when attempting to use InternetOpenA (and later InternetCloseHandle), during a call to Rtlp/LrdpHandleInvalidUserCallTarget which appears to be part of Control Flow Guard.

I was able to debug it with Mozilla symbols (took a while, this is a dual-core 2011 netbook) and obtained the following call stack (which I hope is right, had to hack a return from the ntdll.dll). The crash occurred during the call to __guard_dispatch_icall_fptr

ntdll.dll!LdrpHandleInvalidUserCallTarget()	Unknown

xul.dll!mozilla::plugins::FunctionBroker<mozilla::plugins::ID_InternetOpenA,void *(const char *, unsigned long, const char *, const char *, unsigned long),mozilla::plugins::SslEHContainer>::BrokerCallServer(unsigned long aClientId, const mozilla::plugins::IpdlTuple & aInTuple, mozilla::plugins::IpdlTuple * aOutTuple, const char * & aParams, unsigned long & aParams, const char * & aParams, const char * & aParams, unsigned long & aParams) Line 1379 C++
xul.dll!mozilla::plugins::FunctionBroker<mozilla::plugins::ID_InternetOpenA,void *(const char *, unsigned long, const char *, const char , unsigned long),mozilla::plugins::SslEHContainer>::RunOriginalFunction(unsigned long aClientId, const mozilla::plugins::IpdlTuple & aInTuple, mozilla::plugins::IpdlTuple * aOutTuple) Line 1203 C++
xul.dll!mozilla::plugins::FunctionBrokerParent::RecvBrokerFunction(const mozilla::plugins::FunctionHookId & aFunctionId, const mozilla::plugins::IpdlTuple & aInTuple, mozilla::plugins::IpdlTuple * aOutTuple) Line 101 C++
xul.dll!mozilla::plugins::PFunctionBrokerParent::OnMessageReceived(const IPC::Message & msg__, IPC::Message * & reply__) Line 138 C++
xul.dll!mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message && aMsg) Line 2070 C++
xul.dll!mozilla::ipc::MessageChannel::MessageTask::Run() Line 1955 C++
xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1234 C++
xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate) Line 332 C++
xul.dll!MessageLoop::RunHandler() Line 328 C++
xul.dll!MessageLoop::Run() Line 310 C++
xul.dll!nsThread::ThreadFunc(void * aArg) Line 444 C++
nss3.dll!_PR_NativeRunThread(void * arg) Line 421 C
nss3.dll!pr_root(void * arg) Line 140 C
ucrtbase.dll!thread_start<unsigned int (__cdecl)(void *),1>() Unknown
kernel32.dll!BaseThreadInitThunk() Unknown
ntdll.dll!RtlUserThreadStart() Unknown

members of this at the selected point:
mStub 0x0,
mRegistration 0x0
mModuleName wininet.dll
mFunctionName InternetOpenA
aInTuple had an mTupleEvents with an mHdr with length/capacity 5
aOutTuple had a sEmptyTArrayHeader.
aParams was Shockwave Flash

Comment 8

[(Away)]

Does this build work? https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/ZEglJ-2ORr24nLVKVYOoAw/runs/0/artifacts/public/build/target.zip

Comment 9

[Laurence "GreenReaper" Parry]

The build correctly plays the audio and video at both of the above links, including on multiple pages at once and setting it fullscreen, exiting Firefox halfway through and after restoring tabs on startup (after re-enabling Flash).

Comment 10

[(Away)]

Attachment: Disable CFG for FunctionBroker hook returns

Due to a bug in clang, some brokered function thunks don't get CFG-approved. As a workaround this patch disables CFG when we return to the original thunks. Note that, unlike previous issues with CFG and hooks, this affects 64-bit builds too.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67acd8531789
Disable CFG for FunctionBroker hook returns r=handyman

Comment 13

[Alexandru Michis [:malexandru]]

Backed out changeset 67acd8531789 (Bug 1665486) for causing hazard bustages in FunctionBroker.h

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=316590054&repo=autoland&lineNumber=16072

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b82d40128e88
Backed out changeset 67acd8531789 for causing hazard bustages in FunctionBroker.h

Comment 15

[Laurence "GreenReaper" Parry]

(In reply to Alexandru Michis [:malexandru] from comment #13)

Backed out changeset 67acd8531789 (Bug 1665486) for causing hazard bustages in FunctionBroker.h

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=316590054&repo=autoland&lineNumber=16072

Looks like something like this is needed for GCC compatibility:

#ifndef __has_declspec_attribute         // Optional of course.
  #define __has_declspec_attribute(x) 0  // Compatibility with non-clang compilers.
#endif

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d3754d87d25
Disable CFG for FunctionBroker hook returns r=handyman

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8d3754d87d25

Comment 18

[(Away)]

Comment on attachment 9177446 [details]
Disable CFG for FunctionBroker hook returns

Beta/Release Uplift Approval Request

• User impact if declined: Flash crashes
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: https://bugzilla.mozilla.org/show_bug.cgi?id=1665486#c3
  and
  https://bugzilla.mozilla.org/show_bug.cgi?id=1666484#c0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Targeted set of plugin-specific functions, reverting them to clang-9 behavior
• String changes made/needed: no

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 9177446 [details]
Disable CFG for FunctionBroker hook returns

approved for 82.0b4

Comment 20

[Laurence "GreenReaper" Parry]

I verified that the issue no longer impacts Nightly using build 20200925094208 on the original machine (Windows 10 x64).
Firefox Developer (82.0b3) on my Surface Pro crashes the same way; I can check that once it updates, if nobody else does.

Nightly build 2020092312316 did not crash on my 32-bit hardware running Windows 10 preview build 20221. It still does not on Nightly build 20200925094208. Perhaps because this issue impacts thunks? Because I could only reproduce it on x86_64, I've set the platform to that.

I understand Control Flow Guard was available as an update for Windows 8.1 (KB3000850). Given the nature of this issue and its fix, I doubt there'll be a difference; but if someone has a box with an updated version of Windows 8.1 to QA on, it may be worth doing a quick test on it (and lowering platform version if impacted). 8.1 is still supported to 2023, but my own machines are all on Windows 10 by now.

Unsure if S1 given crash, or S2 since only occurs in restricted circumstances (but 100% repeatable). I did not experience noticeable data loss, but I did have to switch browser to watch the content, so I picked S2. Feel free to change.

Comment 21

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/378193ed98cb

Comment 22

[Laurence "GreenReaper" Parry]

Verified fixed on 82.0b4 build 20200926073307 x64. Thanks! Let's hope LLVM's proposed patch for the underlying issue works out too.

Comment 23

[Brindusa Tot, Desktop QA]

Based on comments 22 and 20, this issue was verified. Removing the qe-verify+ flag.

Comment 24

[Tom Ritter [:tjr]]

Is there/should there be a bug on file for reverting this when the llvm patch is available in a version we're using?

Comment 25

[Chris Peterson [:cpeterson]]

(In reply to Tom Ritter [:tjr] (ni? for response to sec-[advisories/bounties/ratings/cves]) from comment #24)

Is there/should there be a bug on file for reverting this when the llvm patch is available in a version we're using?

btw, we will be turning off Flash support (meta bug 1455897) in Fx85 in Nightly later this month. Perhaps we can revert this change then even without a patched llvm?

Comment 26

[(Away)]

I assume this code will just go away when we drop Flash support.