1511054 - AddressSanitizer: heap-use-after-free [@ mozilla::layers::APZSampler::~APZSampler] with WRITE of size 8

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 65.0a1-20181128100125-https://hg.mozilla.org/mozilla-central/rev/5c66354bff282452a6f1a3c911fa8756b6e752af.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Andrew McCreight [:mccr8]]

kats, any ideas? This looks like a race condition of some sort.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Same as bug 1502495 probably. But the stacks here are useful.

It looks like the "threadsafe refcounting" in APZSampler is not as threadsafe as I expected. What could happen is that:

(a) StopAndClearResources sets mApzSampler to nullptr [1].
(b) This decrements the refcount to 0, which triggers a delete on the object.
(c) Before the delete runs, i.e. at [2], we thread-switch to the RenderBackend thread which runs through SampleForWebRender [3].
(d) SampleForWebRender wraps a rawptr stored in a static table with a RefPtr for the duration of the function. This will increment the refcount back to 1 and then drop it back to 0, and delete the object.
(e) We switch back to the compositor thread and run the delete from (c) which is a double-delete and crashes.

I'll see if I can fix this.

[1] https://searchfox.org/mozilla-central/rev/e22c0d152060f4f8d4ca8904094f15f65a1b6f93/gfx/layers/ipc/CompositorBridgeParent.cpp#491
[2] https://searchfox.org/mozilla-central/rev/e22c0d152060f4f8d4ca8904094f15f65a1b6f93/xpcom/base/nsISupportsImpl.h#627
[3] https://searchfox.org/mozilla-central/rev/e22c0d152060f4f8d4ca8904094f15f65a1b6f93/gfx/layers/apz/src/APZSampler.cpp#76

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I wrote a patch to just store RefPtrs instead of raw pointers in the static table. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=b346c418a3cc320312f2d04f0056b3d90ccdc8b4 seems happy. I'll do another one with more QR tests just to make sure it doesn't act up on other platforms.

Also the SampleForWebRender call only happens if WebRender is enabled, which means it's restricted to nightly and a subset of the beta population. That might affect the sec-* rating of this bug.

Would it be better for me to put the patch on this bug and jump through the security-patch hoops? Or can I just do it on bug 1502495 and follow regular patch process?

Comment 6

[Christian Holler (:decoder)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> 
> Would it be better for me to put the patch on this bug and jump through the
> security-patch hoops? Or can I just do it on bug 1502495 and follow regular
> patch process?


I recommend fixing the bug here one way or the other, because the trace that led to the fix and the explanation of what is wrong is here. The sec-* rating of the bug is not affected by the fact that it is Nightly-only (and subset of beta), but the rating itself is required first to determine if there is any special process at all (only sec-critical/high bugs require sec-approval).

From reading comment 4, this sounds like there is a use-after-free, which will lead to a double-free. The use-after-free can be exploitable, but in the context of races, it is really hard, so I guess that would be sec-moderate. However, double-free can also be exploitable (even in races), depending on the compiler and allocator. I don't know exactly how jemalloc reacts to double-frees. Mike, can you comment on that?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

My wider try push at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&selectedJob=214754495&revision=47767929f182b3d73db5b6618216ad145e0baa3b showed a bug in my patch.

That should be fixed in this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43bc8050505a8f6f0465b9c1c43d32c5ff7d8b70

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1511054 - Make APZSampler more threadsafe. r?botond

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 9028979 [details]
Bug 1511054 - Make APZSampler more threadsafe. r?botond

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Not easily

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?: none (because WR is now disabled on beta)

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

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: If this lands on current nightly, it will go to beta 65 and will be present for webrender being enabled to a limited population on 65 beta. We don't need to fix it on 64 beta because as of bug 1499088 WR is disabled on 64 beta.

How likely is this patch to cause regressions; how much testing does it need?: Hopefully it won't cause regressions, but if it does, the most likely would be a shutdown crash, I think. Some testing wouldn't hurt but probably more useful to just keep an eye on crash-stats.

Comment 10

[Andrew McCreight [:mccr8]]

If this is really Nightly only, then it doesn't need sec-approval. (I'm assuming that we won't re-enable WR on this particular beta version later or whatever.)

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> If this is really Nightly only, then it doesn't need sec-approval. (I'm
> assuming that we won't re-enable WR on this particular beta version later or
> whatever.)

Thanks. I figured better safe than sorry so I filled out the form anyway :)

Landed: https://hg.mozilla.org/integration/autoland/rev/4a7e5e05432e

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4a7e5e05432e

Comment 13

[Mike Hommey [:glandium]]

> I don't know exactly how jemalloc reacts to double-frees. Mike, can you comment on that?

If it's a clear double-free and the memory hasn't been given out as a new allocation, it crashes.