Bugzilla

Comment 2

•

12 years ago

Is an overread in a text string (based on the asan log) a problem in this context, or just possibly slurping too much into the display. Of course the underlying issue could cause worse problems in other contexts, won't know until we find it.

Keywords: testcase

(no longer active)

Comment 3

•

12 years ago

(In reply to Daniel Veditz [:dveditz] from comment #2)
> Is an overread in a text string (based on the asan log) a problem in this
> context, or just possibly slurping too much into the display. Of course the
> underlying issue could cause worse problems in other contexts, won't know
> until we find it.

I'm not sure if I grok the question!  :-)

Assignee

Comment 4

•

12 years ago

Attached file stack for "ASSERTION: aPrevFrame must be the last continuation in its chain!" (obsolete) — Details

There's a few assertions leading up to the ASAN exception:

###!!! ASSERTION: aPrevFrame must be the last continuation in its chain!: '!aPrevFrame || (!aPrevFrame->GetNextContinuation() || (aPrevFrame->GetNextContinuation()->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER)) && !(aPrevFrame->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER)', file layout/base/nsFrameManager.cpp, line 456
###!!! ASSERTION: Range out of bounds: 'IsInBounds(mStart, mLength, aStart, aLength)', file layout/generic/nsTextFrameThebes.cpp, line 2812
###!!! ASSERTION: No text for IsSpace!: 'aPos < aFrag->GetLength()', file layout/generic/nsTextFrameThebes.cpp, line 639
###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file layout/base/../../content/base/src/nsTextFragment.h, line 177

This is the stack for the first assertion.

Assignee

Updated

•

12 years ago

Severity: normal → critical

Keywords: crash

(no longer active)

Comment 5

•

12 years ago

(In reply to Mats Palmgren [:mats] from comment #4)
> Created attachment 641286 [details]
> stack for "ASSERTION: aPrevFrame must be the last continuation in its chain!"
> 
> There's a few assertions leading up to the ASAN exception:
> 
> ###!!! ASSERTION: aPrevFrame must be the last continuation in its chain!:
> '!aPrevFrame || (!aPrevFrame->GetNextContinuation() ||
> (aPrevFrame->GetNextContinuation()->GetStateBits() &
> NS_FRAME_IS_OVERFLOW_CONTAINER)) && !(aPrevFrame->GetStateBits() &
> NS_FRAME_IS_OVERFLOW_CONTAINER)', file layout/base/nsFrameManager.cpp, line
> 456
> ###!!! ASSERTION: Range out of bounds: 'IsInBounds(mStart, mLength, aStart,
> aLength)', file layout/generic/nsTextFrameThebes.cpp, line 2812
> ###!!! ASSERTION: No text for IsSpace!: 'aPos < aFrag->GetLength()', file
> layout/generic/nsTextFrameThebes.cpp, line 639
> ###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file
> layout/base/../../content/base/src/nsTextFragment.h, line 177
> 
> This is the stack for the first assertion.

Hmm, that doesn't look good at all.  The bug as filed might in fact be the side-effect of something very bad happening here.  Mats, can you take this?  Or jfkthame perhaps?

Updated

•

12 years ago

Keywords: sec-critical

Comment 6

•

12 years ago

Assigning to Mats but feel free to reassign as necessary (Critsmash triage).

Assignee: nobody → matspal

Abhishek Arya

Reporter

Comment 7

•

12 years ago

Attached file Testcase — Details

Another testcase, looks similar, but feel free to split if it turns out to be different.

