Bug 1041512 (CVE-2014-1576)

Heap-buffer-overflow in nsTransformedTextRun::SetCapitalization

VERIFIED FIXED in Firefox 33, Firefox OS v2.0

Status

()

--
critical
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: attekett, Assigned: heycam)

Tracking

(5 keywords)

unspecified
mozilla35
crash, csectype-bounds, regression, sec-high, testcase
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?
qe-verify +

Firefox Tracking Flags

(firefox31 wontfix, firefox32 wontfix, firefox33+ verified, firefox34+ verified, firefox35+ verified, firefox-esr24 unaffected, firefox-esr3133+ verified, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main33+][adv-esr31.2+][b2g-adv-main2.2-])

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 8459558 [details]
Repro-file

Tested on:

OS: Ubuntu 12.04

Firefox: ASAN debug prebuild from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/1405918761/

Sometimes the repro-file fails to trigger the crash. If the crash doesn't occur after couple of refresh, restart the browser. Also the crash reproduces a lot more reliably with debug-build than with opt-build. 


ASAN trace:

[7111] ###!!! ASSERTION: Text run should be transformed!: 'mTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_TRANSFORMED', file /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp, line 970
=================================================================
==7111==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60c0002d1a40 at pc 0x7fdb4338f050 bp 0x7fffaeeeb670 sp 0x7fffaeeeb668
READ of size 8 at 0x60c0002d1a40 thread T0
    #0 0x7fdb4338f04f in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length() const /builds/slave/m-cen-l64-asan-d-0000000000000/build/xpcom/build/../glue/nsTArray.h:323
    #1 0x7fdb433fe598 in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::IsEmpty() const /builds/slave/m-cen-l64-asan-d-0000000000000/build/obj-firefox/xpcom/base/../../dist/include/nsTArray.h:326
    #2 0x7fdb47483e48 in nsTransformedTextRun::SetCapitalization(unsigned int, unsigned int, bool*, gfxContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextRunTransformations.cpp:60
    #3 0x7fdb46783ded in nsLineBreaker::AppendText(nsIAtom*, char16_t const*, unsigned int, unsigned int, nsILineBreakSink*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/content/base/src/nsLineBreaker.cpp:291
    #4 0x7fdb46784360 in nsLineBreaker::AppendText(nsIAtom*, unsigned char const*, unsigned int, unsigned int, nsILineBreakSink*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/content/base/src/nsLineBreaker.cpp:327
    #5 0x7fdb4745ddca in BuildTextRunsScanner::SetupBreakSinksForTextRun(gfxTextRun*, void const*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2399
    #6 0x7fdb474580b1 in BuildTextRunsScanner::SetupLineBreakerContext(gfxTextRun*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2304
    #7 0x7fdb474574ea in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:1453
    #8 0x7fdb47460bee in BuildTextRuns(gfxContext*, nsTextFrame*, nsIFrame*, nsLineList_iterator const*, nsTextFrame::TextRunType) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:1401
    #9 0x7fdb4745f4e8 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, gfxContext*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2559
    #10 0x7fdb4747a49e in nsTextFrame::AddInlineMinWidthForFlow(nsRenderingContext*, nsIFrame::InlineMinWidthData*, nsTextFrame::TextRunType) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:7018
    #11 0x7fdb4747b87f in nsTextFrame::AddInlineMinWidth(nsRenderingContext*, nsIFrame::InlineMinWidthData*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:7175
    #12 0x7fdb4731cb1d in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:733
    .
    .
    .
    #60 0x7fdb48107135 in nsAppStartup::Run() /builds/slave/m-cen-l64-asan-d-0000000000000/build/toolkit/components/startup/nsAppStartup.cpp:278
    #61 0x7fdb47f69b3b in XREMain::XRE_mainRun() /builds/slave/m-cen-l64-asan-d-0000000000000/build/toolkit/xre/nsAppRunner.cpp:4013
    #62 0x7fdb47f6af4f in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/toolkit/xre/nsAppRunner.cpp:4084
    #63 0x7fdb47f6b9d2 in XRE_main /builds/slave/m-cen-l64-asan-d-0000000000000/build/toolkit/xre/nsAppRunner.cpp:4298
    #64 0x489fed in do_main(int, char**, nsIFile*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/browser/app/nsBrowserApp.cpp:282
    #65 0x489571 in main /builds/slave/m-cen-l64-asan-d-0000000000000/build/browser/app/nsBrowserApp.cpp:643
    #66 0x7fdb529e276c in ?? ??:0
    #67 0x48925c in _start ??:0

