Closed
Bug 752176
Opened 9 years ago
Closed 9 years ago
out-of-bounds read at nsFontVariantTextRunFactory::RebuildTextRun
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
VERIFIED
FIXED
mozilla15
Tracking | Status | |
---|---|---|
firefox14 | --- | unaffected |
firefox15 | --- | fixed |
firefox16 | --- | unaffected |
firefox17 | --- | unaffected |
firefox-esr10 | --- | unaffected |
People
(Reporter: attekett, Assigned: jfkthame)
References
Details
(Keywords: crash, csectype-dos, regression, Whiteboard: [advisory-tracking-][asan])
Crash Data
Attachments
(3 files, 2 obsolete files)
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•9 years ago
|
||
Confirmed using try build at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/decoder@own-hero.net-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 :)
Component: General → Layout: Text
Product: Firefox → Core
QA Contact: general → layout.fonts-and-text
Related to bug 752168?
Assignee | ||
Comment 3•9 years ago
|
||
Most likely, yes. I'll take a look - given a reproducible testcase, it should be debuggable without too much trouble.
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee: nobody → jfkthame
Attachment #621306 -
Flags: review?(smontagu)
Updated•9 years ago
|
Keywords: regression
Version: Trunk → 15 Branch
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #621416 -
Flags: review?(smontagu)
Assignee | ||
Comment 8•9 years ago
|
||
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.
Attachment #621306 -
Attachment is obsolete: true
Attachment #621416 -
Attachment is obsolete: true
Attachment #621306 -
Flags: review?(smontagu)
Attachment #621416 -
Flags: review?(smontagu)
Attachment #621419 -
Flags: review?(smontagu)
Comment 9•9 years ago
|
||
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.
Attachment #621419 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e6228decd18e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
![]() |
||
Updated•9 years ago
|
Crash Signature: [@ nsFontVariantTextRunFactory::RebuildTextRun(nsTransformedTextRun*, gfxContext*)]
Updated•9 years ago
|
status-firefox-esr10:
--- → unaffected
Comment 11•9 years ago
|
||
Verified fixed using testcase and ASAN build from 5/10.
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 12•9 years ago
|
||
Does this bug have any security impact?
Comment 13•9 years ago
|
||
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
status-firefox14:
--- → unaffected
status-firefox15:
--- → fixed
status-firefox16:
--- → unaffected
status-firefox17:
--- → unaffected
Updated•9 years ago
|
Whiteboard: [advisory-tracking-]
Updated•9 years ago
|
Group: core-security
Summary: ASAN: heap-buffer-overflow at nsFontVariantTextRunFactory::RebuildTextRun → out-of-bounds read at nsFontVariantTextRunFactory::RebuildTextRun
Whiteboard: [advisory-tracking-] → [advisory-tracking-][asan]
You need to log in
before you can comment on or make changes to this bug.
Description
•