=================================================================
==20825== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f892366f763 at pc 0x7f8908745bc1 bp 0x7fff26ee99d0 sp 0x7fff26ee99c8
READ of size 1 at 0x7f892366f763 thread T0
    #0 0x7f8908745bc1 in nsTextFragment::CharAt(int) const asn1cmn.c:0
    #1 0x7f8909276bf6 in IsCSSWordSpacingSpace(nsTextFragment const*, unsigned int, nsStyleText const*) src/layout/generic/nsTextFrameThebes.cpp:641
    #2 0x7f890927446f in PropertyProvider::GetSpacingInternal(unsigned int, unsigned int, gfxFont::Spacing*, bool) src/layout/generic/nsTextFrameThebes.cpp:2838
    #3 0x7f8909277f33 in PropertyProvider::CalcTabWidths(unsigned int, unsigned int) src/layout/generic/nsTextFrameThebes.cpp:2968
    #4 0x7f89092748b3 in PropertyProvider::GetSpacingInternal(unsigned int, unsigned int, gfxFont::Spacing*, bool) src/layout/generic/nsTextFrameThebes.cpp:2858
    #5 0x7f890927366d in PropertyProvider::GetSpacing(unsigned int, unsigned int, gfxFont::Spacing*) src/layout/generic/nsTextFrameThebes.cpp:2797
    #6 0x7f89141e4603 in GetAdjustedSpacing(gfxTextRun*, unsigned int, unsigned int, gfxTextRun::PropertyProvider*, gfxFont::Spacing*) src/gfx/thebes/gfxFont.cpp:4528
    #7 0x7f89141e318c in gfxTextRun::GetAdjustedSpacingArray(unsigned int, unsigned int, gfxTextRun::PropertyProvider*, unsigned int, unsigned int, nsTArray<gfxFont::Spacing, nsTArrayDefaultAllocator>*) src/gfx/thebes/gfxFont.cpp:4543
    #8 0x7f89141ed60d in gfxTextRun::AccumulateMetricsForRun(gfxFont*, unsigned int, unsigned int, gfxFont::BoundingBoxType, gfxContext*, gfxTextRun::PropertyProvider*, unsigned int, unsigned int, gfxFont::RunMetrics*) src/gfx/thebes/gfxFont.cpp:4790
    #9 0x7f89141eaf97 in gfxTextRun::MeasureText(unsigned int, unsigned int, gfxFont::BoundingBoxType, gfxContext*, gfxTextRun::PropertyProvider*) src/gfx/thebes/gfxFont.cpp:4866
    #10 0x7f89092e8393 in nsTextFrame::ComputeTightBounds(gfxContext*) const src/layout/generic/nsTextFrameThebes.cpp:6936
    #11 0x7f8908ee0cc1 in nsFrame::ComputeSimpleTightBounds(gfxContext*) const src/layout/generic/nsFrame.cpp:3972
    #12 0x7f8908d681aa in nsBlockFrame::ComputeTightBounds(gfxContext*) const src/layout/generic/nsBlockFrame.cpp:861
    #13 0x7f8909f07ebf in nsMathMLContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/mathml/nsMathMLContainerFrame.cpp:895
    #14 0x7f8909f0925e in nsMathMLContainerFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/mathml/nsMathMLContainerFrame.cpp:925
    #15 0x7f89091009bf in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:822
    #16 0x7f8908db63bf in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) src/layout/generic/nsBlockFrame.cpp:3830
    #17 0x7f8908dafe1a in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) src/layout/generic/nsBlockFrame.cpp:3626
    #18 0x7f8908da2897 in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:3478
    #19 0x7f8908d9124c in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:2566
    #20 0x7f8908d76771 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) src/layout/generic/nsBlockFrame.cpp:2016
    #21 0x7f8908d69b7f in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1070
    #22 0x7f8908e59f47 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:906
    #23 0x7f890902f3a7 in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsCanvasFrame.cpp:468
    #24 0x7f8908e59f47 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:906
    #25 0x7f8908fa630e in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) src/layout/generic/nsGfxScrollFrame.cpp:519
    #26 0x7f8908fabe3a in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) src/layout/generic/nsGfxScrollFrame.cpp:619
    #27 0x7f8908fb015f in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsGfxScrollFrame.cpp:860
    #28 0x7f8908e59f47 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:906
    #29 0x7f8909388790 in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsViewportFrame.cpp:200
    #30 0x7f8908ad9436 in PresShell::DoReflow(nsIFrame*, bool) src/layout/base/nsPresShell.cpp:7454
    #31 0x7f8908b06e3d in PresShell::ProcessReflowCommands(bool) src/layout/base/nsPresShell.cpp:7595
    #32 0x7f8908b0554d in PresShell::FlushPendingNotifications(mozFlushType) src/layout/base/nsPresShell.cpp:3864
    #33 0x7f8908ba9dbb in nsRefreshDriver::Notify(nsITimer*) src/layout/base/nsRefreshDriver.cpp:396
    #34 0x7f8913bf3056 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:477
    #35 0x7f8913bf4bcc in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:558
    #36 0x7f8913bb71fd in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:625
    #37 0x7f8913845fad in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:217
    #38 0x7f891084a866 in nsXULWindow::ShowModal() src/xpfe/appshell/src/nsXULWindow.cpp:379
    #39 0x7f891082d302 in nsContentTreeOwner::ShowAsModal() src/xpfe/appshell/src/nsContentTreeOwner.cpp:529
    #40 0x7f891082d47c in non-virtual thunk to nsContentTreeOwner::ShowAsModal() asn1cmn.c:0
    #41 0x7f891065588b in nsWindowWatcher::OpenWindowJSInternal(nsIDOMWindow*, char const*, char const*, char const*, bool, nsIArray*, bool, nsIDOMWindow**) src/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:996
    #42 0x7f891064bf11 in nsWindowWatcher::OpenWindow(nsIDOMWindow*, char const*, char const*, char const*, nsISupports*, nsIDOMWindow**) src/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:381
    #43 0x7f8913cb64ca in NS_InvokeByIndex_P src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:161
    #44 0x7f890f2c26eb in CallMethodHelper::Call() src/js/xpconnect/src/XPCWrappedNative.cpp:2416
    #45 0x7f890f329a74 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1474
    #46 0x7f891a853cc7 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:382
    #47 0x7f891a7c79a1 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2424
    #48 0x7f891a747ae5 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) src/js/src/jsinterp.cpp:302
    #49 0x7f891a8540e9 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:356
    #50 0x7f891a17c890 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) src/js/src/jsinterp.h:119
    #51 0x7f891a85924d in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) src/js/src/jsinterp.cpp:388
    #52 0x7f891a024a49 in JS_CallFunctionValue src/js/src/jsapi.cpp:5569
    #53 0x7f890f271abe in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJSClass.cpp:1436
    #54 0x7f890f2179b8 in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJS.cpp:580
    #55 0x7f8913cbc080 in PrepareAndDispatch src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
    #56 0x7f8913cb9817 in SharedStub src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:0
    #57 0x7f8913cb64ca in NS_InvokeByIndex_P src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:161
    #58 0x7f890f2c26eb in CallMethodHelper::Call() src/js/xpconnect/src/XPCWrappedNative.cpp:2416
    #59 0x7f890f329a74 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1474
    #60 0x7f891a853cc7 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:382
    #61 0x7f891a7c79a1 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2424
    #62 0x7f891a747ae5 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) src/js/src/jsinterp.cpp:302
    #63 0x7f891a8540e9 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:356
