830192 - Out of bounds read in nsCellMap::GetRowSpanForNewCell

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Abhishek Arya]

Attachment: Testcase

>==26516== ERROR: AddressSanitizer: global-buffer-overflow on address 0x7ffc05bd5070 at pc 0x7ffbe9bbd2a3 bp 0x7fffd602c1b0 sp 0x7fffd602c1a8
>READ of size 8 at 0x7ffc05bd5070 thread T0
>    #0 0x7ffbe9bbd2a2 in nsCellMap::GetRowSpanForNewCell(nsTableCellFrame*, int, bool&) const src/layout/tables/nsCellMap.cpp:2082
>    #1 0x7ffbe9b8ebce in nsCellMap::AppendCell(nsTableCellMap&, nsTableCellFrame*, int, bool, int, nsIntRect&, int*) src/layout/tables/nsCellMap.cpp:1429
>    #2 0x7ffbe9b8dd8b in nsTableCellMap::AppendCell(nsTableCellFrame&, int, bool, nsIntRect&) src/layout/tables/nsCellMap.cpp:562
>    #3 0x7ffbe9c1d97c in nsTableFrame::AppendCell(nsTableCellFrame&, int) src/layout/tables/nsTableFrame.cpp:751
>    #4 0x7ffbe9ce748c in nsTableRowFrame::AppendFrames(mozilla::layout::FrameChildListID, nsFrameList&) src/layout/tables/nsTableRowFrame.cpp:184
>    #5 0x7ffbe89a08ea in nsFrameManager::AppendFrames(nsIFrame*, mozilla::layout::FrameChildListID, nsFrameList&) src/layout/base/nsFrameManager.cpp:446
>    #6 0x7ffbe85f3b75 in nsFrameConstructorState::ProcessFrameInsertions(nsAbsoluteItems&, mozilla::layout::FrameChildListID) src/layout/base/nsCSSFrameConstructor.cpp:1286
>    #7 0x7ffbe85f24e6 in nsFrameConstructorState::~nsFrameConstructorState() src/layout/base/nsCSSFrameConstructor.cpp:996
>    #8 0x7ffbe867759a in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) src/layout/base/nsCSSFrameConstructor.cpp:7401
>    #9 0x7ffbe866edf7 in nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) src/layout/base/nsCSSFrameConstructor.cpp:6849
>    #10 0x7ffbe865f1c9 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool) src/layout/base/nsCSSFrameConstructor.cpp:9420
>    #11 0x7ffbe868fce3 in nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList&, mozilla::css::OverflowChangedTracker&) src/layout/base/nsCSSFrameConstructor.cpp:8210
>    #12 0x7ffbe8696788 in nsCSSFrameConstructor::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::css::RestyleTracker&, bool, mozilla::css::OverflowChangedTracker&) src/layout/base/nsCSSFrameConstructor.cpp:8358
>    #13 0x7ffbe85d5446 in mozilla::css::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint, mozilla::css::OverflowChangedTracker&) src/layout/base/RestyleTracker.cpp:123
>    #14 0x7ffbe85d0478 in mozilla::css::RestyleTracker::DoProcessRestyles() src/layout/base/RestyleTracker.cpp:213
>    #15 0x7ffbe86c2701 in mozilla::css::RestyleTracker::ProcessRestyles() src/layout/base/RestyleTracker.h:192
>    #16 0x7ffbe86c2039 in nsCSSFrameConstructor::ProcessPendingRestyles() src/layout/base/nsCSSFrameConstructor.cpp:12156
>    #17 0x7ffbe8b2ce12 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/nsPresShell.cpp:3867
>    #18 0x7ffbe8bdbe35 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:898
>    #19 0x7ffbe8bf391b in mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:164
>    #20 0x7ffbe8bf2f32 in mozilla::RefreshDriverTimer::Tick() src/layout/base/nsRefreshDriver.cpp:156
>    #21 0x7ffbe8bf23db in mozilla::RefreshDriverTimer::TimerTick(nsITimer*, void*) src/layout/base/nsRefreshDriver.cpp:181
>    #22 0x7ffbf63cc9db in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:482
>    #23 0x7ffbf63cde64 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:565
>    #24 0x7ffbf639024f in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
>    #25 0x7ffbf6004bd5 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
>    #26 0x7ffbf361517c in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
>    #27 0x7ffbf66821d2 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
>    #28 0x7ffbf6682009 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
>    #29 0x7ffbf6681ede in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
>    #30 0x7ffbf29d07f7 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
>    #31 0x7ffbf14dc0e5 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:288
>    #32 0x7ffbe67427b4 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3823
>    #33 0x7ffbe674839a in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3890
>    #34 0x7ffbe674b170 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4093
>    #35 0x41d963 in do_main(int, char**, nsIFile*) src/browser/app/nsBrowserApp.cpp:195
>    #36 0x41ac69 in main src/browser/app/nsBrowserApp.cpp:388
>    #37 0x7ffc098f276c in
>0x7ffc05bd5070 is located 16 bytes to the right of global variable 'vtable for nsTableOuterFrame (src/layout/tables/nsTableOuterFrame.cpp)' (0x7ffc05bd4be0) of size 1152
>Shadow bytes around the buggy address:
>  0x1fff80b7a9b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1fff80b7a9c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1fff80b7a9d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1fff80b7a9e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1fff80b7a9f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>=>0x1fff80b7aa00: 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9[f9]f9
>  0x1fff80b7aa10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1fff80b7aa20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1fff80b7aa30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1fff80b7aa40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1fff80b7aa50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>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 righ 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
>  ASan internal:         fe
>Stats: 249M malloced (269M for red zones) by 401098 calls
>Stats: 47M realloced by 24048 calls
>Stats: 215M freed by 270001 calls
>Stats: 81M really freed by 188881 calls
>Stats: 472M (472M-0M) mmaped; 118 maps, 0 unmaps
>  mmaps   by size class: 8:294894; 9:32764; 10:8190; 11:14329; 12:2048; 13:1536; 14:1280; 15:384; 16:1216; 17:1312; 18:48; 19:40; 20:24;
>  mallocs by size class: 8:334868; 9:32118; 10:8856; 11:16113; 12:2497; 13:1708; 14:1597; 15:401; 16:1446; 17:1362; 18:69; 19:40; 20:23;
>  frees   by size class: 8:221635; 9:22227; 10:5199; 11:13888; 12:1473; 13:1251; 14:1432; 15:276; 16:1161; 17:1344; 18:57; 19:38; 20:20;
>  rfrees  by size class: 8:166822; 9:7743; 10:2132; 11:9241; 12:606; 13:512; 14:448; 15:160; 16:970; 17:216; 18:26; 19:4; 20:1;
>Stats: malloc large: 1494 small slow: 2344
>Stats: StackDepot: 0 ids; 0M mapped
>==26516== ABORTING
>
>
>

