821479 - Out-of-bounds read crash in PropertyProvider::GetSpacingInternal

Status: RESOLVED FIXED

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

Description

[Matt Wobensmith [:mwobensmith][:matt:]]

Nightly, 2012-12-12, found using bug files from related bug #765621.

I'm guessing on the title, based on the crash report. Please edit the title for accuracy if I have gotten it wrong.

A similar bug exists in #807891. 


=================================================================
==1661== ERROR: AddressSanitizer heap-buffer-overflow on address 0x0001294ddc63 at pc 0x105ecb04c bp 0x7fff5fbf4990 sp 0x7fff5fbf4988
READ of size 1 at 0x0001294ddc63 thread T0
    #0 0x105ecb04b in PropertyProvider::GetSpacingInternal nsTextFragment.h:162
    #1 0x105ecb9d3 in PropertyProvider::CalcTabWidths nsTextFrameThebes.cpp:3046
    #2 0x105ec9d1b in PropertyProvider::GetSpacingInternal nsTextFrameThebes.cpp:2936
    #3 0x109da4dd6 in GetAdjustedSpacing gfxFont.cpp:4713
    #4 0x109da4bf9 in gfxTextRun::GetAdjustedSpacingArray gfxFont.cpp:4744
    #5 0x109da7126 in gfxTextRun::AccumulateMetricsForRun gfxFont.cpp:5012
    #6 0x109da6db7 in gfxTextRun::MeasureText gfxFont.cpp:5083
    #7 0x105ef8d24 in nsTextFrame::ComputeTightBounds const nsTextFrameThebes.cpp:7289
    #8 0x105d4d488 in nsFrame::ComputeSimpleTightBounds const nsFrame.cpp:4132
    #9 0x105ca45d3 in nsBlockFrame::ComputeTightBounds const nsBlockFrame.cpp:831
    #10 0x106372ee7 in nsMathMLContainerFrame::ReflowChild nsMathMLContainerFrame.cpp:895
    #11 0x106373772 in nsMathMLContainerFrame::Reflow nsMathMLContainerFrame.cpp:926
    #12 0x105e3707a in nsLineLayout::ReflowFrame nsLineLayout.cpp:840
    #13 0x105cc84c8 in nsBlockFrame::ReflowInlineFrame nsBlockFrame.cpp:3723
    #14 0x105cc6262 in nsBlockFrame::DoReflowInlineFrames nsBlockFrame.cpp:3520
    #15 0x105cc15a9 in nsBlockFrame::ReflowInlineFrames nsBlockFrame.cpp:3374
    #16 0x105caf54e in nsBlockFrame::ReflowDirtyLines nsBlockFrame.cpp:1998
    #17 0x105ca58f7 in nsBlockFrame::Reflow nsBlockFrame.cpp:1041
    #18 0x105d0adfc in nsContainerFrame::ReflowChild nsContainerFrame.cpp:952
    #19 0x105de22e1 in nsCanvasFrame::Reflow nsCanvasFrame.cpp:472
    #20 0x105d0adfc in nsContainerFrame::ReflowChild nsContainerFrame.cpp:952
    #21 0x105dad4e9 in nsHTMLScrollFrame::ReflowScrolledFrame nsGfxScrollFrame.cpp:433
    #22 0x105dadd22 in nsHTMLScrollFrame::ReflowContents nsGfxScrollFrame.cpp:533
    #23 0x105db0ec1 in nsHTMLScrollFrame::Reflow nsGfxScrollFrame.cpp:774
    #24 0x105d0adfc in nsContainerFrame::ReflowChild nsContainerFrame.cpp:952
    #25 0x105f2c593 in ViewportFrame::Reflow nsViewportFrame.cpp:202
    #26 0x105ba0e9e in PresShell::DoReflow nsPresShell.cpp:7554
    #27 0x105bb69aa in PresShell::ProcessReflowCommands nsPresShell.cpp:7695
    #28 0x105bb5d14 in PresShell::FlushPendingNotifications nsPresShell.cpp:3907
    #29 0x105bfbd17 in nsRefreshDriver::Tick nsRefreshDriver.cpp:897
    #30 0x105c00931 in mozilla::RefreshDriverTimer::Tick nsRefreshDriver.cpp:164
    #31 0x109c1bdbe in nsTimerImpl::Fire nsTimerImpl.cpp:482
    #32 0x109c1c95e in nsTimerEvent::Run nsTimerImpl.cpp:565
    #33 0x109c09f3e in nsThread::ProcessNextEvent nsThread.cpp:627
    #34 0x109af8918 in NS_ProcessPendingEvents_P nsThreadUtils.cpp:171
    #35 0x109013f1e in nsBaseAppShell::NativeEventCallback nsBaseAppShell.cpp:97
    #36 0x108f57b0c in nsAppShell::ProcessGeckoEvents nsAppShell.mm:387
    #37 0x7fff91b4e100 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (in CoreFoundation) + 16
    #38 0x7fff91b4da24 in __CFRunLoopDoSources0 (in CoreFoundation) + 244
    #39 0x7fff91b70dc4 in __CFRunLoopRun (in CoreFoundation) + 788
    #40 0x7fff91b706b1 in CFRunLoopRunSpecific (in CoreFoundation) + 289
    #41 0x7fff8b5140a3 in RunCurrentEventLoopInMode (in HIToolbox) + 208
    #42 0x7fff8b513e41 in ReceiveNextEventCommon (in HIToolbox) + 355
    #43 0x7fff8b513cd2 in BlockUntilNextEventMatchingListInMode (in HIToolbox) + 61
    #44 0x7fff8d811612 in _DPSNextEvent (in AppKit) + 684
    #45 0x7fff8d810ed1 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127
    #46 0x108f55bd7 in -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] nsAppShell.mm:164
    #47 0x7fff8d808282 in -[NSApplication run] (in AppKit) + 516
    #48 0x108f588a9 in nsAppShell::Run nsAppShell.mm:741
    #49 0x10887b2b0 in nsAppStartup::Run nsAppStartup.cpp:291
    #50 0x1050c56ae in XREMain::XRE_mainRun nsAppRunner.cpp:3824
    #51 0x1050c6cdc in XREMain::XRE_main nsAppRunner.cpp:3891
    #52 0x1050c76ca in XRE_main nsAppRunner.cpp:4089
    #53 0x10000294f in main nsBrowserApp.cpp:174
    #54 0x100001553 in start (in firefox-bin) + 51
    #55 0x0 in 0x0000000100000000 (in firefox-bin)
