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

Status: RESOLVED FIXED

(Core :: Web Speech, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: 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.

Comment 1

[Julian Seward [:jseward]]

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.

Comment 2

[Bob Clary [:bc] (inactive)]

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.

Comment 3

[Julian Seward [:jseward]]

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.

Comment 4

[Julian Seward [:jseward]]

Attachment: 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.

Comment 5

[Julian Seward [:jseward]]

(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.

Comment 6

[Julian Seward [:jseward]]

Attachment: Initialise mMatches in MediaQueryList::MediaQueryList

Fixes the valgrind complaints I saw.

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: start up asan report

Bughunter saw this again just starting up the browser.

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Julian Seward [:jseward]]

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?

Comment 10

[Bob Clary [:bc] (inactive)]

jseward: I would say it would probably be cleaner to file a new bug.

Comment 11

[Julian Seward [:jseward]]

(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.

Comment 12

[Eitan Isaacson [:eeejay]]

(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?

Comment 13

[Eitan Isaacson [:eeejay]]

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.

Comment 14

[Andrew McCreight [:mccr8]]

(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.

Comment 15

[Andrew McCreight [:mccr8]]

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.

Comment 16

[Eitan Isaacson [:eeejay]]

(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.

Comment 17

[Eitan Isaacson [:eeejay]]

Attachment: Separate construction from off-main-thread setup in SpeechDispatcherService.

Comment 18

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/862b3306d68a

Comment 19

[Eitan Isaacson [:eeejay]]

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.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/862b3306d68a

Comment 21

[Andrew McCreight [:mccr8]]

Adjusting security rating per comment 19.