Comment 1

[Mats Palmgren (inactive)]

###!!! ASSERTION: unexpected child list: 'aListID == kPrincipalList', file layout/tables/nsTableRowFrame.cpp, line 175

aListID is kFixedList and we're trying to add a fixed-pos child frame
because the table-row frame has a transform and:

  5652  nsCSSFrameConstructor::GetFixedContainingBlock(nsIFrame* aFrame)
  5653  {
  5654    NS_PRECONDITION(nullptr != mRootElementFrame, "no root element frame");
  5655  
  5656    // Starting with aFrame, look for a frame that is CSS-transformed
  5657    for (nsIFrame* frame = aFrame; frame; frame = frame->GetParent()) {
  5658      if (frame->GetStyleDisplay()->HasTransform(frame)) {
  5659        return frame;
  5660      }
  5661    }
  5662  
  5663    return mFixedContainingBlock;
  5664  }

apparently made it the containingBlock for the mFixedItems.

So now all frame classes are expected to deal with fixed pos children?
This doesn't make sense to me, so I feel like I'm missing something...

Comment 2

[Boris Zbarsky [:bzbarsky]]

> So now all frame classes are expected to deal with fixed pos children?

Ugh.

Long-term, I the answer is "yes".  For now, we should probably skip here exactly the things we skip in GetAbsoluteContainingBlock, with some XXX comments pointing back and forth.  Does doing that help?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Requesting tracking for 19 and 20, since I think the patch that caused this should go in those releases...

