Closed Bug 1363280 Opened 2 years ago Closed 2 years ago

Intermittent damp | application crashed [@ mozilla::layers::AutoLayerTransactionParentAsyncMessageSender::AutoLayerTransactionParentAsyncMessageSender(mozilla::layers::LayerTransactionParent *,nsTArray<mozilla::layers::OpDestroy> const *)]

Categories

(Core :: Graphics: Layers, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: intermittent-bug-filer, Assigned: sotaro)

Details

(4 keywords, Whiteboard: [gfx-noted][post-critsmash-triage][adv-main54+][adv-esr52.2+])

Crash Data

Attachments

(4 files, 2 obsolete files)

Attached file crash stack
23:39:40     INFO -  Crash address: 0xffffffffffffffff might be a sec issue
Group: core-security
The negative one crash address is only semi-interesting (it's near-null, after all), but rax = 0xe5e5e5e5e5e5e5e5 is clearly a marker of use-after-free. rcx is similar.
Group: core-security → gfx-core-security
Flags: needinfo?(dvander)
QA Whiteboard: [gfx-noted]
QA Whiteboard: [gfx-noted]
Whiteboard: [gfx-noted]
Neat. This function calls [1] into [2] into [3]. And it turns out sImageBridges is totally bogus, there are *assertions* that it's modified on the main thread, when it's clearly only ever read on the compositor thread. If we spawn or shutdown a content process when a video composites, the std::map could be corrupted.

The easiest thing to do would be to throw a lock around all accesses of the map. I'm a bit overloaded at the moment, Milan is there anyone else who could take this?

[1] http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/gfx/layers/ipc/LayerTransactionParent.cpp#114
[2] http://searchfox.org/mozilla-central/rev/02cae69058d41e2c6b635edccbec91a6ca2cb57f/gfx/layers/ipc/ImageBridgeParent.cpp#383
[3] http://searchfox.org/mozilla-central/rev/02cae69058d41e2c6b635edccbec91a6ca2cb57f/gfx/layers/ipc/ImageBridgeParent.cpp#334
Flags: needinfo?(dvander) → needinfo?(milan)
Assignee: nobody → sotaro.ikeda.g
Flags: needinfo?(milan)
We could remove the followings from AutoLayerTransactionParentAsyncMessageSender since they were necessary for gonk.
 - ImageBridgeParent::SetAboutToSendAsyncMessages();
 - ImageBridgeParent::SendPendingAsyncMessages();

But we need to keep ImageBridgeParent::sImageBridges for ImageBridgeParent::NotifyImageComposites().
Then, We could change sImageBridges as to add/remove entry only from Compositor thread.
attachment 8870722 [details] [diff] [review] does not work, since ImageBridgeParent::ActorDestroy is not always called on compositor thread.
Attachment #8870722 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> attachment 8870722 [details] [diff] [review] does not work, since
> ImageBridgeParent::ActorDestroy is not always called on compositor thread.

Hrm, are you sure? ActorDestroy should not be called from any other thread, since it either comes off the MessageChannel or via Close, which has a thread assertion.
The destructor might be called from any thread.
(In reply to David Anderson [:dvander] from comment #8)
> (In reply to Sotaro Ikeda [:sotaro] from comment #7)
> > attachment 8870722 [details] [diff] [review] does not work, since
> > ImageBridgeParent::ActorDestroy is not always called on compositor thread.
> 
> Hrm, are you sure? ActorDestroy should not be called from any other thread,
> since it either comes off the MessageChannel or via Close, which has a
> thread assertion.

Sorry, I was wrong, I checked with CompositorThreadHolder::IsInCompositorThread(). But it returns false since CompositorThreadHolder::Shutdown().
attachment 8871153 [details] [diff] [review] moved "sImageBridges.erase(OtherPid())" from ImageBridgeParent::~ImageBridgeParent() to ImageBridgeParent::ActorDestroy(). By it, the entry is always removed on CompositorThread.
Attachment #8871153 - Flags: review?(dvander)
Attachment #8871153 - Flags: review?(nical.bugzilla)
Comment on attachment 8871153 [details] [diff] [review]
patch - Fix Update sImageBridges handling

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

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +342,5 @@
>    mCompositorThreadHolder = nullptr;
>    mSelfRef = nullptr; // "this" ImageBridge may get deleted here.
>  }
>  
>  ImageBridgeParent*

I think this might be safe as-is, but it's better to make this return a RefPtr<ImageBridgeParent> to eliminate any possibility of a race with the refcount dropping to 0.
Attachment #8871153 - Flags: review?(dvander) → review+
Attachment #8871153 - Flags: review?(nical.bugzilla) → review+
Applied the comment.
Attachment #8871153 - Attachment is obsolete: true
Attachment #8872496 - Flags: review+
Comment on attachment 8872496 [details] [diff] [review]
patch - Fix Update sImageBridges handling

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

> It is not easy to exploit based on the patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

> No.

Which older supported branches are affected by this flaw?

> The bug affects since Firefox 32.

If not all supported branches, which bug introduced the flaw?

> Bug 1006957.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

> No, but it is easy to create a patch and it is not risky.

How likely is this patch to cause regressions; how much testing does it need?

> It is not likely to cause a regression. I tested locally and on tryserver, it seems enough.
Attachment #8872496 - Flags: sec-approval?
Want to get Release Management's take on whether we have time to take this and still get it into Beta and ESR52 as well.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
We will gtb b13 on Thursday so yes we can take it.
Flags: needinfo?(rkothari)
Comment on attachment 8872496 [details] [diff] [review]
patch - Fix Update sImageBridges handling

Please nominate patches for Beta and ESR52 approval, to be checked in after mozilla-central.
sec-approval+
Attachment #8872496 - Flags: sec-approval? → sec-approval+
Looks like this'll need rebased patches for Beta and ESR52.
Flags: needinfo?(lhenry) → needinfo?(sotaro.ikeda.g)
https://hg.mozilla.org/mozilla-central/rev/29fa3eb2f2fb
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Flags: needinfo?(sotaro.ikeda.g)
Comment on attachment 8872496 [details] [diff] [review]
patch - Fix Update sImageBridges handling

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1006957
[User impact if declined]: A process that owns compositor might crash.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No. low risk.
[Why is the change risky/not risky?]: The patch basically just add protection by Monitor and remove unnecessary code except gonk. gonk is invalid platform now.
[String changes made/needed]:None.
Attachment #8872496 - Flags: approval-mozilla-esr52?
Attachment #8872496 - Flags: approval-mozilla-beta?
Attachment #8872496 - Flags: approval-mozilla-aurora?
Comment on attachment 8872496 [details] [diff] [review]
patch - Fix Update sImageBridges handling

Sec-fix, Beta54+, ESR52+
Attachment #8872496 - Flags: approval-mozilla-esr52?
Attachment #8872496 - Flags: approval-mozilla-esr52+
Attachment #8872496 - Flags: approval-mozilla-beta?
Attachment #8872496 - Flags: approval-mozilla-beta+
Attachment #8872496 - Flags: approval-mozilla-aurora?
Flags: qe-verify-
Whiteboard: [gfx-noted] → [gfx-noted][post-critsmash-triage]
Group: gfx-core-security → core-security-release
Whiteboard: [gfx-noted][post-critsmash-triage] → [gfx-noted][post-critsmash-triage][adv-main54+][adv-esr52.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.