Closed Bug 785555 (CVE-2013-0771) Opened 12 years ago Closed 12 years ago

Heap-buffer-overflow in gfxTextRun::ShrinkToLigatureBoundaries

Categories

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

x86_64
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox18 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- wontfix

People

(Reporter: inferno, Assigned: MatsPalmgren_bugz)

References

Details

(5 keywords, Whiteboard: [asan][adv-main18+])

Attachments

(7 files, 1 obsolete file)

Attached file Testcase
Reproduces on trunk ================================================================= ==12169== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f45e076bf00 at pc 0x7f45ff1828e9 bp 0x7fff4b935330 sp 0x7fff4b935328 READ of size 4 at 0x7f45e076bf00 thread T0 #0 0x7f45ff1828e8 in gfxShapedWord::CompressedGlyph::IsLigatureGroupStart() const src/../../dist/include/gfxFont.h:1897 #1 0x7f460a041636 in gfxTextRun::ShrinkToLigatureBoundaries(unsigned int*, unsigned int*) src/gfx/thebes/gfxFont.cpp:4555 #2 0x7f460a04d863 in gfxTextRun::BreakAndMeasureText(unsigned int, unsigned int, bool, double, gfxTextRun::PropertyProvider*, bool, double*, gfxFont::RunMetrics*, gfxFont::BoundingBoxType, gfxContext*, bool*, unsigned int*, bool, gfxBreakPriority*) src/gfx/thebes/gfxFont.cpp:4946 #3 0x7f45ff148515 in nsTextFrame::ReflowText(nsLineLayout&, int, nsRenderingContext*, bool, nsHTMLReflowMetrics&, unsigned int&) src/layout/generic/nsTextFrameThebes.cpp:7785 #4 0x7f45fef550cd in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:829 #5 0x7f45fef25f3b in nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) src/layout/generic/nsInlineFrame.cpp:680 #6 0x7f45fef22b75 in nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:543 #7 0x7f45fef1fbdf in nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:397 #8 0x7f45fef54c93 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:822 #9 0x7f45fef25f3b in nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) src/layout/generic/nsInlineFrame.cpp:680 #10 0x7f45fef22b75 in nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:543 #11 0x7f45fef1fbdf in nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:397 #12 0x7f45fef54c93 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:822 #13 0x7f45fef25f3b in nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) src/layout/generic/nsInlineFrame.cpp:680 #14 0x7f45fef22b75 in nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:543 #15 0x7f45fef1fbdf in nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:397 #16 0x7f45fef54c93 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:822 #17 0x7f45fec12318 in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) src/layout/generic/nsBlockFrame.cpp:3835 #18 0x7f45fec0c421 in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) src/layout/generic/nsBlockFrame.cpp:3631 #19 0x7f45febff041 in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:3483 #20 0x7f45febedecb in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:2571 #21 0x7f45febd4579 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) src/layout/generic/nsBlockFrame.cpp:2021 #22 0x7f45febc7557 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1070 #23 0x7f45fec55b20 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) src/layout/generic/nsBlockReflowContext.cpp:268 #24 0x7f45febf8cfd in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:3207 #25 0x7f45febed996 in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:2515 #26 0x7f45febd4579 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) src/layout/generic/nsBlockFrame.cpp:2021 #27 0x7f45febc7557 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1070 #28 0x7f45fecb5398 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:946 #29 0x7f45fee86485 in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsCanvasFrame.cpp:463 #30 0x7f45fecb5398 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:946 #31 0x7f45fedfcec9 in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) src/layout/generic/nsGfxScrollFrame.cpp:523 #32 0x7f45fee01e9e in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) src/layout/generic/nsGfxScrollFrame.cpp:623 #33 0x7f45fee0630a in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsGfxScrollFrame.cpp:864 #34 0x7f45fecb5398 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:946 #35 0x7f45ff1deb8d in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsViewportFrame.cpp:200 #36 0x7f45fe9439fc in PresShell::DoReflow(nsIFrame*, bool) src/layout/base/nsPresShell.cpp:7430 #37 0x7f45fe97036b in PresShell::ProcessReflowCommands(bool) src/layout/base/nsPresShell.cpp:7577 #38 0x7f45fe96ebf1 in PresShell::FlushPendingNotifications(mozFlushType) src/layout/base/nsPresShell.cpp:3898 #39 0x7f45fea109f1 in nsRefreshDriver::Notify(nsITimer*) src/layout/base/nsRefreshDriver.cpp:398 #40 0x7f4609a629c6 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:476 #41 0x7f4609a641b8 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:556 #42 0x7f4609a275ae in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:624 #43 0x7f46096c89c7 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220 #44 0x7f4608605275 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82 #45 0x7f4609cd4279 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208 #46 0x7f4609cd40c2 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201 #47 0x7f4609cd3fa7 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175 #48 0x7f4607acfa0e in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163 #49 0x7f4606740528 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:273 #50 0x7f45fcf4c350 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3800 #51 0x7f45fcf525c4 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3877 #52 0x7f45fcf5568e in XRE_main src/toolkit/xre/nsAppRunner.cpp:3953 #53 0x40c5bb in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174 #54 0x409e1f in main src/browser/app/nsBrowserApp.cpp:279 #55 0x7f4619e1ec4d in ?? ??:0 0x7f45e076bf00 is located 4 bytes to the right of 124-byte region [0x7f45e076be80,0x7f45e076befc) allocated by thread T0 here: #0 0x4c3e40 in __interceptor_malloc ??:0 #1 0x7f4616cbc9d2 in moz_malloc src/memory/mozalloc/mozalloc.cpp:67 #2 0x7f460a038f43 in gfxTextRun::AllocateStorageForTextRun(unsigned long, unsigned int) src/gfx/thebes/gfxFont.cpp:4302 #3 0x7f460a01c06b in gfxTextRun::Create(gfxTextRunFactory::Parameters const*, unsigned int, gfxFontGroup*, unsigned int) src/gfx/thebes/gfxFont.cpp:4319 #4 0x7f460a01c720 in gfxFontGroup::MakeSpaceTextRun(gfxTextRunFactory::Parameters const*, unsigned int) src/gfx/thebes/gfxFont.cpp:3311 #5 0x7f460a01f534 in gfxFontGroup::MakeTextRun(unsigned char const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) src/gfx/thebes/gfxFont.cpp:3355 #6 0x7f45ff0b4128 in gfxTextRun* MakeTextRun<unsigned char>(unsigned char const*, unsigned int, gfxFontGroup*, gfxTextRunFactory::Parameters const*, unsigned int) src/layout/generic/nsTextFrameThebes.cpp:549 #7 0x7f45ff0a82fa in BuildTextRunsScanner::BuildTextRunForFrames(void*) src/layout/generic/nsTextFrameThebes.cpp:2025 #8 0x7f45ff09de6b in BuildTextRunsScanner::FlushFrames(bool, bool) src/layout/generic/nsTextFrameThebes.cpp:1388 #9 0x7f45ff0ada39 in BuildTextRunsScanner::ScanFrame(nsIFrame*) src/layout/generic/nsTextFrameThebes.cpp:1578 #10 0x7f45ff0ae326 in BuildTextRunsScanner::ScanFrame(nsIFrame*) src/layout/generic/nsTextFrameThebes.cpp:1618 #11 0x7f45ff0ae326 in BuildTextRunsScanner::ScanFrame(nsIFrame*) src/layout/generic/nsTextFrameThebes.cpp:1618 #12 0x7f45ff0ae326 in BuildTextRunsScanner::ScanFrame(nsIFrame*) src/layout/generic/nsTextFrameThebes.cpp:1618 #13 0x7f45ff0c17eb in BuildTextRuns(gfxContext*, nsTextFrame*, nsIFrame*, nsLineList_iterator const*, nsTextFrame::TextRunType) src/layout/generic/nsTextFrameThebes.cpp:1291 #14 0x7f45ff0bd322 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, gfxContext*, nsIFrame*, nsLineList_iterator const*, unsigned int*) src/layout/generic/nsTextFrameThebes.cpp:2441 #15 0x7f45ff146c51 in nsTextFrame::ReflowText(nsLineLayout&, int, nsRenderingContext*, bool, nsHTMLReflowMetrics&, unsigned int&) src/layout/generic/nsTextFrameThebes.cpp:7705 #16 0x7f45fef550cd in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:829 #17 0x7f45fef25f3b in nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) src/layout/generic/nsInlineFrame.cpp:680 #18 0x7f45fef22b75 in nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:543 #19 0x7f45fef1fbdf in nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:397 #20 0x7f45fef54c93 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:822 #21 0x7f45fef25f3b in nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) src/layout/generic/nsInlineFrame.cpp:680 #22 0x7f45fef22b75 in nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:543 #23 0x7f45fef1fbdf in nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:397 #24 0x7f45fef54c94 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:822 Shadow byte and word: 0x1fe8bc0ed7e0: fa 0x1fe8bc0ed7e0: fa fa fa fa fa fa fa fa More shadow bytes: 0x1fe8bc0ed7c0: fa fa fa fa fa fa fa fa 0x1fe8bc0ed7c8: fa fa fa fa fa fa fa fa 0x1fe8bc0ed7d0: 00 00 00 00 00 00 00 00 0x1fe8bc0ed7d8: 00 00 00 00 00 00 00 04 =>0x1fe8bc0ed7e0: fa fa fa fa fa fa fa fa 0x1fe8bc0ed7e8: fa fa fa fa fa fa fa fa 0x1fe8bc0ed7f0: 00 00 03 fb fb fb fb fb 0x1fe8bc0ed7f8: fb fb fb fb fb fb fb fb 0x1fe8bc0ed800: fa fa fa fa fa fa fa fa Stats: 225M malloced (237M for red zones) by 356222 calls Stats: 41M realloced by 20031 calls Stats: 195M freed by 233151 calls Stats: 61M really freed by 153616 calls Stats: 428M (109644 full pages) mmaped in 107 calls mmaps by size class: 8:245745; 9:40955; 10:16380; 11:14329; 12:2048; 13:1536; 14:1280; 15:256; 16:448; 17:1248; 18:144; 19:40; 20:16; mallocs by size class: 8:270186; 9:45811; 10:15863; 11:16411; 12:2410; 13:1791; 14:1427; 15:303; 16:504; 17:1289; 18:170; 19:41; 20:16; frees by size class: 8:163811; 9:36853; 10:12738; 11:13321; 12:1546; 13:1490; 14:1240; 15:259; 16:449; 17:1277; 18:115; 19:39; 20:13; rfrees by size class: 8:115976; 9:19137; 10:7708; 11:8747; 12:536; 13:469; 14:336; 15:146; 16:338; 17:189; 18:28; 19:5; 20:1; Stats: malloc large: 1516 small slow: 1815 ==12169== ABORTING
Component: General → Layout: Text
Product: Firefox → Core
This also crashes in a non-asan build. Assertions leading up to the crash: ###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file gfx/thebes/gfxSkipChars.cpp, line 60 ###!!! ASSERTION: Text run does not map enough text for our reflow: 'gfxSkipCharsIterator(iter).ConvertOriginalToSkipped(offset + length) <= mTextRun->GetLength()', file layout/generic/nsTextFrameThebes.cpp, line 7717 ###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file gfx/thebes/gfxSkipChars.cpp, line 60 ###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file gfx/thebes/gfxSkipChars.cpp, line 60 ###!!! ASSERTION: Substring out of range: 'aStart + aMaxLength <= mCharacterCount', file gfx/thebes/gfxFont.cpp, line 4911 ###!!! ASSERTION: invalid use of GetDetailedGlyphs; check the caller!: 'mDetailedGlyphs != nullptr && !mCharacterGlyphs[aCharIndex].IsSimpleGlyph() && mCharacterGlyphs[aCharIndex].GetGlyphCount() > 0', file gfxFont.h, line 2823 ###!!! ASSERTION: You can't dereference a NULL nsAutoPtr with operator->().: 'mRawPtr != 0', file ../../dist/include/nsAutoPtr.h, line 150
Severity: normal → critical
Whiteboard: [asan]
Attached file frame tree
The problem appears to be in stack frame #6, nsTextFrame::ReflowText. The frame tree looks fine to me at this point though. But offset=2 and length=1 which seems wrong. Earlier in this call we hit the block at line 7698: http://hg.mozilla.org/mozilla-central/annotate/0eebcc79ee05/layout/generic/nsTextFrameThebes.cpp#l7691 if (mTextRun && iter.GetOriginalEnd() < offset + length) iter.GetOriginalEnd() was 1 at that point. After that block, iter.GetOriginalEnd() is still 1, and offset=2, length=1 haven't changed. This seems odd to me.
Mats, want to take a stab at security rating?
Assignee: nobody → matspal
In a Linux64 debug ASAN build, I get the same error as reported. In a Linux64 debug (non-ASAN) build, I get a null-pointer crash: 192 return mHdr->mLength; (gdb) bt 12 #0 nsTArray_base<nsTArrayDefaultAllocator>::Length (this=0x8) at nsTArray.h:192 #1 0x00007ffff3049bc9 in gfxShapedWord::DetailedGlyphStore::Get (this=0x0, aOffset=2) at gfxFont.h:2198 #2 0x00007ffff3049919 in gfxTextRun::GetDetailedGlyphs (this=0x7fffdafeb300, aCharIndex=2) at gfxFont.h:2824 #3 0x00007ffff4bd4f6b in gfxTextRun::GetAdvanceForGlyphs (this=0x7fffdafeb300, aStart=2, aEnd=3) at gfx/thebes/gfxFont.cpp:4491 #4 0x00007ffff4bd712b in gfxTextRun::BreakAndMeasureText (this=0x7fffdafeb300, aStart=2, aMaxLength=1, ...) at gfx/thebes/gfxFont.cpp:5003 #5 0x00007ffff3035e8b in nsTextFrame::ReflowText I get the same null-pointer crash in a Nightly build: bp-007d738d-1028-4451-a68f-908b42120829 I haven't tried other platforms than Linux64. I'm guessing sec-low for now.
Keywords: sec-low
Attached patch wip (obsolete) — Splinter Review
This fixes the crash and assertions for me locally. I cant' say I know what the heck I'm doing though, so it's probably not the right fix.
Attachment #656652 - Flags: feedback?(roc)
Can you explain what you think the bug is?
If you look in the attached frame tree dump, the text frame has length 1, but GetTrimmableWhitespaceCount looks further than that, and comes up with offset=2 (which seems right given that the starts with "\n "). It seems the gfxSkipChars code is confused by an offset that is beyond what is mapped by the current frame. Why? I don't know. I just tried to avoid that and it seemed to work...
Why are we breaking after the \n? Is this text white-space:pre-line or pre? I don't see why it would be, looking at the testcase, but why else would we break after the \n? In any case I would have expected length to be 1 and skipLength to be 0 here, in which case GetTrimmableWhitespaceCount should return 0, not 2.
I don't think we're breaking, just trimming the insignificant "\n " at the start of the content. I think this explains the 'length': 7539 // Temporarily map all possible content while we construct our new textrun. 7540 // so that when doing reflow our styles prevail over any part of the 7541 // textrun we look at. Note that next-in-flows may be mapping the same 7542 // content; gfxTextRun construction logic will ensure that we take priority. 7543 int32_t maxContentLength = GetInFlowContentLength(); 7544 ... 7581 // DOM offsets of the text range we need to measure, after trimming 7582 // whitespace, restricting to first-letter, and restricting preformatted text 7583 // to nearest newline 7584 int32_t length = maxContentLength; 7585 int32_t offset = GetContentOffset(); before calling GetTrimmableWhitespaceCount length is 3 (==maxContentLength). textStyle->WhiteSpaceIsSignificant() is false. textStyle->NewlineIsSignificant() is false. Looking at the frame tree I would actually expect length == 21, because the last text frame continuation is: Text(1)"\n"@0x7fffe3baa758 [run=0x7fffda84d200][20,1,T] But, this is BIDI text (dir=rtl) so GetInFlowContentLength() http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#594 does NOT return "mContent->TextLength() - mContentOffset" but calculates the length of the fluid chain. The last-in-flow is the frame for "H", hence the return value 3.
(In reply to Mats Palmgren [:mats] from comment #11) > I don't think we're breaking, just trimming the insignificant "\n " > at the start of the content. Sure we're breaking. In your frame dump, the text node 0x7fffde332b00 has frame 0x7fffd80a0e18 for the first character '\n' and frame 0x7fffe3df1840 for the next character ' ', and furthermore those frames are on different lines. Which seems odd. Anyway, it looks like offset==2, length==1 is correct in this case. So why has the textrun only mapped the first character?
Ah, you mean the the incoming frame tree to ReflowText. Yes, I agree it's odd that the text frames are on separate lines there - it appears to come from some generated content for <dd> ?
Attached file <dd> frame tree
Here's the frame tree for data:text/html,<dd>x</dd>
Attached file <dd><div> frame tree
Here's the tree frame tree for data:text/html,<dd><div>x</div></dd> It looks a lot like our frame tree.
This is likely the source of the generated content: http://mxr.mozilla.org/mozilla-central/source/layout/style/quirk.css#178
That's interesting but frame 0x7fffd80a0e18 in your dump does not belong to that generated content, it's part of the text node in the testcase DOM.
Attached file frame tree evolution
It's bidi resolution that causes the text frame split. The text "\n H.*XX mhF ~0Gdv`a\n" is split by EnsureBidiContinuation before the "H" because ResolveParagraph decides it's not in the same directional run: http://hg.mozilla.org/mozilla-central/annotate/c64a9f342156/layout/base/nsBidiPresUtils.cpp#l761 Then there's a call to SplitInlineAncestors which split all the ancestors up to the block we're resolving, which creates a new line for the new sub-tree: http://hg.mozilla.org/mozilla-central/annotate/c64a9f342156/layout/generic/nsBlockFrame.cpp#l4928
Stepping through all of the bidi resolution it's basically the same story for splitting "\n " into "\n" " " then SplitInlineAncestors creates a line for that sub-tree etc.
It seems to me that SplitInlineAncestors should not be creating new lines!
It's not clear to me why the new line is being created here: SplitInlineAncestors explicitly doesn't split line frames. If I understand Mats' analysis correctly, the reparenting of child frames after splitting a *child* of a line frame then causes the new line to be created in nsBlockFrame::AddFrames. Maybe the solution is to add a flag to nsBlockFrame::AddFrames/InsertFrames to indicate that we are just moving existing frames and not adding new ones, so no new line needs to be created?
... except that doing that doesn't seem to prevent the assertions and crash :(
I now believe that this is caused by a silly mistake in my patch to bug 731594; testing a fix to that in https://tbpl.mozilla.org/?tree=Try&rev=6978e32308cf
Attached patch fixSplinter Review
I'm convinced now that this is the correct fix for this crash. We should do this even if we make other changes to avoid creating the frame tree that we do for this testcase. https://tbpl.mozilla.org/?tree=Try&rev=0f24512cd5e5
Attachment #657874 - Flags: review?(roc)
Attachment #656652 - Attachment is obsolete: true
Attachment #656652 - Flags: feedback?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > It seems to me that SplitInlineAncestors should not be creating new lines! Maybe so, but as Simon points out, it doesn't fix the crash. And even if Simon's patch avoids causing a crash for the testcase at hand, we should still fix ReflowText because it's assuming things about the frames it shouldn't do -- there's nothing *invalid* about the frame tree when we crash (from a strict frame tree point of view). The crash is *caused* by ReflowText's wrong assumptions. I suggest we do the following 1. fix ReflowText here 2. file a bug to investigate whether nsBlockFrame::AddFrames should create a new line or not 3. file a separate bug for Simon's follow-up to bug 731594
Flags: in-testsuite?
Target Milestone: --- → mozilla18
Filed bug 788086 for the followup to bug 731594
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [asan] → [asan][adv-track-main18+]
Whiteboard: [asan][adv-track-main18+] → [asan][adv-main18+]
Did this affect a shipped version of Firefox or ESR 17?
The 8/27 branch date would make it seem like Firefox 17 and ESR 17 would have been affected since this was found a couple of days before that.
Alias: CVE-2013-0771
bug 779003 and bug 793233 is on esr17 though: https://hg.mozilla.org/releases/mozilla-esr17/filelog/tip/layout/base/nsBidiPresUtils.cpp maybe those removed the root of the problem for the testcase at hand, because I can't reproduce it in my esr17 debug build. Anyway, the patch applies as is on esr17 so we should probably take it just in case.
Comment on attachment 657874 [details] [diff] [review] fix [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: theoretical sec-critical crash (the known tests doesn't seem to trigger it for me on esr17) Fix Landed on Version: Fx18 Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #657874 - Flags: approval-mozilla-esr17?
(In reply to Mats Palmgren [:mats] from comment #32) > User impact if declined: theoretical sec-critical crash That contradicts your sec-low rating from comment 6. Bugs that might have a sec-critical type exploit should never be sec-low, but they might be sec-moderate if we're pretty sure they're likely un-exploitable.
Comment on attachment 657874 [details] [diff] [review] fix Even if this ends up being bumped up to sec-moderate that doesn't put it in the ESR landing criteria range so minusing, no need to take this. Renominate if the rating changes to sec-high or above.
Attachment #657874 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17-
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: