1025078 - AddressSanitizer: Crash at nsPerformance::GetDOMTiming()

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

I just saw this crash, testing with my new Mac ASan build on OS X 10.8.5.

http://people.mozilla.org/~stmichaud/bmo/firefox-asan.dmg
http://people.mozilla.org/~stmichaud/bmo/firefox-asan-howto.txt

I can't find any of these in bmo or Socorro, and they happen in code that hasn't been changed recently.  But given the high quality of ASan crash stacks, I figure it's worth opening a bug.

Note that the main process appears to have been interacting with a plugin when the crash happened -- my stack (when I attach it) will show the plugin-container process aborting after the main process has died.

Though you can't tell (directly) from the stack, the plugin is probably Flash.  My Mac ASan builds, though universal, only use ASan in the 64-bit component.  And I can't think of any other 64-bit plugins.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: ASan crash log

The crashes happen here:

https://hg.mozilla.org/mozilla-central/annotate/48eee276b1ee/dom/base/nsPerformance.h#l272

As best I can tell, the value of mDOMTiming is invalid -- not NULL.  If so, then it must have been initialized with an invalid value, here:

https://hg.mozilla.org/mozilla-central/annotate/48eee276b1ee/dom/base/nsPerformance.cpp#l370

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Er, not a security bug I think. Just a null pointer crash (null + offset)

Comment 3

[Steven Michaud [:smichaud] (Retired)]

I actually don't think it's a null pointer dereference -- see comment #1.

But I agree that it's not security sensitive -- I don't think it can possibly be a use-after-free.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Based on the log it is null + offset crash (which is effectively a null pointer crash).

Comment 5

[Steven Michaud [:smichaud] (Retired)]

So a call to nsRefPtr<blah>::get() on a NULL (or unitialized) nsRefPtr object crashes, like here?

I'd have thought it'd return NULL (and not crash).

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

This should do it. As far as I see we end up doing null_nsPerformance->mDOMTiming access. So, null+offset

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> As far as I see we end up doing null_nsPerformance->mDOMTiming access. So, null+offset

OK, now I get it.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8439998 [details] [diff] [review]
patch

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

Is it correct that the window returns a null Performance object without an error?
I think the issue is here:

https://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#3643

3643   if (mPerformance || !mDoc) {
3644     return;
3645   }

Smaug, what do you think about this?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I guess we could make GetPerformance throw in case of null.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

Comment 11

[Steven Michaud [:smichaud] (Retired)]

For what it's worth, I just got another one of these.  They must be more common than I thought.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8440014 [details] [diff] [review]
v2

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

lgtm

Comment 13

[Boris Zbarsky [:bzbarsky]]

Mark it not nullable in the IDL too, if it's no longer nullable?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

bz prefers .Throw
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c7598b60128

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Too late :/.


I wonder why the attribute got [Throws] and ?, yet the spec doesn't have either one.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Because the spec has no concept of "window without a document".

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

That doesn't explain both [Throws] and ?.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c7598b60128

Comment 19

[Boris Zbarsky [:bzbarsky]]

The [Throws] was due to the fallible forward-to-inner-window...

Comment 20

[(Away)]

#22 crash on v30. Not very high on 31 beta, but perhaps that might change when it reaches the release population. Should we uplift?

Comment 21

[Robert Kaiser]

I think if we uplift depends on the risk of the patch. Saving .5% of our crashes on release is a small step, but one in the right direction. And this happens across all OSes and versions to some degree, see https://crash-stats.mozilla.com/report/list?signature=nsPerformance%3A%3ANow%28%29

Smaug, do you think the risk/reward of this patch warrants an uplift?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Very low risk.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8440071 [details] [diff] [review]
v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 965860
User impact if declined: crashes
Testing completed (on m-c, etc.): landed to m-c two weeks ago
Risk to taking this patch (and alternatives if risky): shouldn't be risky
String or IDL/UUID changes made by this patch: NA

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fba1326dd727
https://hg.mozilla.org/releases/mozilla-beta/rev/be6b13f262cc

Comment 25

[Bogdan Maris, Desktop Test Engineering]

Steven, is there a way I can reproduce this crash in order to verify that the issue is fixed?

Comment 26

[Steven Michaud [:smichaud] (Retired)]

> Steven, is there a way I can reproduce this crash in order to verify that the issue is fixed?

No, these weren't easily reproducible.

Comment 27

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Looking through Socorro (https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=nsPerformance%3A%3ANow%28%29#tab-reports) I don't see any more crashes reported for:
- Firefox 31 Beta builds after build 20140623175014 (the fix entered Beta on June 26)
- Firefox 32 Aurora builds after build 20140625004001 (the fix entered Aurora on June 26)
- Firefox 33 Nightly builds after build 20140612030349 (the fix entered Nightly on June 13)