1141919 - Heap-use-after-free in UnhookTextRunFromFrames

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Abhishek Arya]

Attachment: Testcase

Let the repro run for 30 secs and in a few refreshes, you will see a crash.

==7620==ERROR: AddressSanitizer: heap-use-after-free on address 0x6250002e5f38 at pc 0x7f9fdd6d4490 bp 0x7fff7c02cc00 sp 0x7fff7c02cbf8
READ of size 8 at 0x6250002e5f38 thread T0 (Web Content)
    #0 0x7f9fdd6d448f in GetTextRun /build/firefox/src/layout/generic/nsTextFrame.h:471:14
    #1 0x7f9fdd6d448f in UnhookTextRunFromFrames(gfxTextRun*, nsTextFrame*) /build/firefox/src/layout/generic/nsTextFrame.cpp:474
    #2 0x7f9fdd718d66 in FrameTextRunCache::NotifyExpired(gfxTextRun*) /build/firefox/src/layout/generic/nsTextFrame.cpp:567:5
    #3 0x7f9fdd71d4a9 in AgeOneGeneration /build/firefox/src/objdir-ff-asan/layout/generic/../../dist/include/nsExpirationTracker.h:204:7
    #4 0x7f9fdd71d4a9 in nsExpirationTracker<gfxTextRun, 3u>::TimerCallback(nsITimer*, void*) /build/firefox/src/objdir-ff-asan/layout/generic/../../dist/include/nsExpirationTracker.h:343
    #5 0x7f9fd787a65f in nsTimerImpl::Fire() /build/firefox/src/xpcom/threads/nsTimerImpl.cpp:631:7
    #6 0x7f9fd787b1f6 in nsTimerEvent::Run() /build/firefox/src/xpcom/threads/nsTimerImpl.cpp:724:3
    #7 0x7f9fd78708d6 in nsThread::ProcessNextEvent(bool, bool*) /build/firefox/src/xpcom/threads/nsThread.cpp:855:7
    #8 0x7f9fd78ca5b0 in NS_ProcessNextEvent(nsIThread*, bool) /build/firefox/src/xpcom/glue/nsThreadUtils.cpp:265:10
    #9 0x7f9fd81518df in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /build/firefox/src/ipc/glue/MessagePump.cpp:140:5
    #10 0x7f9fd80fe2a1 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:233:3
    #11 0x7f9fd80fe2a1 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:226
    #12 0x7f9fd80fe2a1 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:200
    #13 0x7f9fdcb6ba4f in nsBaseAppShell::Run() /build/firefox/src/widget/nsBaseAppShell.cpp:164:3
    #14 0x7f9fde77c6d3 in XRE_RunAppShell /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:743:12
    #15 0x7f9fd80fe2a1 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:233:3
    #16 0x7f9fd80fe2a1 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:226
    #17 0x7f9fd80fe2a1 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:200
    #18 0x7f9fde77bb0c in XRE_InitChildProcess /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:580:7
    #19 0x4dc17e in content_process_main(int, char**) /build/firefox/src/ipc/app/../contentproc/plugin-container.cpp:211:19
    #20 0x7f9fd4deaec4 in __libc_start_main
    #21 0x41c438 in _start