0x0001294ddc63 is located 29 bytes to the left of 94-byte region [0x0001294ddc80,0x0001294ddcde)
allocated by thread T0 here:
    #0 0x10000d7cc in (anonymous namespace)::mz_malloc(_malloc_zone_t*, unsigned long) (in firefox-bin) + 44
    #1 0x7fff8c723152 in malloc_zone_malloc (in libsystem_c.dylib) + 70
    #2 0x7fff8c723ba6 in malloc (in libsystem_c.dylib) + 40
    #3 0x10274250d in moz_xmalloc mozalloc.cpp:54
    #4 0x106706bb2 in nsTextFragment::SetTo nsMemory.h:36
    #5 0x1066102ad in nsGenericDOMDataNode::SetTextInternal nsGenericDOMDataNode.cpp:290
    #6 0x106615a44 in nsGenericDOMDataNode::SetText nsGenericDOMDataNode.cpp:833
    #7 0x106542c92 in nsDocument::CreateTextNode nsIContent.h:521
    #8 0x1083541ac in nsIDOMDocument_CreateTextNode dom_quickstubs.cpp:1332
    #9 0x10b2a77fa in js::CallJSNative, JS::CallArgs const&) jscntxtinlines.h:364
    #10 0x10b29810a in js::InvokeKernel jsinterp.cpp:382
    #11 0x10b285525 in js::Interpret jsinterp.cpp:2348
    #12 0x10b873f20 in js::mjit::EnterMethodJIT MethodJIT.cpp:1066
    #13 0x10b874d42 in CheckStackAndEnterMethodJIT MethodJIT.cpp:1097
    #14 0x10b87490b in js::mjit::JaegerShot MethodJIT.cpp:1115
    #15 0x10b26ec09 in js::RunScript jsinterp.cpp:343
    #16 0x10b298024 in js::InvokeKernel jsinterp.cpp:404
    #17 0x10b299a3d in js::Invoke jsinterp.h:112
    #18 0x10b0b099e in JS_CallFunctionValue jsapi.cpp:5789
    #19 0x1082f8197 in nsXPCWrappedJSClass::CallMethod XPCWrappedJSClass.cpp:1432
    #20 0x1082e043f in nsXPCWrappedJS::CallMethod XPCWrappedJS.cpp:580
    #21 0x109c6680f in PrepareAndDispatch xptcstubs_x86_64_darwin.cpp:121
    #22 0x109c6513a in SharedStub (in XUL) + 90
Shadow byte and word:
  0x10002529bb8c: fa
  0x10002529bb88: fa fa fa fa fa fa fa fa
More shadow bytes:
  0x10002529bb68: fa fa fa fa fa fa fa fa
  0x10002529bb70: fd fd fd fd fd fd fd fd
  0x10002529bb78: fd fd fd fd fd fd fd fd
  0x10002529bb80: fa fa fa fa fa fa fa fa
=>0x10002529bb88: fa fa fa fa fa fa fa fa
  0x10002529bb90: 00 00 00 00 00 00 00 00
  0x10002529bb98: 00 00 00 06 fb fb fb fb
  0x10002529bba0: fa fa fa fa fa fa fa fa
  0x10002529bba8: fa fa fa fa fa fa fa fa
