Tyson Smith [:tsmith] Reporter
	Description • 6 years ago

==71981==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f7a0934f872 bp 0x7ffd9ee67430 sp 0x7ffd9ee67300 T0)
==71981==The signal is caused by a WRITE memory access.
==71981==Hint: address points to the zero page.
    #0 0x7f7a0934f871 in nsCycleCollector::ScanWhiteNodes(bool) src/xpcom/base/nsCycleCollector.cpp:680:3
    #1 0x7f7a0934fd08 in nsCycleCollector::ScanRoots(bool) src/xpcom/base/nsCycleCollector.cpp:3288:3
    #2 0x7f7a09354085 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) src/xpcom/base/nsCycleCollector.cpp:3782:9
    #3 0x7f7a093539d4 in nsCycleCollector::ShutdownCollect() src/xpcom/base/nsCycleCollector.cpp:3701:10
    #4 0x7f7a09355d77 in nsCycleCollector::Shutdown(bool) src/xpcom/base/nsCycleCollector.cpp:4011:5
    #5 0x7f7a09358f0f in nsCycleCollector_shutdown(bool) src/xpcom/base/nsCycleCollector.cpp:4400:23
    #6 0x7f7a09531d0d in mozilla::ShutdownXPCOM(nsIServiceManager*) src/xpcom/build/XPCOMInit.cpp:1010:3
    #7 0x7f7a14e9b25c in XRE_TermEmbedding() src/toolkit/xre/nsEmbedFunctions.cpp:230:3
    #8 0x7f7a0a413c95 in mozilla::ipc::ScopedXREEmbed::Stop() src/ipc/glue/ScopedXREEmbed.cpp:108:5
    #9 0x7f7a14e9bd18 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:772:16
    #10 0x4f2294 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #11 0x4f2294 in main src/browser/app/nsBrowserApp.cpp:287
    #12 0x7f7a28a2382f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #13 0x4216b8 in _start (firefox+0x4216b8)

	Andrew McCreight [:mccr8]
	Comment 2 • 6 years ago

From the stack, it looks like this is crashing in a call to PtrInfo::AnnotatedReleaseAssert().

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 6 years ago

is this a regression?

	Daniel Veditz [:dveditz]
	Comment 4 • 6 years ago

mccr8 suggested sec-high for this.

	Andrew McCreight [:mccr8]
	Comment 5 • 6 years ago

Olli, any chance you can look at this next week? If not, I can get to it the week after that when I get back from PTO.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 6 years ago

I'll try to look at this next week.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 6 years ago

As far as I see, this is a regression from bug 1354599.

Baku, see the traversing/unlinking code in nsDocument.
But I'd argue MediaQueryList's ownership model has been really error prone, hmm, always.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 6 years ago

Comment on attachment 8997411 [details] [diff] [review]
mql.patch

ok, so CheckInnerWindowCorrectness() returns false when window is in bfcache, but we just don't traverse in that case. And that shouldn't matter since document itself stays alive in bfcache.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 6 years ago

Comment on attachment 8997411 [details] [diff] [review]
mql.patch

I could reproduce the crash in asan built without the patch, but couldn't with the patch.

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 6 years ago

This is a MOZ_ASSERT crash. I guess we can land this bug immediately. Right?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 6 years ago

Thinking... probably yes. But this is marked as sec-high, so as of now you need approval.

	Andrea Marchesini [:baku] Assignee
	Comment 13 • 6 years ago

Comment on attachment 8997411 [details] [diff] [review]
mql.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

this is a MOZ_ASSERT. No way to exploit it.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

no. Actually no comments at all. The commit message would be: "Fix the traverse() of MediaQueryList"

Which older supported branches are affected by this flaw?

Just m-c.

If not all supported branches, which bug introduced the flaw?

Bug 1278831

How likely is this patch to cause regressions; how much testing does it need?

None.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 6 years ago

Comment 7 said this was a regression from a much older bug. If it's really a trunk-only regression, it can land without sec-approval. But we should try to clarify that :)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 6 years ago

This is not trunk only. But I haven't see reasoning for sec-high.

	Al Billings [:abillings - ex-MoCo]
	Comment 16 • 6 years ago

Andrew made it sec-high so I'm not going to gainsay that but I'll give it sec-approval for trunk.

Since comment 13 and comment 15 say different things, I looked at bug 1278831, which is introduced as the regression cause. That only went in with a fix on August 1 so it seems weird to give that as a regressor but then say it isn't trunk only.

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 6 years ago

Because of the timing of bug 1278831 and this bug, I though the 2 issues were connected.
I'll check again if this crash can be reproduced without bug 1278831.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 18 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/c54958b74fd2

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 6 years ago

Hmm hmm, is this after all a regression from bug 1278831.
Can't reproduce without bug 1278831.


Now that I think, we'd have the extra refcnt before bug 1278831, which is traversed, but after that, we don't have.

So, sorry, I was wrong on which bug regressed this and baku was right.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 6 years ago

Thanks for double-checking. Sounds like we're all done here modulo landing a test maybe?

	Andrew McCreight [:mccr8]
	Comment 21 • 6 years ago

I suppose it doesn't need to be hidden at this point, but I think it is a sec issue. If you tell the CC that you have a reference to something, but you don't, then a live object can end up getting unlinked. These "use after unlink" issues aren't as bad as use-after-frees, but they are sort of related, and maybe one could lead to the other.

	Ciprian Georgiu, Desktop QA
	Comment 22 • 6 years ago

I have reproduced this crash using an affected asan Nightly build, with the test case from comment 1.

This is verified fixed on latest asan Beta build 63.0b15 (20181014190738) on Ubuntu 16.04 x64.