752176 - out-of-bounds read at nsFontVariantTextRunFactory::RebuildTextRun

Status: VERIFIED FIXED

Description

[Atte Kettunen]

Attachment: Reprofile

Reprofile as attachment.

ASAN report:

==9109== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f785d2a3a90 at pc 0x7f78882553fb bp 0x7fffcab62c90 sp 0x7fffcab62c88
READ of size 8 at 0x7f785d2a3a90 thread T0
    #0 0x7f78882553fb in nsFontVariantTextRunFactory::RebuildTextRun(nsTransformedTextRun*, gfxContext*) /home/attekett/firefox/src/../../dist/include/nsAutoPtr.h:1037
    #1 0x7f7888257656 in nsCaseTransformTextRunFactory::RebuildTextRun(nsTransformedTextRun*, gfxContext*) /home/attekett/firefox/src/layout/generic/nsTextRunTransformations.cpp:961
    #2 0x7f7888213c08 in nsTArray_base<nsTArrayDefaultAllocator>::Length() const /home/attekett/firefox/src/../../dist/include/nsTArray.h:224
    #3 0x7f788820fa2d in BuildTextRunsScanner::FlushFrames(bool, bool) /home/attekett/firefox/src/layout/generic/nsTextFrameThebes.cpp:1408
    #4 0x7f7888218841 in ~BuildTextRunsScanner /home/attekett/firefox/src/layout/generic/nsTextFrameThebes.cpp:795
    #5 0x7f788823e2f8 in nsTextFrame::ReflowText(nsLineLayout&, int, nsRenderingContext*, bool, nsHTMLReflowMetrics&, unsigned int&) /home/attekett/firefox/src/layout/generic/nsTextFrameThebes.cpp:7412
    #6 0x7f78881c11ae in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) /home/attekett/firefox/src/layout/generic/nsLineLayout.cpp:870
    #7 0x7f78881b63d9 in nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) /home/attekett/firefox/src/layout/generic/nsInlineFrame.cpp:713
    #8 0x7f78881b5836 in nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) /home/attekett/firefox/src/layout/generic/nsInlineFrame.cpp:575
    #9 0x7f78881b4da6 in nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /home/attekett/firefox/src/layout/generic/nsInlineFrame.cpp:429
    #10 0x7f78881c10f1 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) /home/attekett/firefox/src/layout/generic/nsLineLayout.cpp:863
    #11 0x7f78880ef319 in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:3834
    #12 0x7f78880ee0ce in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:3630
    #13 0x7f78880eb35a in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:3482
    #14 0x7f78880e5cc3 in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:2570
    #15 0x7f78880dfef0 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:2020
    #16 0x7f78880dab59 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:1069
    #17 0x7f78880fd63a in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) /home/attekett/firefox/src/layout/generic/nsBlockReflowContext.cpp:295
0x7f785d2a3a90 is located 0 bytes to the right of 16-byte region [0x7f785d2a3a80,0x7f785d2a3a90)
allocated by thread T0 here:
    #0 0x41b722 in malloc ??:0
    #1 0x7f788f189398 in moz_xmalloc /home/attekett/firefox/src/memory/mozalloc/mozalloc.cpp:87
==9109== ABORTING
Stats: 141M malloced (146M for red zones) by 300926 calls
Stats: 43M realloced by 18278 calls
Stats: 104M freed by 182253 calls
Stats: 0M really freed by 0 calls
Stats: 312M (79917 full pages) mmaped in 78 calls
  mmaps   by size class: 8:229362; 9:49146; 10:16380; 11:18423; 12:3072; 13:2048; 14:1536; 15:384; 16:384; 17:128; 18:160; 19:48; 20:12;
  mallocs by size class: 8:220142; 9:42496; 10:15462; 11:16463; 12:2208; 13:1672; 14:1475; 15:322; 16:352; 17:118; 18:156; 19:48; 20:12;
  frees   by size class: 8:120175; 9:32460; 10:12053; 11:13298; 12:1391; 13:797; 14:1307; 15:271; 16:285; 17:103; 18:57; 19:46; 20:10;
  rfrees  by size class:
Stats: malloc large: 334 small slow: 1634
Shadow byte and word:
  0x1fef0ba54752: fb
  0x1fef0ba54750: 00 00 fb fb fb fb fb fb
More shadow bytes:
  0x1fef0ba54730: 00 04 fb fb fb fb fb fb
  0x1fef0ba54738: fb fb fb fb fb fb fb fb
  0x1fef0ba54740: fa fa fa fa fa fa fa fa
  0x1fef0ba54748: fa fa fa fa fa fa fa fa
=>0x1fef0ba54750: 00 00 fb fb fb fb fb fb
  0x1fef0ba54758: fb fb fb fb fb fb fb fb
  0x1fef0ba54760: fa fa fa fa fa fa fa fa
  0x1fef0ba54768: fa fa fa fa fa fa fa fa
  0x1fef0ba54770: 00 00 fb fb fb fb fb fb

Comment 1

[Christian Holler (:decoder)]

Attachment: AddressSanitizer Debug Log

Confirmed using try build at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6d976534074e/try-linux64-debug/firefox-15.0a1.en-US.linux-x86_64.tar.bz2

Attached is the more detailed ASan log from this debug+opt build :)

Comment 2

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Related to bug 752168?

Comment 3

[Jonathan Kew (:jfkthame)]

Most likely, yes. I'll take a look - given a reproducible testcase, it should be debuggable without too much trouble.

Comment 4

[Jonathan Kew (:jfkthame)]

Attachment: patch, don't look at styles[] entry beyond the end of the array

This was a bug in the patch for bug 307039 - it added the local |styleContext| variable to avoid repeatedly looking up the current element in the textRun's mStyles[] array, but it's not valid to do this for the last iteration of the loop, when i==length and we're beyond the end of the array. In this case |styleContext| is not used anyway, so we shouldn't try to set it in the first place.

Comment 7

[Jonathan Kew (:jfkthame)]

Attachment: alternate version of the patch, easier to understand IMO

Comment 8

[Jonathan Kew (:jfkthame)]

Attachment: patch with added comments to clarify the tricky loop

OK, one more update - this is the same code as the previous version, but has added comments to clarify what's actually going on with the loop here.

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 621419 [details] [diff] [review]
patch with added comments to clarify the tricky loop

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

The comments help, but there's still something a bit clunky about this loop. However, I can't see a way to improve it.

Comment 10

[Jonathan Kew (:jfkthame)]

https://hg.mozilla.org/mozilla-central/rev/e6228decd18e

Comment 11

[Al Billings [:abillings]]

Verified fixed using testcase and ASAN build from 5/10.

Comment 12

[Atte Kettunen]

Does this bug have any security impact?

Comment 13

[Daniel Veditz [:dveditz]]

This does not look like a security issue -- the previous code read a possibly invalid style[i] but did not use it unless i was safely < length. A time bomb: might have triggered an access violation (denial of service) and a slight chance some future coder might have started using that handy (and usually correct) styleContext variable without checking for the i==length case. But not actually vulnerable in the pre-fixed code.

Thanks for catching and reporting this case