Stats: 926M malloced (702M for red zones) by 1093052 calls
Stats: 117M realloced by 48243 calls
Stats: 871M freed by 883111 calls
Stats: 728M really freed by 617834 calls
Stats: 848M (217234 full pages) mmaped in 195 calls
  mmaps   by size class: 8:425958; 9:65528; 10:24570; 11:18423; 12:6144; 13:3584; 14:1792; 15:2048; 16:1280; 17:1312; 18:64; 19:40; 20:24; 21:24; 22:11; 23:8; 24:3;
  mallocs by size class: 8:813948; 9:131520; 10:62859; 11:48412; 12:13338; 13:9046; 14:4430; 15:4650; 16:2349; 17:2137; 18:158; 19:96; 20:43; 21:36; 22:12; 23:10; 24:8;
  frees   by size class: 8:640884; 9:108444; 10:55114; 11:45679; 12:11474; 13:8453; 14:4122; 15:4467; 16:2037; 17:2107; 18:134; 19:91; 20:40; 21:35; 22:12; 23:10; 24:8;
  rfrees  by size class: 8:417410; 9:85525; 10:46759; 11:40530; 12:9482; 13:7117; 14:3698; 15:3880; 16:1242; 17:1911; 18:119; 19:66; 20:37; 21:31; 22:11; 23:10; 24:6;
Stats: malloc large: 2563 small slow: 7378

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Mats, any thoughts on sec rating?

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix, part 1

The problem is that TabWidthStore::mLimit is stored as a skipped
string offset, which can vary over the frame lifetime.
(TabWidthStore is stored on a frame property)

The fix is to store it as an unskipped offset relative the current
frame and then add the current skipped offset when using it (i.e. when
calculating 'tabsEnd').  PropertyProvider::mTabWidthsAnalyzedLimit is
also stored as a skipped string offset, but I don't think it has the
same problem since it doesn't persist between reflows (I think).
It makes the code simpler to change that too though.

Note that the TabWidth::mOffset is already stored in the same
way (i - startOffset) here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#3066

This fixes the crash and pass tests:
https://tbpl.mozilla.org/?tree=Try&rev=f6f0a6823a64

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix, part 2

I'm still a little worried that the stored indices are wrong if the text
frame's offset changes so perhaps we should store it and blow away the
property if it's not the same.  Or can't that happen for some reason?
(if so we could make this an assertion instead)

With both patches:
https://tbpl.mozilla.org/?tree=Try&rev=459fe3118d61

Comment 4

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

Comment on attachment 698924 [details] [diff] [review]
fix, part 1

Review of attachment 698924 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a comment where mLimit and mOffset are declared saying that they're DOM offsets relative to the current frame's offset.

::: layout/generic/nsTextFrameThebes.cpp
@@ +3021,5 @@
>        // tab widths; check that they're available as far as the last
>        // tab character present (if any)
>        for (uint32_t i = aStart + aLength; i > aStart; --i) {
>          if (mTextRun->CharIsTab(i - 1)) {
> +          uint32_t startOffset = mStart.GetSkippedOffset();

DebugOnly<uint32_t>

Comment 5

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

Comment on attachment 698931 [details] [diff] [review]
fix, part 2

Review of attachment 698931 [details] [diff] [review]:
-----------------------------------------------------------------

This seems good.

Comment 6

[Mats Palmgren (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> > +          uint32_t startOffset = mStart.GetSkippedOffset();
> 
> DebugOnly<uint32_t>

Good point but this whole block is inside #ifdef DEBUG.

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix, part 1

Updated code comments.

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix, part 2

Updated code comments.

Comment 9

[Mats Palmgren (inactive)]

(In reply to David Bolter [:davidb] from comment #1)
> Mats, any thoughts on sec rating?

This code is rather complicated so it's hard to guess.  I've only seen
out-of-bounds READ so far.  In theory you could get the read value reflected
in layout (at least in some cases) in the form of spacing for a "tab",
but it looks very hard to do.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 699153 [details] [diff] [review]
fix, part 1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
looks hard to me

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
not really

Which older supported branches are affected by this flaw?
all

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I expect the patches to apply to all supported branches

How likely is this patch to cause regressions; how much testing does it need?
medium risk

Comment 11

[Alex Keybl [:akeybl]]

If this sec-low bug is medium risk, let's let it ride the trains. Severity doesn't seem high enough for the risk.

Comment 12

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/678cac97bf1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/63ba23c13e9b

Comment 13

[Mats Palmgren (inactive)]

https://hg.mozilla.org/mozilla-central/rev/63ba23c13e9b
https://hg.mozilla.org/mozilla-central/rev/678cac97bf1a