Closed Bug 989994 (CVE-2014-1536) Opened 6 years ago Closed 6 years ago

out of bounds read in PropertyProvider::FindJustificationRange

Categories

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

x86_64
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox29 --- wontfix
firefox30 --- verified
firefox31 --- verified
firefox-esr24 --- unaffected
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
seamonkey2.26 --- fixed

People

(Reporter: inferno, Assigned: smontagu)

References

Details

(4 keywords, Whiteboard: [asan][adv-main30+])

Attachments

(4 files, 1 obsolete file)

Attached file Testcase
>==2982==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60c0002ef74c at pc 0x7f2ce0d18cdf bp 0x7fff28700280 sp 0x7fff28700278
>READ of size 4 at 0x60c0002ef74c thread T0
>    #0 0x7f2ce0d18cde in PropertyProvider::FindJustificationRange(gfxSkipCharsIterator*, gfxSkipCharsIterator*) objdir-ff-asan/layout/generic/../../dist/include/gfxFont.h:2167
>    #1 0x7f2ce0d47a23 in nsTextFrame::TrimTrailingWhiteSpace(nsRenderingContext*) layout/generic/nsTextFrame.cpp:8204
>    #2 0x7f2ce0b43aeb in nsLineLayout::TrimTrailingWhiteSpaceIn(nsLineLayout::PerSpanData*, int*) layout/generic/nsLineLayout.cpp:2344
>    #3 0x7f2ce0b43a82 in nsLineLayout::TrimTrailingWhiteSpaceIn(nsLineLayout::PerSpanData*, int*) layout/generic/nsLineLayout.cpp:2291
>    #4 0x7f2ce0b43a82 in nsLineLayout::TrimTrailingWhiteSpaceIn(nsLineLayout::PerSpanData*, int*) layout/generic/nsLineLayout.cpp:2291
>    #5 0x7f2ce0b448bc in nsLineLayout::TrimTrailingWhiteSpace() layout/generic/nsLineLayout.cpp:2412
>    #6 0x7f2ce0b92d47 in nsBlockFrame::PlaceLine(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFloatManager::SavedState*, nsRect&, int&, bool*) layout/generic/nsBlockFrame.cpp:4042
>    #7 0x7f2ce0b917ec in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) layout/generic/nsBlockFrame.cpp:3646
>    #8 0x7f2ce0b8a720 in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3374
>    #9 0x7f2ce0b7d2b8 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2528
>    #10 0x7f2ce0b75e2f in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1072
>    #11 0x7f2ce0b8eee1 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:260
>    #12 0x7f2ce0b87876 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3099
>    #13 0x7f2ce0b7d2d5 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2525
>    #14 0x7f2ce0b75e2f in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1072
>    #15 0x7f2ce0b8eee1 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:260
>    #16 0x7f2ce0b87876 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3099
>    #17 0x7f2ce0b7d2d5 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2525
>    #18 0x7f2ce0b75e2f in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1072
>    #19 0x7f2ce0b8eee1 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:260
>    #20 0x7f2ce0b87876 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3099
>    #21 0x7f2ce0b7d2d5 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2525
>    #22 0x7f2ce0b75e2f in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1072
>    #23 0x7f2ce0bbc680 in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsContainerFrame.cpp:957
>    #24 0x7f2ce0bbd5ce in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) layout/generic/nsContainerFrame.cpp:957
>    #25 0x7f2ce0c485ec in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) layout/generic/nsGfxScrollFrame.cpp:459
>    #26 0x7f2ce0c4a527 in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) layout/generic/nsGfxScrollFrame.cpp:570
>    #27 0x7f2ce0c4c9c9 in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsGfxScrollFrame.cpp:809
>    #28 0x7f2ce0bbd5ce in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) layout/generic/nsContainerFrame.cpp:957
>    #29 0x7f2ce0d5a170 in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsViewportFrame.cpp:221
>    #30 0x7f2ce09288c1 in PresShell::DoReflow(nsIFrame*, bool) layout/base/nsPresShell.cpp:8272
>    #31 0x7f2ce093c329 in PresShell::ProcessReflowCommands(bool) layout/base/nsPresShell.cpp:8428
>    #32 0x7f2ce093ba77 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:4082
>    #33 0x7f2cdf35c9f9 in nsEventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsEventStatus*) dom/events/nsEventStateManager.cpp:5265
>    #34 0x7f2ce095a64e in PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*) layout/base/nsPresShell.cpp:7214
>    #35 0x7f2ce09591aa in PresShell::HandlePositionedEvent(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*) layout/base/nsPresShell.cpp:6981
>    #36 0x7f2ce09581d1 in PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) layout/base/nsPresShell.cpp:6781
>    #37 0x7f2cdf924291 in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) view/src/nsViewManager.cpp:783
>    #38 0x7f2ce0939b32 in PresShell::DispatchSynthMouseMove(mozilla::WidgetGUIEvent*, bool) layout/base/nsPresShell.cpp:3628
>    #39 0x7f2ce09494c7 in PresShell::ProcessSynthMouseMoveEvent(bool) layout/base/nsPresShell.cpp:5487
>    #40 0x7f2ce0968b3f in PresShell::nsSynthMouseMoveEvent::WillRefresh(mozilla::TimeStamp) layout/base/nsPresShell.h:616
>    #41 0x7f2ce096ee59 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1077
>    #42 0x7f2ce0974ef4 in mozilla::RefreshDriverTimer::Tick() layout/base/nsRefreshDriver.cpp:168
>    #43 0x7f2cdc38b814 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:551
>    #44 0x7f2cdc38be9e in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:635
>    #45 0x7f2cdc382f20 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:694
>    #46 0x7f2cdc24b4da in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
>    #47 0x7f2cdcb40059 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:95
>    #48 0x7f2cdcae93a0 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:226
>    #49 0x7f2cded80277 in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:164
>    #50 0x7f2ce1b93b38 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:276
>    #51 0x7f2ce1a03483 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4019
>    #52 0x7f2ce1a0436d in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:4088
>    #53 0x7f2ce1a051bd in XRE_main toolkit/xre/nsAppRunner.cpp:4300
>    #54 0x48c6e7 in main browser/app/nsBrowserApp.cpp:282
>    #55 0x7f2ceaa85de4 in __libc_start_main /build/buildd/eglibc-2.17/csu/libc-start.c:260
>0x60c0002ef74c is located 16 bytes to the right of 124-byte region [0x60c0002ef6c0,0x60c0002ef73c)
>allocated by thread T0 here:
>    #0 0x473d81 in malloc _asan_rtl_
>    #1 0x7f2cdd7bf1e4 in gfxFontGroup::MakeTextRun(char16_t const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) gfx/thebes/gfxFont.cpp:5958
>    #2 0x7f2ce0d0a862 in BuildTextRunsScanner::BuildTextRunForFrames(void*) layout/generic/nsTextFrame.cpp:569
>    #3 0x7f2ce0d054b8 in BuildTextRunsScanner::FlushFrames(bool, bool) layout/generic/nsTextFrame.cpp:1488
>    #4 0x7f2ce0d0db5d in BuildTextRunsScanner::ScanFrame(nsIFrame*) layout/generic/nsTextFrame.cpp:1679
>    #5 0x7f2ce0d0de73 in BuildTextRunsScanner::ScanFrame(nsIFrame*) layout/generic/nsTextFrame.cpp:1719
>    #6 0x7f2ce0d0de73 in BuildTextRunsScanner::ScanFrame(nsIFrame*) layout/generic/nsTextFrame.cpp:1719
>    #7 0x7f2ce0d12a95 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, gfxContext*, nsIFrame*, nsLineList_iterator const*, unsigned int*) layout/generic/nsTextFrame.cpp:1392
>    #8 0x7f2ce0d426d9 in nsTextFrame::ReflowText(nsLineLayout&, int, nsRenderingContext*, nsHTMLReflowMetrics&, unsigned int&) layout/generic/nsTextFrame.cpp:7800
>    #9 0x7f2ce0b3a677 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) layout/generic/nsLineLayout.cpp:849
>    #10 0x7f2ce0bcaf46 in nsFirstLetterFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsFirstLetterFrame.cpp:217
>    #11 0x7f2ce0b3a5d2 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) layout/generic/nsLineLayout.cpp:842
>    #12 0x7f2ce0ca3ce3 in nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) layout/generic/nsInlineFrame.cpp:721
>    #13 0x7f2ce0ca28f7 in nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) layout/generic/nsInlineFrame.cpp:587
>    #14 0x7f2ce0ca1441 in nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsInlineFrame.cpp:395
>    #15 0x7f2ce0b3a5d2 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) layout/generic/nsLineLayout.cpp:842
>    #16 0x7f2ce0b924ff in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) layout/generic/nsBlockFrame.cpp:3716
>    #17 0x7f2ce0b90d4c in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) layout/generic/nsBlockFrame.cpp:3513
>    #18 0x7f2ce0b8a720 in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3374
>    #19 0x7f2ce0b7d2b8 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2528
>    #20 0x7f2ce0b75e2f in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1072
>    #21 0x7f2ce0b8eee1 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:260
>    #22 0x7f2ce0b87876 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3099
>    #23 0x7f2ce0b7d2d5 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2525
>    #24 0x7f2ce0b75e2f in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1072
>    #25 0x7f2ce0b8eee1 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:260
>    #26 0x7f2ce0b87876 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3099
>    #27 0x7f2ce0b7d2d5 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2525
>    #28 0x7f2ce0b75e2f in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1072
>    #29 0x7f2ce0b8eee1 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:260
>
>SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ??
>Shadow bytes around the buggy address:
>  0x0c1880055e90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x0c1880055ea0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x0c1880055eb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x0c1880055ec0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x0c1880055ed0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
>=>0x0c1880055ee0: 00 00 00 00 00 00 00 04 fa[fa]fa fa fa fa fa fa
>  0x0c1880055ef0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x0c1880055f00: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
>  0x0c1880055f10: fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa fa
>  0x0c1880055f20: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x0c1880055f30: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
>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 right 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
>  Contiguous container OOB:fc
>  ASan internal:           fe
>==2982==ABORTING
Severity: normal → critical
Keywords: crash, testcase
Whiteboard: [asan]
When I load this testcase, it triggers several assertions:

[37314] ###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file /Users/jkew/mozdev/mc-clean/layout/base/nsLayoutUtils.cpp, line 5304
[37314] ###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxSkipChars.cpp, line 13
[37314] ###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxSkipChars.cpp, line 13
[37314] ###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxSkipChars.cpp, line 13
[37314] ###!!! ASSERTION: aPos out of range: 'aPos < GetLength()', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxFont.h, line 2742

The ASAN error is expected if we have an incorrect (out-of-date?) gfxSkipChars that doesn't match the textrun involved, as suggested by the assertions here. I don't know whether the first assertion (frame tree not empty) is directly related as well, or if that's a separate problem.
The first assertion is likely to indicate frames getting lost from the frame tree or other similar bad things; I think it's worth investigating first.
until we know more calling this sec-critical based on the assertions, but could be lower if we're just reading "text" rather than objects.
Keywords: sec-critical
Jonathan, assigning to you for now because we love you.
Assignee: nobody → jfkthame
Is this tractionable?
Flags: needinfo?(jfkthame)
Flags: needinfo?(dbaron)
Attached file simplified testcase
This is a slightly simplified version of the testcase that still hits the first assertion:

###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()'

It also shows a visible rendering bug: the text "foo" that should precede the "Z" on the second line fails to appear at all.

