Closed Bug 830192 Opened 7 years ago Closed 7 years ago

Out of bounds read in nsCellMap::GetRowSpanForNewCell

Categories

(Core :: Layout: Tables, defect, critical)

x86_64
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- unaffected
firefox19 - unaffected
firefox20 + fixed
firefox21 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: inferno, Assigned: roc)

References

Details

(5 keywords, Whiteboard: [asan])

Attachments

(2 files, 2 obsolete files)

Attached file 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
>
>
>
###!!! 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...
Blocks: 827577
Severity: normal → critical
Flags: in-testsuite?
Whiteboard: [asan]
> 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?
Requesting tracking for 19 and 20, since I think the patch that caused this should go in those releases...
(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.
Duplicate of this bug: 830924
Summary: Global-buffer-overflow in nsCellMap::GetRowSpanForNewCell → Out of bounds read in nsCellMap::GetRowSpanForNewCell
Assignee: nobody → roc
Attached patch fix (obsolete) — Splinter Review
Also fixes bug 830138 and bug 830299.
Attachment #704764 - Flags: review?(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.
Attachment #704764 - Flags: review?(bzbarsky) → review+
(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.
Attached patch better fix (obsolete) — Splinter Review
See changes to nsCSSFrameConstructorState's constructors.
Attachment #704768 - Flags: review?(bzbarsky)
Comment on attachment 704768 [details] [diff] [review]
better fix

Yeah, looks much better.  r=me
Attachment #704768 - Flags: review?(bzbarsky) → review+
Duplicate of this bug: 833309
Duplicate of this bug: 833604
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.
This was backed out (please post cset links in the future).
https://hg.mozilla.org/mozilla-central/rev/8762543c77fa
Blocks: 833895
Attached patch fix v3Splinter Review
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.
Attachment #704764 - Attachment is obsolete: true
Attachment #704768 - Attachment is obsolete: true
Attachment #705707 - Flags: review?(bzbarsky)
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.
Crashtests and reftests pass on Mac debug with this patch:
https://tbpl.mozilla.org/?tree=Try&rev=5c44ad2988bd
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
Attachment #705707 - Flags: review?(bzbarsky) → review+
> I have to say that the whole setup for finding containing blocks is not nice.

Indeed.  I would love for it to become better.  :(
Flags: sec-bounty?
Oh, dear me, I'm stupid. It's not a regression, it's an unexpected pass, which is as it should be! Relanding.
Duplicate of this bug: 835056
https://hg.mozilla.org/mozilla-central/rev/4856e2c22f35

\o/
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla21
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Flags: sec-bounty? → sec-bounty+
No longer blocks: 830138
Duplicate of this bug: 830138
No longer blocks: 831335
Duplicate of this bug: 831335
Duplicate of this bug: 833895
No longer blocks: 833895
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
Attachment #705707 - Flags: approval-mozilla-beta?
Attachment #705707 - Flags: approval-mozilla-aurora?
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).
Attachment #705707 - Flags: approval-mozilla-beta?
Attachment #705707 - Flags: approval-mozilla-beta+
Attachment #705707 - Flags: approval-mozilla-aurora?
Attachment #705707 - Flags: approval-mozilla-aurora+
Does that mean that bug 827577 is being un-WONTFIXed for Fx19?
(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
Attachment #705707 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
Group: core-security
You need to log in before you can comment on or make changes to this bug.