0x60c0002d1a40 is located 0 bytes to the right of 128-byte region [0x60c0002d19c0,0x60c0002d1a40)
allocated by thread T0 here:
    #0 0x471871 in __interceptor_malloc _asan_rtl_
    #1 0x7fdb449893f8 in gfxTextRun::AllocateStorageForTextRun(unsigned long, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/gfx/thebes/gfxFont.cpp:6461
    #2 0x7fdb4497fb13 in gfxTextRun::Create(gfxTextRunFactory::Parameters const*, unsigned int, gfxFontGroup*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/gfx/thebes/gfxFont.cpp:6478
    #3 0x7fdb44980f38 in gfxFontGroup::MakeTextRun(unsigned char const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/gfx/thebes/gfxFont.cpp:5196
    #4 0x7fdb4745d811 in gfxTextRun* MakeTextRun<unsigned char>(unsigned char const*, unsigned int, gfxFontGroup*, gfxTextRunFactory::Parameters const*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:569
    #5 0x7fdb47459dc5 in BuildTextRunsScanner::BuildTextRunForFrames(void*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2145
    #6 0x7fdb47457642 in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:1472
    #7 0x7fdb47460bee in BuildTextRuns(gfxContext*, nsTextFrame*, nsIFrame*, nsLineList_iterator const*, nsTextFrame::TextRunType) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:1401
    #8 0x7fdb4745f4e8 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, gfxContext*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2559
    #9 0x7fdb4747a49e in nsTextFrame::AddInlineMinWidthForFlow(nsRenderingContext*, nsIFrame::InlineMinWidthData*, nsTextFrame::TextRunType) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:7018
    #10 0x7fdb4747b87f in nsTextFrame::AddInlineMinWidth(nsRenderingContext*, nsIFrame::InlineMinWidthData*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:7175
    #11 0x7fdb4731cb1d in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:733
    #12 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #13 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #14 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #15 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #16 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #17 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #18 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #19 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #20 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #21 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #22 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #23 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #24 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #25 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #26 0x7fdb4739806e in nsFrame::ShrinkWidthToFit(nsRenderingContext*, int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsFrame.cpp:4240
    #27 0x7fdb47364d14 in nsContainerFrame::ComputeAutoSize(nsRenderingContext*, nsSize, int, nsSize, nsSize, nsSize, bool) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsContainerFrame.cpp:899
    #28 0x7fdb47396adc in nsFrame::ComputeSize(nsRenderingContext*, nsSize, int, nsSize, nsSize, nsSize, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsFrame.cpp:4030
    #29 0x7fdb473f7c1d in nsHTMLReflowState::InitAbsoluteConstraints(nsPresContext*, nsHTMLReflowState const*, int, int, nsIAtom*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsHTMLReflowState.cpp:1458

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ??
Shadow bytes around the buggy address:
  0x0c18800522f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1880052300: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c1880052310: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
  0x0c1880052320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1880052330: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
=>0x0c1880052340: 00 00 00 00 00 00 00 00[fa]fa fa fa fa fa fa fa
  0x0c1880052350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1880052360: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c1880052370: 00 00 00 00 00 00 00 04 fa fa fa fa fa fa fa fa
  0x0c1880052380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1880052390: fa fa fa fa fa fa fa fa 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 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
==7111==ABORTING
Component: General → Layout: Text
Keywords: csectype-bounds, sec-high
Nathan, do you know how we could hit a buffer overflow while calling Length() on an nsTArray? That seems weird to me.
Flags: needinfo?(nfroyd)
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Nathan, do you know how we could hit a buffer overflow while calling
> Length() on an nsTArray? That seems weird to me.

I'd guess somebody stomping on the Header pointer for the array.  Just from looking at the stacks above, that sounds plausible...
Flags: needinfo?(nfroyd)
Jonathan, can you recreate this bug? (Or find someone to take a look?)
Flags: needinfo?(jfkthame)
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Nathan, do you know how we could hit a buffer overflow while calling
> Length() on an nsTArray? That seems weird to me.

I don't think that's exactly what's happening. The problem here is that we've got a normal gfxTextRun, as created via gfxFontGroup::MakeTextRun; but then we're treating it as an nsTransformedTextRun, which is a subclass that has some additional fields. The nsTArray we're trying to look at in SetCapitalization is one of those added fields, but because this textrun was not an nsTransformedTextRun at all, the field isn't present, and the attempt to access its header ends up looking beyond the allocated size of the object. The problem isn't that the nsTArray we're examining has been stomped; it's that there was no nsTArray there to be examined.

The "ASSERTION: Text run should be transformed!" that's firing before the ASan error confirms this, too. That assertion is at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#969, and if it fires, then the static_cast immediately following is clearly invalid, and will lead directly to the failure here.

So we need to figure out why we haven't got the kind of textrun we expected here. Offhand, my guess is that perhaps we're failing to discard and rebuild a textrun at some point when style information changes dynamically. A reduced/simplified testcase that reproduces the assertion would be awesome... maybe something that involves changing line widths, so that we're constantly re-doing linebreaking, along with toggling the text-transform:capitalize property.
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #4)
>  A reduced/simplified testcase that reproduces the assertion would be
> awesome... maybe something that involves changing line widths, so that we're
> constantly re-doing linebreaking, along with toggling the
> text-transform:capitalize property.

My naive attempt to make a testcase along these lines doesn't seem to hit the problem at all. The original testcase here does hit the assertion (and then crashes), but it's far from being easily debuggable. Mats, you have any insight on how to track this down?
Flags: needinfo?(mats)
Created attachment 8475392 [details] [diff] [review]
wip

As you guessed, it's a "text-transform:capitalize" style change
that makes the existing textrun bogus since it's of the wrong type
for the new style data. 

I don't know where we're supposed to detect this and discard the
old textrun, but DidSetStyleContext seems like a good candidate.
Flags: needinfo?(mats)
It also crashes a -release DEBUG build on Linux64 so I suspect it affects
all branches.
Severity: normal → critical
status-b2g18: --- → affected
status-b2g-v1.1hd: --- → affected
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox-esr24: --- → affected
status-firefox-esr31: --- → affected
status-seamonkey2.26: --- → affected
Keywords: crash, testcase
OS: Linux → All
Hardware: x86_64 → All
It seems odd that that would be necessary given that nsStyleText::CalcDifference returns NS_STYLE_HINT_REFLOW which includes nsChangeHint_ClearAncestorIntrinsics and nsChangeHint_ClearDescendantIntrinsics, and nsTextFrame::MarkIntrinsicISizesDirty already calls ClearTextRuns.  I'm curious which step along that path isn't happening as it should be.
Also, if that WIP fixes things, than anything pre-Gecko 31 is not affected, since this would be a regression from bug 950526.
I think the frame in question had not been reflowed at all, so
RestyleManager::StyleChangeReflow took the early return.
Interesting.

I guess I see two possible fixes for that problem:

 (1) push the not-reflowed-at-all test deeper in, into the dirty-bits part of FrameNeedsReflow

 (2) add a separate style change hint for clearing text runs

Presumably this will show up in the form of correctness bugs as well.

(I'm curious how we managed to build text runs for a frame and not reflow it before going back to the event loop, but I guess that should be allowable, given interruptible reflow.  Or do we build text runs in frame construction?)
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #9)
> Also, if that WIP fixes things, than anything pre-Gecko 31 is not affected,
> since this would be a regression from bug 950526.

The WIP fixes the crash, so the unconditional ClearTextRuns() in
nsTextFrame::DidSetStyleContext in builds before bug 950526 should
make them unaffected.
https://bugzilla.mozilla.org/attachment.cgi?id=8393354&action=diff#a/layout/generic/nsTextFrame.cpp_sec2

FWIW, I confirmed by testing an ESR24 DEBUG build on Linux64 without crashing.
status-b2g18: affected → unaffected
status-b2g-v1.1hd: affected → ---
status-b2g-v1.2: affected → ---
status-b2g-v1.3: affected → ---
status-b2g-v1.3T: affected → ---
status-b2g-v1.4: affected → ---
status-b2g-v2.0: affected → ---
status-b2g-v2.1: affected → ---
status-seamonkey2.26: affected → ---
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #11)
> (I'm curious how we managed to build text runs for a frame and not reflow it
> before going back to the event loop, but I guess that should be allowable,
> given interruptible reflow.  Or do we build text runs in frame construction?)

This is the stack where we create the initial text run:
#0  nsTextFrame::SetTextRun
#1  BuildTextRunsScanner::AssignTextRun
#2  BuildTextRunsScanner::BuildTextRunForFrames
#3  BuildTextRunsScanner::FlushFrames
#4  BuildTextRuns
#5  nsTextFrame::EnsureTextRun
#6  nsTextFrame::AddInlineMinISizeForFlow
#7  nsTextFrame::AddInlineMinISize
#8  nsBlockFrame::GetMinISize
#9  nsLayoutUtils::IntrinsicForContainer
... GetMinISize / IntrinsicForContainer ...
#23 nsLayoutUtils::IntrinsicForContainer
#24 nsBlockFrame::GetMinISize
#25 nsFrame::ShrinkWidthToFit
#26 nsContainerFrame::ComputeAutoSize
#27 nsFrame::ComputeSize
#28 nsHTMLReflowState::InitAbsoluteConstraints
#29 nsHTMLReflowState::InitConstraints
#30 nsHTMLReflowState::Init
#31 nsHTMLReflowState::nsHTMLReflowState
#32 nsAbsoluteContainingBlock::ReflowAbsoluteFrame
#33 nsAbsoluteContainingBlock::Reflow
#34 ViewportFrame::Reflow
#35 PresShell::DoReflow
...

The Reflow at #33 is interrupted before we reflow the text frame (at an
ancestor 4-5 levels up).  We exit ViewportFrame::Reflow and then the
restyling happens.  The next time we enter ViewportFrame::Reflow we
crash with the same stack up to FlushFrames, see comment 0.
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #11)
> I guess I see two possible fixes for that problem:
> 
>  (1) push the not-reflowed-at-all test deeper in, into the dirty-bits part
> of FrameNeedsReflow
> 
>  (2) add a separate style change hint for clearing text runs

Both those means clearing text runs for all text frame descendants, no?

It would be nice if we could avoid that on text frames where the
style context didn't change.
The properties that require clearing text runs are almost entirely inherited properties, so many if not most changes of those properties will also change all descendants.

When we consider optimization, we should consider it from the starting point of correct code, and consider it when we have reason to think that optimization is necessary.  I don't think it's worth trying to micro-optimize this one case by papering over the bug rather than fixing the real problem, which will show up in a bunch of other ways than just text-transform cases (although those might be the only ones that crash).

It might make some sense to take your patch as a *secondary* belt-and-suspenders fix for the bug.  (It would be nice to assert that we don't hit the need for it, but I think things happen in the wrong order to be able to do that.)
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Keywords: regression
Any updates on a patch for this?
Assignee: nobody → mats
status-firefox31: affected → wontfix
status-firefox32: affected → wontfix
status-firefox35: --- → affected
Flags: needinfo?(mats)
I guess we could take my "wip" patch as a wallpaper for branches if you want.

The preferred fix, described in comment 15, is mostly style system work.
Cameron can take it perhaps?
Assignee: mats → nobody
Component: Layout: Text → CSS Parsing and Computation
Flags: needinfo?(mats) → needinfo?(cam)
(Assignee)

Updated

4 years ago
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
(Assignee)

Comment 19

4 years ago
(In reply to David Baron [:dbaron] (UTC+2) (needinfo? for questions) (away/busy until Sep 11) from comment #11)
> I guess I see two possible fixes for that problem:
> 
>  (1) push the not-reflowed-at-all test deeper in, into the dirty-bits part
> of FrameNeedsReflow
> 
>  (2) add a separate style change hint for clearing text runs

I went with the first approach.  I tried adding a change hint initially, but since I'd need to treat this new hint in pretty much the same way as the existing Clear{Ancestor,Descendant}Intrinsics hint, it didn't seem worth it in the end.
(Assignee)

Comment 20

4 years ago
Created attachment 8487740 [details] [diff] [review]
patch
Attachment #8487740 - Flags: review?(dbaron)
(Assignee)

Comment 21

4 years ago
By the way, is there a way to write a test that relies on interruptible reflow interrupting at a certain point?
Attachment #8487740 - Flags: review?(dbaron) → review+
(Assignee)

Comment 22

4 years ago
Comment on attachment 8487740 [details] [diff] [review]
patch

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

Not obvious from the patch.

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

No.

Which older supported branches are affected by this flaw?

Whichever branches are based on Gecko 31.

If not all supported branches, which bug introduced the flaw?

Bug 950526.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

No but I suspect the patch will apply to older branches since the changed bits of code haven't been touched lately.

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

Unlikely to cause regressions.  Not sure how to make an automated regression test for it.  Manual testing shows that the ASAN issue with the test case in this bug is resolved by the patch.
Attachment #8487740 - Flags: sec-approval?
sec-approval+ for trunk. We should get Aurora, Beta, and ESR31 patches nominated as well.
status-firefox-esr24: affected → unaffected
tracking-firefox33: --- → +
tracking-firefox34: --- → +
tracking-firefox35: --- → +
tracking-firefox-esr31: --- → +
Attachment #8487740 - Flags: sec-approval? → sec-approval+
tracking-firefox-esr31: + → 33+
(Assignee)

Comment 25

4 years ago
Comment on attachment 8487740 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 35
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: bug 950526
[User impact if declined]: sg:high bug remains
[Describe test coverage new/current, TBPL]: tested locally with the test case in this bug; only just landed on inbound
[Risks and why]: should be low, as we're only calling MarkIntrinsicISizesDirty on some frames where we didn't before
[String/UUID change made/needed]: none
Attachment #8487740 - Flags: approval-mozilla-esr31?
Attachment #8487740 - Flags: approval-mozilla-beta?
Attachment #8487740 - Flags: approval-mozilla-aurora?
Attachment #8487740 - Flags: approval-mozilla-esr31?
Attachment #8487740 - Flags: approval-mozilla-esr31+
Attachment #8487740 - Flags: approval-mozilla-beta?
Attachment #8487740 - Flags: approval-mozilla-beta+
Attachment #8487740 - Flags: approval-mozilla-aurora?
Attachment #8487740 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/f840facdeb03
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g18: unaffected → ---
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox35: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
https://hg.mozilla.org/releases/mozilla-aurora/rev/80e3a7e6be35
https://hg.mozilla.org/releases/mozilla-beta/rev/dafe68644b45
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/cfedc95605d4
https://hg.mozilla.org/releases/mozilla-esr31/rev/d7140f3afd61
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: affected → fixed
status-firefox33: affected → fixed
status-firefox34: affected → fixed
status-firefox-esr31: affected → fixed
Confirmed crash on Fx31.1.0esr, ASan build
Verified fixed on Fx31.1.1esr.
status-firefox-esr31: fixed → verified
Reproduced the original issue using the following build that was mentioned in comment #0:
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/1405918761/

fx35: Passed
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/1412330524/

fx34: Passed
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan-debug/1412347357/

fx33: Passed
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan-debug/1412347593/

Test Cases used on all the channels:

- Opened the poc from comment #0 in three different windows/tabs and let each page refresh several times
- Opened the poc from comment #0 in three different private window/tabs and let each page refresh several times
- Opened the poc from comment #0 in three different e10s window/tabs and let each page refresh several times (m-c only)
Status: RESOLVED → VERIFIED
status-firefox33: fixed → verified
status-firefox34: fixed → verified
status-firefox35: fixed → verified
Flags: qe-verify+
Whiteboard: [adv-main33+][adv-esr31.2+]
Alias: CVE-2014-1576
Whiteboard: [adv-main33+][adv-esr31.2+] → [adv-main33+][adv-esr31.2+][b2g-adv-main2.2-]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.