Use-after-free of memory reporter during tests

RESOLVED FIXED in Firefox 38

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: njn, Assigned: njn)

Tracking

({csectype-uaf})

unspecified
mozilla39
csectype-uaf
Points:
---

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Margaret was seeing ASan failures for test_memoryReporters.xul in bug 1124011. 

The memory reporter manager ("Mgr") has a test-only facility whereby it can save the existing memory reporters, install some temporary ones, do some stuff, and then restore the original ones. The exact problem with this was as follows.

- Reporter R gets registered as a weak reporter. This means Mgr holds a weak reference to it.

- Mgr is instructed to stash all its reporters.

- Reporter R gets unregistered. The unregistration silently fails because stashed reporters don't get considered in this step.

- Reporter R is destroyed.

- Mgr is instructed to unstash all its stashed reporters.

- Mgr iterates over all its reporters, which includes the weak reference to the now-dead R. ASan rightly complains.

I couldn't reproduce this locally but I confirmed it via debugging printfs on the tryserver.

The fix is simple: the third step above should unregister reporters that are stashed. It's a test-only fix, which is good.
(Assignee)

Comment 1

3 years ago
Created attachment 8571722 [details] [diff] [review]
Make memory reporters that have been stashed for testing eligible for unregistration
Attachment #8571722 - Flags: review?(continuation)
Comment on attachment 8571722 [details] [diff] [review]
Make memory reporters that have been stashed for testing eligible for unregistration

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

nice
Attachment #8571722 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/655ddeba3c94
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Comment 5

3 years ago
We are trying to ship reading list/reader view in Firefox 38, so eventually we will want to turn on the toolbar button there as well. Could you uplift this patch to Aurora in anticipation of us enabling this feature on Aurora some time in the coming weeks?
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 6

3 years ago
Comment on attachment 8571722 [details] [diff] [review]
Make memory reporters that have been stashed for testing eligible for unregistration

Approval Request Comment

[Feature/regressing bug #]: This patches fixes a test bug that blocks Reader Mode (bug 1124011) from landing. It's intended to be uplifted to Aurora some time soon.

[User impact if declined]: Reader Mode cannot be uplifted.

[Describe test coverage new/current, TreeHerder]: the problem is with the test itself :)

[Risks and why]: Extremely low. It was a fix to some code that's only used to facilitate testing, and doesn't get used outside of tests.

[String/UUID change made/needed]: None.
Attachment #8571722 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Flags: needinfo?(n.nethercote)
Attachment #8571722 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox38: --- → affected
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.