Closed Bug 1511054 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- disabled
firefox65 --- fixed

People

(Reporter: decoder, Assigned: kats)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [post-critsmash-triage])

Attachments

(2 files)

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.
Flags: sec-bounty?
kats, any ideas? This looks like a race condition of some sort.
Flags: needinfo?(kats)
Group: core-security → gfx-core-security
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
Assignee: nobody → kats
Flags: needinfo?(kats)
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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Flags: needinfo?(mh+mozilla)
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.
Attachment #9028979 - Flags: sec-approval?
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.)
Attachment #9028979 - Flags: sec-approval?
(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
https://hg.mozilla.org/mozilla-central/rev/4a7e5e05432e
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: sec-bounty? → sec-bounty+
> 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.
Flags: needinfo?(mh+mozilla)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.