0x7f892366f763 is located 29 bytes to the left of 94-byte region [0x7f892366f780,0x7f892366f7de)
allocated by thread T0 here:
    #0 0x4a4452 in __interceptor_malloc ??:0
    #1 0x7f8920bdf717 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7f8913c38343 in NS_Alloc_P src/xpcom/base/nsMemoryImpl.cpp:163
    #3 0x7f89071dbb63 in nsMemory::Alloc(unsigned long) src/../../../dist/include/nsMemory.h:36
    #4 0x7f890a7d00eb in nsTextFragment::SetTo(unsigned short const*, int, bool) src/content/base/src/nsTextFragment.cpp:264
    #5 0x7f890a513c33 in nsGenericDOMDataNode::SetTextInternal(unsigned int, unsigned int, unsigned short const*, unsigned int, bool, CharacterDataChangeInfo::Details*) src/content/base/src/nsGenericDOMDataNode.cpp:308
    #6 0x7f890a5227d9 in nsGenericDOMDataNode::SetText(unsigned short const*, unsigned int, bool) src/content/base/src/nsGenericDOMDataNode.cpp:850
    #7 0x7f8908670cc5 in nsIContent::SetText(nsAString_internal const&, bool) src/../../../../dist/include/nsIContent.h:523
    #8 0x7f890a35e82c in nsDocument::CreateTextNode(nsAString_internal const&, nsIContent**) src/content/base/src/nsDocument.cpp:4429
    #9 0x7f890f42ea54 in nsIDOMDocument_CreateTextNode(JSContext*, unsigned int, JS::Value*) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:3322
    #10 0x7f891a853cc7 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:382
    #11 0x7f891a7c79a1 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2424
    #12 0x7f891a747ae5 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) src/js/src/jsinterp.cpp:302
    #13 0x7f891a8540e9 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:356
    #14 0x7f891a17c890 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) src/js/src/jsinterp.h:119
    #15 0x7f891a85924d in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) src/js/src/jsinterp.cpp:388
    #16 0x7f891a024a49 in JS_CallFunctionValue src/js/src/jsapi.cpp:5569
    #17 0x7f890f271abe in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJSClass.cpp:1436
    #18 0x7f890f2179b8 in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJS.cpp:580
    #19 0x7f8913cbc080 in PrepareAndDispatch src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
    #20 0x7f8913cb9817 in SharedStub src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:0
    #21 0x7f890b0bd7b3 in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:794
    #22 0x7f890b0bec7a in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:867
    #23 0x7f890b25ab67 in nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.h:144
    #24 0x7f890b249806 in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) src/content/events/src/nsEventDispatcher.cpp:184
==20825== ABORTING
Stats: 204M malloced (203M for red zones) by 419604 calls
Stats: 79M realloced by 21269 calls
Stats: 157M freed by 278952 calls
Stats: 24M really freed by 42357 calls
Stats: 416M (106560 full pages) mmaped in 104 calls
  mmaps   by size class: 8:311277; 9:57337; 10:20475; 11:18423; 12:3072; 13:2048; 14:1536; 15:384; 16:576; 17:128; 18:176; 19:112; 20:40;
  mallocs by size class: 8:324925; 9:51704; 10:17898; 11:17545; 12:2679; 13:1992; 14:1491; 15:292; 16:609; 17:135; 18:187; 19:110; 20:37;
  frees   by size class: 8:202445; 9:42553; 10:14293; 11:14511; 12:1758; 13:1054; 14:1277; 15:234; 16:507; 17:106; 18:80; 19:101; 20:33;
  rfrees  by size class: 8:35516; 9:2261; 10:1696; 11:2307; 12:121; 13:208; 14:127; 15:28; 16:35; 17:17; 18:21; 19:8; 20:12;
Stats: malloc large: 469 small slow: 2059
Shadow byte and word:
  0x1ff1246cdeec: fa
  0x1ff1246cdee8: fa fa fa fa fa fa fa fa
More shadow bytes:
  0x1ff1246cdec8: fa fa fa fa fa fa fa fa
  0x1ff1246cded0: fd fd fd fd fd fd fd fd
  0x1ff1246cded8: fd fd fd fd fd fd fd fd
  0x1ff1246cdee0: fa fa fa fa fa fa fa fa
=>0x1ff1246cdee8: fa fa fa fa fa fa fa fa
  0x1ff1246cdef0: 00 00 00 00 00 00 00 00
  0x1ff1246cdef8: 00 00 00 06 fb fb fb fb
  0x1ff1246cdf00: fa fa fa fa fa fa fa fa
  0x1ff1246cdf08: fa fa fa fa fa fa fa fa

Andrew McCreight [:mccr8]

Comment 8

•

12 years ago

