Last Comment Bug 785555 - (CVE-2013-0771) Heap-buffer-overflow in gfxTextRun::ShrinkToLigatureBoundaries
(CVE-2013-0771)
: Heap-buffer-overflow in gfxTextRun::ShrinkToLigatureBoundaries
Status: RESOLVED FIXED
[asan][adv-main18+]
: assertion, crash, regression, sec-low, testcase
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86_64 All
: -- critical (vote)
: mozilla18
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks: 731594
  Show dependency treegraph
 
Reported: 2012-08-24 16:49 PDT by Abhishek Arya
Modified: 2014-07-18 17:57 PDT (History)
9 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
unaffected
wontfix


Attachments
Testcase (225 bytes, text/html)
2012-08-24 16:49 PDT, Abhishek Arya
no flags Details
stack for the first of the assertions (10.44 KB, text/plain)
2012-08-28 20:39 PDT, Mats Palmgren (:mats)
no flags Details
frame tree (15.49 KB, text/html)
2012-08-28 20:49 PDT, Mats Palmgren (:mats)
no flags Details
wip (1.29 KB, patch)
2012-08-29 16:10 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
<dd> frame tree (2.08 KB, text/plain)
2012-08-30 07:33 PDT, Mats Palmgren (:mats)
no flags Details
<dd><div> frame tree (3.47 KB, text/plain)
2012-08-30 07:37 PDT, Mats Palmgren (:mats)
no flags Details
frame tree evolution (40.06 KB, text/html)
2012-09-01 15:18 PDT, Mats Palmgren (:mats)
no flags Details
fix (1.37 KB, patch)
2012-09-03 09:25 PDT, Mats Palmgren (:mats)
roc: review+
lukasblakk+bugs: approval‑mozilla‑esr17-
Details | Diff | Review

Description Abhishek Arya 2012-08-24 16:49:52 PDT
Created attachment 655224 [details]
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
Comment 1 Mats Palmgren (:mats) 2012-08-28 20:13:06 PDT
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
Comment 2 Mats Palmgren (:mats) 2012-08-28 20:39:37 PDT
Created attachment 656318 [details]
stack for the first of the assertions
Comment 3 Mats Palmgren (:mats) 2012-08-28 20:49:50 PDT
Created attachment 656321 [details]
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.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-28 23:08:10 PDT
Yes, that must be wrong.
Comment 5 David Bolter [:davidb] 2012-08-29 12:13:54 PDT
Mats, want to take a stab at security rating?
Comment 6 Mats Palmgren (:mats) 2012-08-29 15:33:11 PDT
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.
Comment 7 Mats Palmgren (:mats) 2012-08-29 16:10:49 PDT
Created attachment 656652 [details] [diff] [review]
wip

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.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-29 16:58:37 PDT
Can you explain what you think the bug is?
Comment 9 Mats Palmgren (:mats) 2012-08-29 17:13:34 PDT
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...
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-29 18:39:07 PDT
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.
Comment 11 Mats Palmgren (:mats) 2012-08-29 19:29:37 PDT
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.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-29 20:21:47 PDT
(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?
Comment 13 Mats Palmgren (:mats) 2012-08-30 07:32:15 PDT
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> ?
Comment 14 Mats Palmgren (:mats) 2012-08-30 07:33:46 PDT
Created attachment 656863 [details]
<dd> frame tree

Here's the frame tree for data:text/html,<dd>x</dd>
Comment 15 Mats Palmgren (:mats) 2012-08-30 07:37:52 PDT
Created attachment 656866 [details]
<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.
Comment 16 Mats Palmgren (:mats) 2012-08-30 07:53:34 PDT
This is likely the source of the generated content:
http://mxr.mozilla.org/mozilla-central/source/layout/style/quirk.css#178
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-30 17:53:47 PDT
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.
Comment 18 Mats Palmgren (:mats) 2012-09-01 15:18:53 PDT
Created attachment 657606 [details]
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
Comment 19 Mats Palmgren (:mats) 2012-09-01 16:02:17 PDT
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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-02 20:02:38 PDT
It seems to me that SplitInlineAncestors should not be creating new lines!
Comment 21 Simon Montagu :smontagu 2012-09-02 22:36:34 PDT
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?
Comment 22 Simon Montagu :smontagu 2012-09-03 00:03:32 PDT
... except that doing that doesn't seem to prevent the assertions and crash :(
Comment 23 Simon Montagu :smontagu 2012-09-03 08:33:48 PDT
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
Comment 24 Mats Palmgren (:mats) 2012-09-03 09:25:19 PDT
Created attachment 657874 [details] [diff] [review]
fix

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
Comment 25 Mats Palmgren (:mats) 2012-09-03 09:26:44 PDT
(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
Comment 26 Mats Palmgren (:mats) 2012-09-03 17:30:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/933e1cf85511

Filed bug 788041 for investigating nsBlockFrame::AddFrames.
Comment 27 Simon Montagu :smontagu 2012-09-03 22:08:32 PDT
Filed bug 788086 for the followup to bug 731594
Comment 28 Ed Morley [:emorley] 2012-09-04 03:11:27 PDT
https://hg.mozilla.org/mozilla-central/rev/933e1cf85511
Comment 29 Al Billings [:abillings] 2013-01-03 15:04:00 PST
Did this affect a shipped version of Firefox or ESR 17?
Comment 30 Al Billings [:abillings] 2013-01-03 17:22:15 PST
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.
Comment 31 Mats Palmgren (:mats) 2013-01-05 19:08:34 PST
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 32 Mats Palmgren (:mats) 2013-01-05 19:20:44 PST
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.
Comment 33 Daniel Veditz [:dveditz] 2013-01-10 16:38:01 PST
(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 34 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-23 13:48:49 PST
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.
Comment 35 Mats Palmgren (:mats) 2014-07-18 10:07:46 PDT
Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cebd47634810
Comment 36 Wes Kocher (:KWierso) 2014-07-18 17:57:12 PDT
https://hg.mozilla.org/mozilla-central/rev/cebd47634810

Note You need to log in before you can comment on or make changes to this bug.