0x6250002e5f38 is located 7736 bytes inside of 8192-byte region [0x6250002e4100,0x6250002e6100)
freed by thread T0 (Web Content) here:
    #0 0x4b4b00 in __interceptor_free _asan_rtl_
    #1 0x7f9fe46ef660 in FreeArenaList /build/firefox/src/nsprpub/lib/ds/plarena.c:277:13
    #2 0x7f9fdd1ab55f in nsPresArena::~nsPresArena() /build/firefox/src/layout/base/nsPresArena.cpp:42:3
    #3 0x7f9fdd42258d in PresShell::~PresShell() /build/firefox/src/layout/base/nsPresShell.cpp:811:1
    #4 0x7f9fdd41f021 in PresShell::Release() /build/firefox/src/layout/base/nsPresShell.cpp:805:1
    #5 0x7f9fdd38bb6b in operator= /build/firefox/src/objdir-ff-asan/layout/base/../../dist/include/nsCOMPtr.h:546:5
    #6 0x7f9fdd38bb6b in nsDocumentViewer::DestroyPresShell() /build/firefox/src/layout/base/nsDocumentViewer.cpp:4430
    #7 0x7f9fdd37b1ac in nsDocumentViewer::Destroy() /build/firefox/src/layout/base/nsDocumentViewer.cpp:1673:5
    #8 0x7f9fdd38d96c in nsDocumentViewer::Show() /build/firefox/src/layout/base/nsDocumentViewer.cpp:1991:5
    #9 0x7f9fdd415ed9 in nsPresContext::EnsureVisible() /build/firefox/src/layout/base/nsPresContext.cpp:2005:27
    #10 0x7f9fdd441e81 in PresShell::UnsuppressAndInvalidate() /build/firefox/src/layout/base/nsPresShell.cpp:3978:40
    #11 0x7f9fdd383f12 in nsDocumentViewer::LoadComplete(nsresult) /build/firefox/src/layout/base/nsDocumentViewer.cpp:1032:7
    #12 0x7f9fddfbf664 in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /build/firefox/src/docshell/base/nsDocShell.cpp:7507:5
    #13 0x7f9fddfbbb47 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /build/firefox/src/docshell/base/nsDocShell.cpp:7324:7
    #14 0x7f9fddfc298f in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /build/firefox/src/docshell/base/nsDocShell.cpp:7221:13
    #15 0x7f9fd8f38d34 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /build/firefox/src/uriloader/base/nsDocLoader.cpp:1263:3
    #16 0x7f9fd8f38052 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /build/firefox/src/uriloader/base/nsDocLoader.cpp:844:5
    #17 0x7f9fd8f352cd in nsDocLoader::DocLoaderIsEmpty(bool) /build/firefox/src/uriloader/base/nsDocLoader.cpp:734:9
    #18 0x7f9fd8f372a6 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /build/firefox/src/uriloader/base/nsDocLoader.cpp:618:5
    #19 0x7f9fd8f37c9c in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /build/firefox/src/uriloader/base/nsDocLoader.cpp:479:14
    #20 0x7f9fd7a12a72 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /build/firefox/src/netwerk/base/nsLoadGroup.cpp:663:18
    #21 0x7f9fd9b29485 in nsDocument::DoUnblockOnload() /build/firefox/src/dom/base/nsDocument.cpp:9120:7
    #22 0x7f9fd9b28e73 in nsDocument::UnblockOnload(bool) /build/firefox/src/dom/base/nsDocument.cpp:9048:9
    #23 0x7f9fd9afba89 in nsDocument::DispatchContentLoadedEvents() /build/firefox/src/dom/base/nsDocument.cpp:5205:3
    #24 0x7f9fd9b712f0 in apply<nsDocument, void (nsDocument::*)()> /build/firefox/src/objdir-ff-asan/dom/base/../../dist/include/nsThreadUtils.h:573:5
    #25 0x7f9fd9b712f0 in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() /build/firefox/src/objdir-ff-asan/dom/base/../../dist/include/nsThreadUtils.h:665
    #26 0x7f9fd78708d6 in nsThread::ProcessNextEvent(bool, bool*) /build/firefox/src/xpcom/threads/nsThread.cpp:855:7
    #27 0x7f9fd78ca5b0 in NS_ProcessNextEvent(nsIThread*, bool) /build/firefox/src/xpcom/glue/nsThreadUtils.cpp:265:10
    #28 0x7f9fd81518fe in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /build/firefox/src/ipc/glue/MessagePump.cpp:99:21
    #29 0x7f9fd80fe2a1 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:233:3
    #30 0x7f9fd80fe2a1 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:226
    #31 0x7f9fd80fe2a1 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:200
    #32 0x7f9fdcb6ba4f in nsBaseAppShell::Run() /build/firefox/src/widget/nsBaseAppShell.cpp:164:3
    #33 0x7f9fde77c6d3 in XRE_RunAppShell /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:743:12

previously allocated by thread T0 (Web Content) here:
    #0 0x4b4e18 in __interceptor_malloc _asan_rtl_
    #1 0x7f9fe46eeed8 in PL_ArenaAllocate /build/firefox/src/nsprpub/lib/ds/plarena.c:203:27
    #2 0x7f9fdd1aba31 in nsPresArena::Allocate(unsigned int, unsigned long) /build/firefox/src/layout/base/nsPresArena.cpp:99:3
    #3 0x7f9fdd15a609 in AllocateByObjectID /build/firefox/src/layout/style/../base/nsPresArena.h:93:12
    #4 0x7f9fdd15a609 in AllocateByObjectID /build/firefox/src/layout/style/../base/nsIPresShell.h:254
    #5 0x7f9fdd15a609 in operator new /build/firefox/src/layout/style/nsStyleContext.cpp:1024
    #6 0x7f9fdd15a609 in NS_NewStyleContext(nsStyleContext*, nsIAtom*, nsCSSPseudoElements::Type, nsRuleNode*, bool) /build/firefox/src/layout/style/nsStyleContext.cpp:1051
    #7 0x7f9fdd1637c3 in nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, nsCSSPseudoElements::Type, mozilla::dom::Element*, unsigned int) /build/firefox/src/layout/style/nsStyleSet.cpp:856:14
    #8 0x7f9fdd16d543 in nsStyleSet::ResolveAnonymousBoxStyle(nsIAtom*, nsStyleContext*) /build/firefox/src/layout/style/nsStyleSet.cpp:1872:10
    #9 0x7f9fdd28cddd in nsCSSFrameConstructor::BeginBuildingScrollFrame(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, nsIAtom*, bool, nsContainerFrame*&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:4433:5
    #10 0x7f9fdd2898cb in nsCSSFrameConstructor::SetUpDocElementContainingBlock(nsIContent*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:2872:25
    #11 0x7f9fdd286472 in nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:2411:3
    #12 0x7f9fdd2a7c40 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:7481:7
    #13 0x7f9fdd429126 in PresShell::Initialize(int, int) /build/firefox/src/layout/base/nsPresShell.cpp:1911:7
    #14 0x7f9fd9a5a08b in nsContentSink::StartLayout(bool) /build/firefox/src/dom/base/nsContentSink.cpp:1196:19
    #15 0x7f9fd91097d3 in nsHtml5TreeOpExecutor::StartLayout() /build/firefox/src/parser/html/nsHtml5TreeOpExecutor.cpp:611:3
    #16 0x7f9fd9116fb5 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) /build/firefox/src/parser/html/nsHtml5TreeOperation.cpp:823:7
    #17 0x7f9fd9107117 in nsHtml5TreeOpExecutor::RunFlushLoop() /build/firefox/src/parser/html/nsHtml5TreeOpExecutor.cpp:448:21
    #18 0x7f9fd910c30b in nsHtml5ExecutorFlusher::Run() /build/firefox/src/parser/html/nsHtml5StreamParser.cpp:127:9
    #19 0x7f9fd78708d6 in nsThread::ProcessNextEvent(bool, bool*) /build/firefox/src/xpcom/threads/nsThread.cpp:855:7
    #20 0x7f9fd78ca5b0 in NS_ProcessNextEvent(nsIThread*, bool) /build/firefox/src/xpcom/glue/nsThreadUtils.cpp:265:10
    #21 0x7f9fd81518df in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /build/firefox/src/ipc/glue/MessagePump.cpp:140:5
    #22 0x7f9fd80fe2a1 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:233:3
    #23 0x7f9fd80fe2a1 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:226
    #24 0x7f9fd80fe2a1 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:200
    #25 0x7f9fdcb6ba4f in nsBaseAppShell::Run() /build/firefox/src/widget/nsBaseAppShell.cpp:164:3
    #26 0x7f9fde77c6d3 in XRE_RunAppShell /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:743:12
    #27 0x7f9fd80fe2a1 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:233:3
    #28 0x7f9fd80fe2a1 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:226
    #29 0x7f9fd80fe2a1 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:200
    #30 0x7f9fde77bb0c in XRE_InitChildProcess /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:580:7
    #31 0x4dc17e in content_process_main(int, char**) /build/firefox/src/ipc/app/../contentproc/plugin-container.cpp:211:19
    #32 0x7f9fd4deaec4 in __libc_start_main