qawanted: The assertions happen in non-asan debug builds, so we need to test Fx 15 and ESR-10 to see if they are affected.

status-firefox16: --- → affected

status-firefox17: --- → affected

Keywords: qawanted

Comment 9

•

12 years ago

esr10 debug build produces the same assertions, so I'm assuming everything is affected.

###!!! ASSERTION: Range out of bounds: 'IsInBounds(mStart, mLength, aStart, aLength)', file /Users/amccreight/mz/esr10/layout/generic/nsTextFrameThebes.cpp, line 2664

over and over.

status-firefox-esr10: --- → affected

status-firefox14: --- → affected

status-firefox15: --- → affected

u279076

Comment 10

•

12 years ago

RE qawanted, do we have Firefox 15 and ESR10 ASAN builds I can use to test this?

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

Updated

•

12 years ago

Keywords: qawanted

Updated

•

12 years ago

status-firefox18: --- → affected

tracking-firefox16: --- → +

tracking-firefox17: --- → +

tracking-firefox18: --- → +

Comment 12

•

12 years ago

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #10)
> RE qawanted, do we have Firefox 15 and ESR10 ASAN builds I can use to test
> this?

The assertions happen in non-ASan plain debug builds, so this can be tested/verified without ASan builds.

Not convinced this is exploitable yet: we're reading beyond the end of a string in order to find its display length during reflow. By itself that may not be too bad (maybe we'll crash if we read too far). But if we're confused about the buffer's length in some fundamental way maybe a different testcase could get us to write into the incorrect buffer.

Summary: Heap-buffer-overflow in IsCSSWordSpacingSpace → Out of bounds read in IsCSSWordSpacingSpace

u279076

Comment 13

•

12 years ago

Does comment 9 address the QAWANTED request or is there more needed here?

Assignee

Comment 14

•

12 years ago

No additional QA is needed at this point, this bug affects all branches.
I think I understand the issue now.

Assignee

Updated

•

12 years ago

Assignee: matspal → nobody

Component: Layout: Text → Layout: Block and Inline

Hardware: x86_64 → All

Assignee

Comment 15

•

12 years ago

Attached file stack + frame dump for "ASSERTION: aPrevFrame must be the last continuation in its chain!" — Details

The problem starts already during frame construction where we append
frames in the middle of a continuation chain.

FindAppendPrevSibling() in nsCSSFrameConstructor.cpp does this:
  aParentFrame->GetLastChild(kPrincipalList);

it doesn't consider there could be an overflow list from an earlier
reflow (interrupted, I assume).

Attachment #641286 - Attachment is obsolete: true

Assignee

Comment 16

•

12 years ago

Attached patch wip (obsolete) — Details — Splinter Review

I guess we could make FindAppendPrevSibling (and frame construction
in general) consider the overflow list, but perhaps it would be simpler
and more robust to eliminate the overflow frames by moving them to the
end of the principal list before attempting any frame tree insertions.
Would this work?

Boris Zbarsky [:bzbarsky]

Assignee

Comment 17

•

12 years ago

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=4d65afccc615

Comment 18

•

12 years ago

Draining the overflow list before doing insertions makes sense to me.  Mats, are you planning to work on this?  It looks like you unassigned from self, but that might have been a side effect of the component change...

Assignee

Comment 19

•

12 years ago

Oops, I cleared it by mistake.  OK, I'll go ahead with that patch then...
I just need to check if there are more places in nsCSSFrameConstructor
that needs a DrainOverflowList() call.  (and I'll probably rename it
since there are other DrainOverflow* methods already and they drain both
the prev-in-flow and self)

Assignee: nobody → matspal

Comment 20

•

12 years ago

The sooner this can land and get some testing the better since we'd like to uplift this to Firefox 16 which is fast approaching.

Boris Zbarsky [:bzbarsky]

Assignee

Comment 21

•

12 years ago

Attached patch fix — Details — Splinter Review

