Closed Bug 1227848 Opened 7 years ago Closed 7 years ago

AddressSanitizer: heap-use-after-free with Text to Speech

Categories

(Core :: Web Speech, defect)

44 Branch
Unspecified
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: bc, Assigned: eeejay)

References

(Blocks 1 open bug, )

Details

(Keywords: csectype-race, csectype-uaf, sec-other, Whiteboard: [post-critsmash-triage][adv-main46-])

Attachments

(5 files)

Attached file Asan report
1. https://m.youtube.com/#/watch%3Fv=ijHAqMYYJ-g
2. On RHEL6 x86_64 asan build I got *once*

AddressSanitizer: heap-use-after-free
fetch_add add inc operator++ operator++

See attachment for full details.

I assume this is due to bug 1003464.

I have not been able to reproduce this manually. The original report was using a 20151121035032 build.

Even though this is not reproducible, I hope this will help diagnose the issue. Hopefully this is not just another wild heap issue from other issues which have yet to be patched.
I tried the URL but wasn't sure what the STRs are.  I get a bunch of
videos offered, but no specific video ready to play.  Also I don't
see anything that might have to do with text-to-speech functionality.

Do you have any further specifics?  I suspect it might also be that
Youtube produces different pages for the same URL because we're loading
it from different locations.  For sure at least the ads must be different.
Typically for Bughunter, it is just loading the url. I could not reproduce this at all either manually or using automation. I even tried loading the urls for every request originally made.

I thought the Address Sanitizer messages contained enough information it might still be useful even it couldn't be reproduced. I hope this is not just another dupe of the other heap issues I've found.
It might just be a threading bug (race).  We have an object being allocated
by the main thread (T0).  Then it's freed by T23 (speechd init) and later
accessed again by T0.

Without a repro case, though, hard to say.
Attached file Valgrind complaints
There is something fishy about this though.  V reports a bunch
of uninitialised value errors with the same URL now.  And I
didn't see any such errors on a complete overnight V/Mochitest
run on exactly the same tree and build.  Strange.
(In reply to Julian Seward [:jseward] from comment #4)
> There is something fishy about this though.  V reports a bunch
> of uninitialised value errors with the same URL now.

It seems likely that this is caused because MediaQueryList::MediaQueryList
fails to initialise mMatches.  Whether that has anything to do with the
problem that Bob saw initially, I don't know.
Fixes the valgrind complaints I saw.
Attached file start up asan report
Bughunter saw this again just starting up the browser.
Eitan, can you look into this? Thanks.

I'm not sure how controllable from content this is, but I'm going to be conservative and mark it sec-critical.
Flags: needinfo?(eitan)
Bob, I'd like to get the patch in comment 6 reviewed/landed.  However I'm
not sure that it really has anything to do with this bug, other than
being caused by the URL in comment 0.

What should I do?  Move it into its own bug and copy the URL there,
or request review here?
jseward: I would say it would probably be cleaner to file a new bug.
(In reply to Julian Seward [:jseward] from comment #3)
> It might just be a threading bug (race).

No sign of any (obviously) related race when testing with a TSan'd build.
I have no leads.  Sorry.
Group: core-security → dom-core-security
Component: DOM → Web Speech
(In reply to Bob Clary [:bc:] from comment #7)
> Created attachment 8694306 [details]
> start up asan report
> 
> Bughunter saw this again just starting up the browser.

This is pretty wild. I have a runnable in a constructor that takes a ref of |this|, and then unrefs it (by completing the runnable) before the constructor even returns. Because the |new| call never completes, the refcount is 0, and it is destroyed. Am I reading that right?
Flags: needinfo?(eitan)
smaug, does my description above make sense? Do I need to have a RefPtr of |this| in the constructor so it doesn't go away? That seems like a very bad solution.
Flags: needinfo?(bugs)
(In reply to Eitan Isaacson [:eeejay] from comment #12)
> This is pretty wild. I have a runnable in a constructor that takes a ref of
> |this|, and then unrefs it (by completing the runnable) before the
> constructor even returns. Because the |new| call never completes, the
> refcount is 0, and it is destroyed. Am I reading that right?

Your analysis sounds correct to me. It looks like the runnable is being dispatched onto another thread, so that thread's event loop spins independently of the main thread where the runnable was created.
A better (In reply to Eitan Isaacson [:eeejay] from comment #13)
> smaug, does my description above make sense? Do I need to have a RefPtr of
> |this| in the constructor so it doesn't go away? That seems like a very bad
> solution.

You'd probably be better off creating a separate Init() method that does the event loop stuff, after there's a reference to the object. It looks like it is only being created inside this GetInstance() method so it won't be too fragile.
(In reply to Andrew McCreight [:mccr8] from comment #15)
> A better (In reply to Eitan Isaacson [:eeejay] from comment #13)
> > smaug, does my description above make sense? Do I need to have a RefPtr of
> > |this| in the constructor so it doesn't go away? That seems like a very bad
> > solution.
> 
> You'd probably be better off creating a separate Init() method that does the
> event loop stuff, after there's a reference to the object. It looks like it
> is only being created inside this GetInstance() method so it won't be too
> fragile.

Perfect. Thanks for the validation, I'll do that.
Flags: needinfo?(bugs)
Attachment #8701141 - Flags: review?(bugs) → review+
Oops, I missed the sec-approval flag. For the record, I don't think this is a security issue as much as a race that is not easily reproducible. I don't think this is exploitable.
https://hg.mozilla.org/mozilla-central/rev/862b3306d68a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Adjusting security rating per comment 19.
Keywords: sec-criticalsec-other
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.