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

Found with m-c:
BuildID=20180507140941
SourceStamp=93443d36d4bd53dba004f7b73430879f96daa681

==4110==ERROR: AddressSanitizer: use-after-poison on address 0x625000332a56 at pc 0x7f7d46f3ede0 bp 0x7ffc1d8a6390 sp 0x7ffc1d8a6388
READ of size 2 at 0x625000332a56 thread T0 (file:// Content)
    #0 0x7f7d46f3eddf in IsFrameModified src/layout/generic/nsIFrame.h:4135:35
    #1 0x7f7d46f3eddf in AnyContentAncestorModified src/layout/painting/RetainedDisplayListBuilder.cpp:193
    #2 0x7f7d46f3eddf in HasModifiedFrame src/layout/painting/RetainedDisplayListBuilder.cpp:332
    #3 0x7f7d46f3eddf in MergeState::ProcessOldNode(Index<OldListUnits>, nsTArray<Index<MergedListUnits> >&&) src/layout/painting/RetainedDisplayListBuilder.cpp:362
    #4 0x7f7d46e37838 in MergeState::Finalize() src/layout/painting/RetainedDisplayListBuilder.cpp:308:7
    #5 0x7f7d46e35ea2 in RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList*, RetainedDisplayList*, RetainedDisplayList*, mozilla::Maybe<mozilla::ActiveScrolledRoot const*>&) src/layout/painting/RetainedDisplayListBuilder.cpp:490:26
    #6 0x7f7d46e3dccc in RetainedDisplayListBuilder::AttemptPartialUpdate(unsigned int, mozilla::DisplayListChecker*) src/layout/painting/RetainedDisplayListBuilder.cpp:1097:7
    #7 0x7f7d465fd93b in nsLayoutUtils::PaintFrame(gfxContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) src/layout/base/nsLayoutUtils.cpp:3679:40
    #8 0x7f7d464f0305 in mozilla::PresShell::Paint(nsView*, nsRegion const&, unsigned int) src/layout/base/PresShell.cpp:6351:5
    #9 0x7f7d45e7ea9a in nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) src/view/nsViewManager.cpp:480:19
    #10 0x7f7d45e7d89c in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) src/view/nsViewManager.cpp:412:33
    #11 0x7f7d45e82ef6 in nsViewManager::ProcessPendingUpdates() src/view/nsViewManager.cpp:1102:5
    #12 0x7f7d464679d4 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:2067:11
    #13 0x7f7d46474f60 in TickDriver src/layout/base/nsRefreshDriver.cpp:337:13
    #14 0x7f7d46474f60 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:307
    #15 0x7f7d46474b26 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:329:5
    #16 0x7f7d4647789e in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:770:5
    #17 0x7f7d4647789e in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:683
    #18 0x7f7d4647749e in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:584:9
    #19 0x7f7d46d1f0bf in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) src/layout/ipc/VsyncChild.cpp:68:16
    #20 0x7f7d3faaedc4 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:167:20
    #21 0x7f7d3f986cd3 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1988:28
    #22 0x7f7d3f4f6b6e in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2141:25
    #23 0x7f7d3f4f3b36 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2071:17
    #24 0x7f7d3f4f52ec in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1917:5
    #25 0x7f7d3f4f5948 in mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1950:15
    #26 0x7f7d3e604ec9 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1090:14
    #27 0x7f7d3e620900 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #28 0x7f7d3f4fe6c6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:125:5
    #29 0x7f7d3f452f89 in RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
    #30 0x7f7d3f452f89 in RunHandler src/ipc/chromium/src/base/message_loop.cc:319
    #31 0x7f7d3f452f89 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
    #32 0x7f7d45f0c57a in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
    #33 0x7f7d4a180f7b in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:893:22
    #34 0x7f7d3f452f89 in RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
    #35 0x7f7d3f452f89 in RunHandler src/ipc/chromium/src/base/message_loop.cc:319
    #36 0x7f7d3f452f89 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
    #37 0x7f7d4a180940 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:719:34
    #38 0x4f1875 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #39 0x4f1875 in main src/browser/app/nsBrowserApp.cpp:280
    #40 0x7f7d5dd8a82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #41 0x420f48 in _start (firefox+0x420f48)

0x625000332a56 is located 4438 bytes inside of 8192-byte region [0x625000331900,0x625000333900)
allocated by thread T0 (file:// Content) here:
    #0 0x4c1c93 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f7d3e5b3ec3 in AllocateChunk src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:193:15
    #2 0x7f7d3e5b3ec3 in InternalAllocate src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:228
    #3 0x7f7d3e5b3ec3 in Allocate src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:75
    #4 0x7f7d3e5b3ec3 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:80
    #5 0x7f7d466d0ecf in AllocateByFrameID src/layout/base/nsPresArena.h:39:12
    #6 0x7f7d466d0ecf in AllocateFrame src/obj-firefox/dist/include/nsIPresShell.h:208
    #7 0x7f7d466d0ecf in operator new src/layout/generic/ViewportFrame.cpp:34
    #8 0x7f7d466d0ecf in NS_NewViewportFrame(nsIPresShell*, mozilla::ComputedStyle*) src/layout/generic/ViewportFrame.cpp:31
    #9 0x7f7d46560eca in nsCSSFrameConstructor::ConstructRootFrame() src/layout/base/nsCSSFrameConstructor.cpp:2705:5
    #10 0x7f7d464bb554 in mozilla::PresShell::Initialize() src/layout/base/PresShell.cpp:1800:36
    #11 0x7f7d416c1172 in nsContentSink::StartLayout(bool) src/dom/base/nsContentSink.cpp:1270:26
    #12 0x7f7d405a2f36 in nsHtml5TreeOpExecutor::StartLayout(bool*) src/parser/html/nsHtml5TreeOpExecutor.cpp:674:18
    #13 0x7f7d4059e48b in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*, bool*) src/parser/html/nsHtml5TreeOperation.cpp:1199:17
    #14 0x7f7d4059b416 in nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:490:17
    #15 0x7f7d405a74cb in nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:121:18
    #16 0x7f7d3e5e60d1 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
    #17 0x7f7d3e604ec9 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1090:14
    #18 0x7f7d3e620900 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #19 0x7f7d3f4fe6da in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #20 0x7f7d3f452f89 in RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
    #21 0x7f7d3f452f89 in RunHandler src/ipc/chromium/src/base/message_loop.cc:319
    #22 0x7f7d3f452f89 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
    #23 0x7f7d45f0c57a in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
    #24 0x7f7d4a180f7b in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:893:22
    #25 0x7f7d3f452f89 in RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
    #26 0x7f7d3f452f89 in RunHandler src/ipc/chromium/src/base/message_loop.cc:319
    #27 0x7f7d3f452f89 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
    #28 0x7f7d4a180940 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:719:34
    #29 0x4f1875 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #30 0x4f1875 in main src/browser/app/nsBrowserApp.cpp:280
    #31 0x7f7d5dd8a82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291

	Tyson Smith [:tsmith] Reporter
	Comment 1 • 6 years ago

Helps with repro

	Tyson Smith [:tsmith] Reporter
	Comment 2 • 6 years ago

This also only seems to repro with GNOME_ACCESSIBILITY=1

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

Calling it sec-high out of caution. Can't tell from just this stack whether it's a simple safely frame-poisoned frame,  or if the display list is pointing at something that isn't a frame at all. Should be easy to tell in a debugger though and then we can downgrade the severity if it's not that bad.

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

Matt, have you had a chance to look at this further?

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 5 • 6 years ago

I can't get this one to reproduce unfortunately.

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

Give this a shot. I could reproduce it consistently with testcase but it did take up to a minute sometimes.

I was using m-c:
BuildID=20180529095249
SourceStamp=f01bb6245db1ea2a87e5360104a4110571265137

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 7 • 6 years ago

Perfect, thanks!

Got a good understanding of this now, will try get a patch up before bed.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 8 • 6 years ago

Basically the same as bug 1452464, just handling the table frame pointers.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 9 • 6 years ago

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Calling it sec-high out of caution. Can't tell from just this stack whether
> it's a simple safely frame-poisoned frame,  or if the display list is
> pointing at something that isn't a frame at all. Should be easy to tell in a
> debugger though and then we can downgrade the severity if it's not that bad.

It is indeed a guaranteed to be a frame pointer that's UAF, so should always be poisoned (or reused for another frame of the exact same type). I think the sec rating can be lowered based on that.

	Miko Mynttinen
	Comment 10 • 6 years ago

Comment on attachment 8982148 [details] [diff] [review]
table-frame-lifetimes

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

This pattern with multiple invalidation frames seems to be repeating in quite a few places. Maybe we could refactor this later to use SmallPointerArray instead.

	Pulsebot
	Comment 11 • 6 years ago

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a768ef5b02e
Add frame tracking for the nsDisplayTable* classes. r=miko

	Cosmin Sabou [:CosminS]
	Comment 12 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6a768ef5b02e

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

Please request Beta approval on this when you get a chance.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 14 • 6 years ago

Comment on attachment 8982148 [details] [diff] [review]
table-frame-lifetimes

Approval Request Comment
[Feature/Bug causing the regression]: Retained-dl
[User impact if declined]: Possible crashes/UAF from table mutations.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Tested with the fuzzer test, can't reproduce the issue.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Hooks up the extra nsIFrame references to the frame tracking infrastructure that we've used without issue for similar bugs in the past.
[String changes made/needed]: None.

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

Comment on attachment 8982148 [details] [diff] [review]
table-frame-lifetimes

RDL crash fix. Approved for 61.0b12.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/f14daaaaf1be