Spawn out the latter part of nsBlockFrame::DrainOverflowLines and
nsContainerFrame::MoveOverflowToChildList into new methods named
DrainSelfOverflowList and make the frame ctor call it before
attempting to insert new frames.

https://tbpl.mozilla.org/?tree=Try&rev=3df24de39a60

Attachment #660274 - Attachment is obsolete: true

Attachment #661217 - Flags: review?(bzbarsky)

Comment 22

•

12 years ago

Comment on attachment 661217 [details] [diff] [review]
fix

>+++ b/layout/generic/nsBlockFrame.cpp
>+  bool didFindOverflow = false;

Maybe "didFindOverflowFromPrevContinuation"?

Either way, I guess.

r=me

Attachment #661217 - Flags: review?(bzbarsky) → review+

Comment 24

•

12 years ago

Bounty Awarded $3000

Assignee

Comment 25

•

12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e016faeb6b15

Flags: in-testsuite?

(no longer active)

Comment 26

•

12 years ago

http://hg.mozilla.org/mozilla-central/rev/e016faeb6b15

Status: NEW → RESOLVED

Closed: 12 years ago

status-firefox18: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla18

Comment 27

•

12 years ago

If we think this is low risk enough to uplift into FF16, please nominate for aurora/beta approval prior to Tuesday's Beta 5 go to build. If not, we'll untrack for release.

Assignee

Comment 28

•

12 years ago

Comment on attachment 661217 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: possibly exploitable crash
Testing completed (on m-c, etc.): on m-c since 2012-09-18
Risk to taking this patch (and alternatives if risky): low, the only real change is the added DrainSelfOverflowList() calls in nsCSSFrameConstructor.cpp which *should* be safe to do, the rest is just re-shuffling of existing code.
String or UUID changes made by this patch: none

Attachment #661217 - Flags: approval-mozilla-beta?

Attachment #661217 - Flags: approval-mozilla-aurora?

Comment 29

•

12 years ago

Comment on attachment 661217 [details] [diff] [review]
fix

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.

Attachment #661217 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Assignee

Comment 30

•

12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/592da114f138

status-firefox17: affected → fixed

Comment 31

•

12 years ago

Comment on attachment 661217 [details] [diff] [review]
fix

[Triage Comment]
Low risk fix for a sec-critical bug, and we've got some time to still find regressions before release. Approving for Beta.

Please also prepare an ESR10 fix.

Attachment #661217 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

•

12 years ago

tracking-firefox-esr10: --- → 16+

Assignee

Comment 32

•

12 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/e97453246c7d

status-firefox16: affected → fixed

Boris Zbarsky [:bzbarsky]

Assignee

Comment 33

•

12 years ago

Attached patch ported to esr branch — Details — Splinter Review

The beta-branch patch applied cleanly except for nsBlockFrame.cpp
which I had to change manually.  Please re-review that file.
(fwiw, crashtests/reftests pass locally)

Attachment #664359 - Flags: review?(bzbarsky)

Comment 34

•

12 years ago

Comment on attachment 664359 [details] [diff] [review]
ported to esr branch

r=me

Attachment #664359 - Flags: review?(bzbarsky) → review+

Assignee

Comment 35

•

12 years ago

Comment on attachment 664359 [details] [diff] [review]
ported to esr branch

[Approval Request Comment]
same as comment 28

Attachment #664359 - Flags: approval-mozilla-esr10?

Updated

•

12 years ago

Whiteboard: [asan] → [asan][advisory-tracking+]

bhavana bajaj [:bajaj]

Updated

•

12 years ago

Attachment #664359 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+

Assignee

Comment 36

•

12 years ago

https://hg.mozilla.org/releases/mozilla-esr10/rev/cd49928c08fb

status-firefox-esr10: affected → fixed

Matt Wobensmith [:mwobensmith][:matt:]

Updated

•

12 years ago

Alias: CVE-2012-3995

u279076

Updated

•

12 years ago

Keywords: verifyme

Updated

•

12 years ago

Blocks: 821479

Updated

•

11 years ago

Group: core-security