Shadow bytes around the buggy address:
  0x0c4a80054b90: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80054ba0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80054bb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80054bc0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80054bd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c4a80054be0: fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd
  0x0c4a80054bf0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80054c00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80054c10: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80054c20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80054c30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==7620==ABORTING

Comment 1

[Mats Palmgren (inactive)]

Hmm, this looks pretty bad.  The nsTextFrame (that the textrun has in its user data)
points into a pres shell that's been deallocated.  Frame poisoning doesn't help here
since the whole arena has been freed.

Comment 2

[Mats Palmgren (inactive)]

Attachment: another stack

Abhishek's stack still has the correct allocate/free calls so the
memory hasn't been reallocated yet.  I got a stack where the
memory has been re-allocated by unrelated code and then freed.
(A slab of JS memory by the looks of it.)

Comment 3

[Mats Palmgren (inactive)]

I see that the testcase has 'style.display = "ruby-base"' in it, so I re-ran it with
layout.css.ruby.enabled set to false in about:config (and restated).  That seems to
make the crash go away (so far).

Abhishek, can you confirm it doesn't occur with ruby disabled?

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

[Tracking Requested - why for this release]:
This bug affects Firefox 38. It seems this bug is only triggered when css ruby is enabled. Since css ruby is enabled by default since Firefox 38, it shouldn't affects version earlier than that.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 38 and 39 since this is sec-critical.

Comment 6

[Abhishek Arya]

(In reply to Mats Palmgren (:mats) from comment #3)
> I see that the testcase has 'style.display = "ruby-base"' in it, so I re-ran
> it with
> layout.css.ruby.enabled set to false in about:config (and restated).  That
> seems to
> make the crash go away (so far).
> 
> Abhishek, can you confirm it doesn't occur with ruby disabled?

Confirmed. This bug does not trigger when layout.css.ruby.enabled is set to false in about:config (and restarted).

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch for aurora

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch for central

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

A test build of aurora could be downloaded from
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-90668cc072cc/try-linux64-asan-debug/

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mats Palmgren (:mats) from comment #2)
> Created attachment 8576206 [details]
> another stack
> 
> Abhishek's stack still has the correct allocate/free calls so the
> memory hasn't been reallocated yet.  I got a stack where the
> memory has been re-allocated by unrelated code and then freed.
> (A slab of JS memory by the looks of it.)

The actual leak happens roughly 30s before the crash, hence the arean must have been reallocated many times. Changing FrameTextRunCache::TIMEOUT_SECONDS in nsTextFrame.cpp to a lower number like 1 could make the crash happen significantly sooner.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: slightly simplified testcase with analysis

This is a slightly simplified testcase with some analysis in it. Might be helpful.

Comment 12

[Mats Palmgren (inactive)]

Xidorn, can you explain why we have a textrun in that cache that points to
destroyed frame?  What are the steps that leads up to that illegal state?

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

I can explain part of it.

First of all, in the current code, a single ruby base with no other ruby element outside would not successfully suppress all line break in it, but the reflow code in nsRubyBaseContainerFrame assumes that no line break happens inside, and ignores any frames after the break. Hence, the text frame is left dirty.

Then, it seems unreflowed text frames may be assigned text run, but they may not be released properly, i.e. DestroyFrame*() and destructor are not called, which makes the text run assigned to them not be unhooked before they are freed.

It is still unknown why text frames which is not reflowed would not be released properly, which dbaron thought should also be fixed besides fixing the line break suppression.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

It is probably because that frames remains in the overflow list after reflow is not correctly recognized by the frame contructor, which causes masses on the frame list. I've observed some inconsistency of frame list in that case, or more particular, FirstChild() != LastChild() but FirstChild()->GetNextSibling() == nullptr.

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - drain overflow list when line break suppression fails

Comment 16

[Mats Palmgren (inactive)]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #13)
> Then, it seems unreflowed text frames may be assigned text run, but they may
> not be released properly, 

Whether a frame has been reflowed shouldn't matter.  If we have code that
depends on Reflow being called before Destroy then that's clearly a bug.

> i.e. DestroyFrame*() and destructor are not
> called, which makes the text run assigned to them not be unhooked before
> they are freed.

Yes, we need to find out *exactly* how that happens.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #14)
> It is probably because that frames remains in the overflow list after reflow
> is not correctly recognized by the frame contructor,

The frame constructor isn't directly involved in the frame destruction
process.  It initiates it on DOM mutations, style changes etc. but it's
not involved in cleaning up frame child lists and such.  So which code
are you talking about here?

> I've observed some inconsistency of frame list in that case,
> or more particular, FirstChild() != LastChild() but
> FirstChild()->GetNextSibling() == nullptr.

Can you be more specific about which code you're talking about here?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does either patch fix the crash on its own?

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-7) from comment #17)
> Does either patch fix the crash on its own?