Comment 4

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> > So now all frame classes are expected to deal with fixed pos children?
> 
> Ugh.
> 
> Long-term, I the answer is "yes".  For now, we should probably skip here
> exactly the things we skip in GetAbsoluteContainingBlock, with some XXX
> comments pointing back and forth.  Does doing that help?

Yeah, I think that is the right fix here.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Also fixes bug 830138 and bug 830299.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 704764 [details] [diff] [review]
fix

>+    // Only root frames (ViewportFrames) are true fixed-pos containers. All
>+    // non-root frames use their abs-pos list.

What about pagecontent frames or whatnot?

Also, can we deal with the null fixed containing block in the MathML case?  I'm not sure we could ever have a null fixed containing block before...

r=me modulo those issues.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> What about pagecontent frames or whatnot?

Argh, that's a good point.

> Also, can we deal with the null fixed containing block in the MathML case? 
> I'm not sure we could ever have a null fixed containing block before...

The testcase tests that. We handle it just like a null abs-pos container.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better fix

See changes to nsCSSFrameConstructorState's constructors.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 704768 [details] [diff] [review]
better fix

Yeah, looks much better.  r=me

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ee71614172

This bug is not yet present on branches so I figure it's OK to check into central without further approval.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This was backed out (please post cset links in the future).
https://hg.mozilla.org/mozilla-central/rev/8762543c77fa

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

The problem was setting mFixedPosIsAbsPos in nsFrameConstructorState's constructor:
+    mFixedPosIsAbsPos(aFixedContainingBlock &&
+        aFixedContainingBlock->GetAbsoluteListID() != nsIFrame::kFixedList),

This is incorrect when we construct frames for fixed-pos content inside an abs-pos container inside a transformed element (as in 827577-1b.html --- the case the original bug was trying to fix!). These lines were setting mFixedPosIsAbsPos to true when it should be false for that case.

I've replaced this with the simpler and in hindsight obvious check aFixedContainingBlock == aAbsoluteContainingBlock.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have to say that the whole setup for finding containing blocks is not nice. We have to duplicate logic between GetXXXContainingBlock and PushXXXContainingBlock. Setting up the initial containing block state is also unnecessary work in the (not uncommon) cases where we're not going to insert a positioned or floating element.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Crashtests and reftests pass on Mac debug with this patch:
https://tbpl.mozilla.org/?tree=Try&rev=5c44ad2988bd

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 705707 [details] [diff] [review]
fix v3

>+    // block, use the abs-pos containing block's abs-pos list.

s/list./list for fixed-pos frames./ perhaps?

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

> I have to say that the whole setup for finding containing blocks is not nice.

Indeed.  I would love for it to become better.  :(

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/342c2b349402

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sigh, backed out again: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e518197a823

The regression is in the test for bug 830299.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, dear me, I'm stupid. It's not a regression, it's an unexpected pass, which is as it should be! Relanding.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/4856e2c22f35

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4856e2c22f35

\o/

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 705707 [details] [diff] [review]
fix v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 827577
User impact if declined: crashes
Testing completed (on m-c, etc.): just landed. Some testcases in patch, dups know to be fixed.
Risk to taking this patch (and alternatives if risky): it's not a trivial patch but it's not bad. At least we have test coverage around it. The alternatives aren't appealing: broken rendering on some sites, or crashes/security bugs.
String or UUID changes made by this patch: None

Comment 33

[Alex Keybl [:akeybl]]

Comment on attachment 705707 [details] [diff] [review]
fix v3

When landed today, this will make it into FF19b4, which should give us enough time to react to regressions (if any).

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Does that mean that bug 827577 is being un-WONTFIXed for Fx19?

Comment 35

[Alex Keybl [:akeybl]]

(In reply to Ryan VanderMeulen from comment #34)
> Does that mean that bug 827577 is being un-WONTFIXed for Fx19?

Good point, forgot FF18 was unaffected and FF19 is not yet affected - setting the status flags appropriately. If we can avoid introducing an sg:crit issue in FF19 at this point (even though we've resolved it), we should maintain the status quo for one more release. https://bugzilla.mozilla.org/show_bug.cgi?id=827577#c26

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c8641ce92c72