Checking old versions of Firefox, this testcase used to render correctly, with all the text visible; it broke during the Firefox 12 cycle.

According to mozregression, the problem originated here:
  Last good revision: 34572943a3e4 (2012-01-17)
  First bad revision: f4049f65efc6 (2012-01-18)
  Pushlog:
  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=34572943a3e4&tochange=f4049f65efc6

Simon, that pushlog range includes bug 698335, which sounds as though it might possibly be related; WDYT?
Flags: needinfo?(jfkthame) → needinfo?(smontagu)
Yes, bug 698335 looks very likely. I'll look into this.
Flags: needinfo?(smontagu)
Attached patch Testcases for checkin (obsolete) — Splinter Review
Assignee: jfkthame → smontagu
Attachment #8411066 - Flags: review?(roc)
Attached patch PatchSplinter Review
This is much the same as bug 818454 -- see bug 818454 comment 16. The patch applies the change from there to another code path where we make fluid continuations non-fluid.

The patch passed try on https://tbpl.mozilla.org/?tree=Try&rev=ad513342e18c, but I didn't want to publish the testcase on try and I don't currently have an ASAN build. Can I assume (based on comment 1) that if there are no assertions the ASAN errors have also been fixed?
Attachment #8411074 - Flags: review?(roc)
Flags: needinfo?(jfkthame)
The patch fixes the crash in my Linux64 ASAN build, fwiw.
(In reply to Simon Montagu :smontagu from comment #9)

> The patch passed try on https://tbpl.mozilla.org/?tree=Try&rev=ad513342e18c,
> but I didn't want to publish the testcase on try and I don't currently have
> an ASAN build. Can I assume (based on comment 1) that if there are no
> assertions the ASAN errors have also been fixed?

Yes; the gfxSkipChars assertions in the original testcase are directly related to the inconsistency that results in the ASAN error.

I can confirm that in my local build with this patch, neither testcase asserts any longer; and all the expected text is now rendered properly. IOW, looks good here.
Flags: needinfo?(jfkthame)
Comment on attachment 8411066 [details] [diff] [review]
Testcases for checkin

(In reply to Jonathan Kew (:jfkthame) from comment #11)

> I can confirm that in my local build with this patch, neither testcase
> asserts any longer; and all the expected text is now rendered properly. IOW,
> looks good here.

Oops, I forgot that the second testcase also tests correct rendering -- I'll convert it to a reftest.
Attachment #8411066 - Attachment is obsolete: true
Attachment #8411066 - Flags: review?(roc)
I believe this should be sec-moderate like other bugs with text run pointers (bug 818454, bug 826163, etc)
Flags: needinfo?(dveditz)
Aurora and beta are affected, and the patch applies to them and fixes the bug.
esr24 is more complicated: the assertions don't occur, but the rendering glitch in attachment 8410437 [details] does. However, the patch doesn't fix it, and I don't immediately see anything checked in since esr24 which I would expect to make a difference.
(In reply to Simon Montagu :smontagu from comment #14)
> esr24 is more complicated: the assertions don't occur, but the rendering
> glitch in attachment 8410437 [details] does. However, the patch doesn't fix
> it, and I don't immediately see anything checked in since esr24 which I
> would expect to make a difference.

The gfxSkipChars assertions will have been affected by my rewrite in bug 955957, which made gfxSkipCharsIterator more careful to check parameters, IIRC. The old code was less likely to assert if it was abused (although it may have silently done the wrong thing!). That landed for Fx29 (currently beta), so it's not part of esr24.

The initial "ASSERTION: frame tree not empty" wasn't dependent on that, however; I'm not sure what else is coming into play here. The fact that the rendering glitch happens seems to imply that the frame tree is getting messed up in a similar way, and this may well be leading to use of a mismatched textrun at some point, even if we're not actually hitting an assertion.
(In reply to Simon Montagu :smontagu from comment #9)
> ... and I don't currently have an ASAN build.

You can get Linux builds at https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/ (also available on other branches.
Blocks: 698335
Flags: needinfo?(dveditz)
Summary: Heap-buffer-overflow in PropertyProvider::FindJustificationRange → out of bounds read in PropertyProvider::FindJustificationRange
(In reply to Daniel Veditz [:dveditz] from comment #16)

> You can get Linux builds at
> https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> central-linux64-asan/ (also available on other branches.

...though not for esr24, AFAICT.
I tried the testcases here in a debug build of esr24 + the gfxSkipChars rewrite from bug 955957, in order to get the stricter assertions from that patch. Neither of the testcases here assert in that build, which implies that the ASAN out-of-bounds error also wouldn't occur, as it arises because of an out-of-range index to gfxSkipCharsIterator.

So my conclusion is that although there's clearly *some* kind of bug present in esr24 - shown by the incorrect rendering, with some of the text missing - it's not the *same* bug as reported here.
https://hg.mozilla.org/mozilla-central/rev/e6eeddc2625a

Are B2G v1.2 (Gecko 26) and v1.3 (Gecko 28) affected?
Status: NEW → RESOLVED
Closed: 6 years ago
status-b2g-v1.2: --- → ?
status-b2g-v1.3: --- → ?
Flags: needinfo?(smontagu)
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
b2g 1.3 is affected and the patch fixes the bug (tested in a desktop build of source cloned from http://hg.mozilla.org/releases/mozilla-b2g28_v1_3)

b2g 1.2 is partially affected and not fixed, exactly the same as esr 24 (see comment 14 and comment 18)
Flags: needinfo?(smontagu)
Please request Aurora and b2g28 approval on the patch then :)
Comment on attachment 8411074 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 698335
User impact if declined: potential crash
Testing completed (on m-c, etc.): Baked on trunk since 2014-04-25
Risk to taking this patch (and alternatives if risky): Some risk of regressions: this area of code has been vulnerable to regressions before (bug 722137, bug 826163). Fuzzing has been effective in finding regressions.
String or IDL/UUID changes made by this patch: N/A
Attachment #8411074 - Flags: approval-mozilla-b2g28?
Attachment #8411074 - Flags: approval-mozilla-aurora?
Comment on attachment 8411074 [details] [diff] [review]
Patch

Gecko 30 is on beta now, so moving the request over.
Attachment #8411074 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8411074 [details] [diff] [review]
Patch

It's early beta and if fuzzing has historically helped in finding regressions, we can take the slight risk here as we'll be on beta for 6 weeks to collect data on this.
Attachment #8411074 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(dbaron)
Attachment #8411074 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
Whiteboard: [asan] → [asan][adv-main30+]
Alias: CVE-2014-1536
Confirmed crash on 2014-03-01, ASan Fx30.
Verified fixed on 2014-06-03, ASan Fx30.
Verified fixed on 2014-05-07, ASan Fx31.
Status: RESOLVED → VERIFIED
Group: core-security
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.