Yes.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So regarding the first pair of patches:  Suppose that you have only a ruby-text-container in the markup and everything else is anonymous?  What makes the descendants of the anonymous ruby-text frame have this bit set?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And regarding the second patch:  is ruby-base-container really the only frame type that needs this fix?  Or do other ruby frame types need it as well?

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-7) from comment #19)
> So regarding the first pair of patches:  Suppose that you have only a
> ruby-text-container in the markup and everything else is anonymous?  What
> makes the descendants of the anonymous ruby-text frame have this bit set?

The rt frame will have that bit set because its parent has a ruby display type. It is same for rbc that its rb children, whether it is anonymous or not, will have that bit set because it's a child of rbc.

(In reply to David Baron [:dbaron] (UTC-7) from comment #20)
> And regarding the second patch:  is ruby-base-container really the only
> frame type that needs this fix?  Or do other ruby frame types need it as
> well?

Only single rb and rt are affected here. When they are alone, that bit is not set to themselves because they have no non-anonymous parent. Actually only rb is affected here because we have an additional line break suppression mechanism for annotation line.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #21)
> (In reply to David Baron [:dbaron] (UTC-7) from comment #19)
> > So regarding the first pair of patches:  Suppose that you have only a
> > ruby-text-container in the markup and everything else is anonymous?  What
> > makes the descendants of the anonymous ruby-text frame have this bit set?
> 
> The rt frame will have that bit set because its parent has a ruby display
> type. It is same for rbc that its rb children, whether it is anonymous or
> not, will have that bit set because it's a child of rbc.

Sure, the anonymous rt will have it set.  But what makes the children of the anonymous rt have the bit set; their style context parent will be the rtc's style context.

(I guess the patch for aurora is fine, but I'm worried about the patch for central.)

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mats Palmgren (:mats) from comment #16)
> 
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #14)
> > It is probably because that frames remains in the overflow list after reflow
> > is not correctly recognized by the frame contructor,
> 
> The frame constructor isn't directly involved in the frame destruction
> process.  It initiates it on DOM mutations, style changes etc. but it's
> not involved in cleaning up frame child lists and such.  So which code
> are you talking about here?
> 
> > I've observed some inconsistency of frame list in that case,
> > or more particular, FirstChild() != LastChild() but
> > FirstChild()->GetNextSibling() == nullptr.
> 
> Can you be more specific about which code you're talking about here?

The reason is currently if the line break suppression fails, some frames will be left in the overflow frame list of the ruby base frame after the reflow. This part of frames are not known to the frame constructor.

To illustrate the situation, I'll show what happens to my simplified test case:

When the display of outer div changes to rb and reflow:
principle.first -> text <- principal.last
overflow.first -> block -> text -> list-item -> text <- overflow.last

When the display of the li changes, and reframe happens, the lists of rb:
principle.first -> text
overflow.first -> block -> text -> block -> text <- principal.last
                            ^ overflow.last

Then reflow. I don't know exactly what will happen then. But any problem happens because of this broken list is not surprising.

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-7) from comment #22)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #21)
> > (In reply to David Baron [:dbaron] (UTC-7) from comment #19)
> > > So regarding the first pair of patches:  Suppose that you have only a
> > > ruby-text-container in the markup and everything else is anonymous?  What
> > > makes the descendants of the anonymous ruby-text frame have this bit set?
> > 
> > The rt frame will have that bit set because its parent has a ruby display
> > type. It is same for rbc that its rb children, whether it is anonymous or
> > not, will have that bit set because it's a child of rbc.
> 
> Sure, the anonymous rt will have it set.  But what makes the children of the
> anonymous rt have the bit set; their style context parent will be the rtc's
> style context.
> 
> (I guess the patch for aurora is fine, but I'm worried about the patch for
> central.)

I guess you misread the code in central. The lines:

  if ((aContainerDisplay->IsRubyDisplayType() &&
       aStyleDisplay->mDisplay != NS_STYLE_DISPLAY_RUBY_BASE_CONTAINER &&
       aStyleDisplay->mDisplay != NS_STYLE_DISPLAY_RUBY_TEXT_CONTAINER) ||

check whether the parent context has a ruby display type and the display type of the *current* context is not the containers.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8577040 [details] [diff] [review]
patch 2 - drain overflow list when line break suppression fails

This is ok, but it still seems like it shouldn't crash if we don't do this.

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-7) from comment #25)
> Comment on attachment 8577040 [details] [diff] [review]
> patch 2 - drain overflow list when line break suppression fails
> 
> This is ok, but it still seems like it shouldn't crash if we don't do this.

I don't think there is any way to do so without harm the performance. See the situation in comment 23, the frame constructor is just unaware of overflow frame list, and I don't think it should be aware of that, either.

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8576349 [details] [diff] [review]
patch for aurora

Approval Request Comment
[Feature/regressing bug #]: css ruby
[User impact if declined]: this security bug
[Describe test coverage new/current, TreeHerder]: probably a crashtest will land in central later
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8577040 [details] [diff] [review]
patch 2 - drain overflow list when line break suppression fails

Approval Request Comment
[Feature/regressing bug #]: css ruby
[User impact if declined]: reduce the risk that similar security bug happens again
[Describe test coverage new/current, TreeHerder]: no
[Risks and why]: none, the added code shouldn't be reached in normal cases at all
[String/UUID change made/needed]: none

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #26)
> (In reply to David Baron [:dbaron] (UTC-7) from comment #25)
> > Comment on attachment 8577040 [details] [diff] [review]
> > patch 2 - drain overflow list when line break suppression fails
> > 
> > This is ok, but it still seems like it shouldn't crash if we don't do this.
> 
> I don't think there is any way to do so without harm the performance. See
> the situation in comment 23, the frame constructor is just unaware of
> overflow frame list, and I don't think it should be aware of that, either.

Why do you think this has anything to do with the frame constructor?

I don't think you can say it's going to be slow when you don't understand what the problem is.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, nsContainerFrame::GetChildList does expose the overflow list through the standard frame tree APIs, so anything that's walking the frame tree should know about it.

Comment 31

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-7) from comment #30)
> Also, nsContainerFrame::GetChildList does expose the overflow list through
> the standard frame tree APIs, so anything that's walking the frame tree
> should know about it.

Yes, but if the frame constructor is going to be aware of this, it would have to check which list a frame is in, which is very slow because the list is a linked list, not a hash table. It would have to really walk through all lists if we do so, but it currently can just simply change some pointers.

Comment 32

[Xidorn Quan [:xidorn] UTC+11]

And AFAICS, the frame constructor currently gets the frame pointer from the content node, instead of walking through the frame tree, when reframe happens.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #31)
> (In reply to David Baron [:dbaron] (UTC-7) from comment #30)
> > Also, nsContainerFrame::GetChildList does expose the overflow list through
> > the standard frame tree APIs, so anything that's walking the frame tree
> > should know about it.
> 
> Yes, but if the frame constructor is going to be aware of this, it would
> have to check which list a frame is in, which is very slow because the list
> is a linked list, not a hash table. It would have to really walk through all
> lists if we do so, but it currently can just simply change some pointers.

Why does the frame constructor need to be aware of this?

Comment 34

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-7) from comment #33)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #31)
> > (In reply to David Baron [:dbaron] (UTC-7) from comment #30)
> > > Also, nsContainerFrame::GetChildList does expose the overflow list through
> > > the standard frame tree APIs, so anything that's walking the frame tree
> > > should know about it.
> > 
> > Yes, but if the frame constructor is going to be aware of this, it would
> > have to check which list a frame is in, which is very slow because the list
> > is a linked list, not a hash table. It would have to really walk through all
> > lists if we do so, but it currently can just simply change some pointers.
> 
> Why does the frame constructor need to be aware of this?

Because in this case, it is the frame constructor breaks the frame list when some frames are left in overflow list after reflow.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What do you mean by "breaks the frame list", where is the code that does that, and why does it cause a crash?

(Do you mean "break" as in line-break or "break" as in broken?)

Comment 36

[Xidorn Quan [:xidorn] UTC+11]

It happens here:
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7916

The line:
> InsertFrames(insertion.mParentFrame, kPrincipalList, prevSibling, frameItems);
tries to insert the newly constructed frames into the principal list while the prevSibling actually referes to a frame in the overflow list.

Comment 37

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/672dba5b1f9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/484e881a1d26

Comment 38

[Mats Palmgren (inactive)]

You shouldn't have landed that without sec-approval.
Only "recent regression on mozilla-central" bugs are exempted from that:
https://wiki.mozilla.org/Security/Bug_Approval_Process

And in this case the real security issue is comment 36, which I'm pretty
sure affects all branches.  (I filed bug 1143299 on the remaining issue
because we need separate tracking for it.)

Since you've done mistakes like this a couple of times before, I kindly
ask you to please be more careful when dealing with security bugs.
If you're unsure about the rules or how to handle a situation, please
just ask for advice.  Thanks.

Comment 39

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mats Palmgren (:mats) from comment #38)
> You shouldn't have landed that without sec-approval.
> Only "recent regression on mozilla-central" bugs are exempted from that:
> https://wiki.mozilla.org/Security/Bug_Approval_Process
> 
> And in this case the real security issue is comment 36, which I'm pretty
> sure affects all branches.  (I filed bug 1143299 on the remaining issue
> because we need separate tracking for it.)
> 
> Since you've done mistakes like this a couple of times before, I kindly
> ask you to please be more careful when dealing with security bugs.
> If you're unsure about the rules or how to handle a situation, please
> just ask for advice.  Thanks.

I'm sorry about that. But I really don't find I've ever made a similar mistake before. I remember that you educated me about the rule to open bug and land testcase patch, but I'm not aware of any warning about landing fixing patches before.

I'll be more careful next time. Thanks for informing.

Comment 40

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/672dba5b1f9f
https://hg.mozilla.org/mozilla-central/rev/484e881a1d26

Comment 41

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8576349 [details] [diff] [review]
patch for aurora

Given bug 1143558 shows another way to bypass the line break suppression, it probably makes less sense to uplift this patch. For fixing this security bug, the second patch is enough.

Comment 42

[Xidorn Quan [:xidorn] UTC+11]

Given I've wrongly committed the patches to m-c (sorry about that again), I guess we don't need a sec-approval, but should land them on aurora as soon as possible?

Comment 43

[Ryan VanderMeulen [:RyanVM]]

sec-approval is still useful after the fact for tracking purposes.

https://hg.mozilla.org/releases/mozilla-aurora/rev/c36dcffd0b40
https://hg.mozilla.org/releases/mozilla-aurora/rev/16fb0cf8266a

Comment 44

[Al Billings [:abillings - ex-MoCo]]

Dan, bug 1143299 is fixed in 38. This bug says to embargo this issue until bug 1143299 is "unhidden." Is that correct? I would think we can disclose it at the same time as bug 1143299.

Comment 45

[Daniel Veditz [:dveditz]]

I guess Mats's wording was better. You can reveal this bug to the extent we're ready to reveal bug 1143299.