Last Comment Bug 798853 - (CVE-2012-4216) Heap-use-after-free in gfxFont::GetFontEntry
(CVE-2012-4216)
: Heap-use-after-free in gfxFont::GetFontEntry
Status: RESOLVED FIXED
[asan][adv-track-main17+][adv-track-e...
: csectype-uaf, reproducible, sec-critical, testcase
Product: Core
Classification: Components
Component: Graphics: Text (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla19
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 822877
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-06 21:47 PDT by Abhishek Arya
Modified: 2014-07-24 14:38 PDT (History)
11 users (show)
dveditz: sec‑bounty+
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
17+
fixed


Attachments
Testcase (289 bytes, text/html)
2012-10-06 21:47 PDT, Abhishek Arya
no flags Details
gfxFontCache::AddNew stack with sizeAdjust==NaN (8.49 KB, text/html)
2012-10-13 20:03 PDT, Mats Palmgren (:mats)
no flags Details
fix (3.07 KB, patch)
2012-10-13 20:20 PDT, Mats Palmgren (:mats)
roc: review-
Details | Diff | Review
fix, part 2 (2.80 KB, patch)
2012-10-13 20:24 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
stack (3.66 KB, text/plain)
2012-10-15 17:43 PDT, Mats Palmgren (:mats)
no flags Details
gfx, add a few asserts (2.38 KB, patch)
2012-10-15 17:56 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Review
style system, more asserts and clamp NaNs in RestrictValue (6.47 KB, patch)
2012-10-15 18:11 PDT, Mats Palmgren (:mats)
roc: review+
dbaron: review+
Details | Diff | Review
gfx, add a few asserts and stop using float/double comparisons in gfxFontStyle::Equals (3.78 KB, patch)
2012-10-16 08:25 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Review
gfx, add a few asserts and stop using float/double comparisons in gfxFontStyle::Equals (4.22 KB, patch)
2012-10-16 18:34 PDT, Mats Palmgren (:mats)
mats: review+
Details | Diff | Review
style system, more asserts and clamp NaNs in RestrictValue (6.68 KB, patch)
2012-10-16 18:42 PDT, Mats Palmgren (:mats)
dbaron: review+
Details | Diff | Review
style system, more asserts and clamp NaNs in RestrictValue (6.53 KB, patch)
2012-10-18 15:45 PDT, Mats Palmgren (:mats)
mats: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
dveditz: sec‑approval+
Details | Diff | Review
Import a few mfbt/ files from trunk (verbatim) (18.42 KB, patch)
2012-10-25 19:00 PDT, Mats Palmgren (:mats)
jwalden+bmo: review-
Details | Diff | Review
Some minor changes to mfbt/FloatingPoint.h to adapt it to esr10. (3.29 KB, patch)
2012-10-25 19:09 PDT, Mats Palmgren (:mats)
jwalden+bmo: review+
Details | Diff | Review
Import mfbt/FloatingPoint.h from trunk (verbatim) (8.31 KB, patch)
2012-10-29 15:37 PDT, Mats Palmgren (:mats)
jwalden+bmo: review+
Details | Diff | Review
Some minor changes to mfbt/FloatingPoint.h to adapt it to esr10. (6.33 KB, patch)
2012-10-29 15:39 PDT, Mats Palmgren (:mats)
jwalden+bmo: review+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description Abhishek Arya 2012-10-06 21:47:59 PDT
Created attachment 668876 [details]
Testcase

Reproduces on trunk. Let the simple testcase run for like 2-5 min to see it crash automatically and reliably.

=================================================================
==6069== ERROR: AddressSanitizer heap-use-after-free on address 0x7fdcc8f3a498 at pc 0x7fdcf32ee33e bp 0x7fff3ac5bef0 sp 0x7fff3ac5bee8
READ of size 8 at 0x7fdcc8f3a498 thread T0
    #0 0x7fdcf32ee33d in nsRefPtr<gfxFontEntry>::get() const ../../dist/include/nsAutoPtr.h:1003
    #1 0x7fdcf3f01350 in gfxFont::GetFontEntry() ../../dist/include/gfxFont.h:1497
    #2 0x7fdcfe894c4c in gfxFontCache::HashEntry::KeyEquals(gfxFontCache::Key const*) const gfx/thebes/gfxFont.cpp:1255
    #3 0x7fdcfe948699 in nsTHashtable<gfxFontCache::HashEntry>::s_MatchEntry(PLDHashTable*, PLDHashEntryHdr const*, void const*) ../../dist/include/nsTHashtable.h:440
    #4 0x7fdcfdf2e9da in SearchTable(PLDHashTable*, void const*, unsigned int, PLDHashOperator) objdir-ff-asan-sym/xpcom/build/pldhash.cpp:434
    #5 0x7fdcfdf2b7c5 in PL_DHashTableOperate objdir-ff-asan-sym/xpcom/build/pldhash.cpp:587
    #6 0x7fdcfe895d87 in nsTHashtable<gfxFontCache::HashEntry>::GetEntry(gfxFontCache::Key const&) const ../../dist/include/nsTHashtable.h:147
    #7 0x7fdcfe897532 in gfxFontCache::DestroyFont(gfxFont*) gfx/thebes/gfxFont.cpp:1321
    #8 0x7fdcfe8965da in gfxFontCache::NotifyExpired(gfxFont*) gfx/thebes/gfxFont.cpp:1314
    #9 0x7fdcfe9470da in nsExpirationTracker<gfxFont, 3u>::AgeOneGeneration() ../../dist/include/nsExpirationTracker.h:188
    #10 0x7fdcfe9466b3 in nsExpirationTracker<gfxFont, 3u>::TimerCallback(nsITimer*, void*) ../../dist/include/nsExpirationTracker.h:311
    #11 0x7fdcfe2c6832 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:473
    #12 0x7fdcfe2c7d3a in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:556
    #13 0x7fdcfe28b440 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:612
    #14 0x7fdcfdf1c2bb in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #15 0x7fdcfc8c7fbd in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:117
    #16 0x7fdcfe564ab1 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:208
    #17 0x7fdcfe5648e6 in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:201
    #18 0x7fdcfe5647cb in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:175
    #19 0x7fdcfbd6c3ea in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #20 0x7fdcfa9a3df4 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:290
    #21 0x7fdcf0c7823d in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3782
    #22 0x7fdcf0c7e0b5 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3848
    #23 0x7fdcf0c80f64 in XRE_main toolkit/xre/nsAppRunner.cpp:3923
    #24 0x40baa3 in do_main(int, char**) browser/app/nsBrowserApp.cpp:174
    #25 0x4091e5 in main browser/app/nsBrowserApp.cpp:279
    #26 0x7fdd0f01b76c in ?? ??:0
0x7fdcc8f3a498 is located 24 bytes inside of 392-byte region [0x7fdcc8f3a480,0x7fdcc8f3a608)
freed by thread T0 here:
    #0 0x4c3580 in __interceptor_free ??:?
    #1 0x7fdd0be93406 in moz_free memory/mozalloc/mozalloc.cpp:51
    #2 0x7fdcfea9372d in operator delete(void*) ../../dist/include/mozilla/mozalloc.h:224
    #3 0x7fdcfe897716 in gfxFontCache::DestroyFont(gfxFont*) gfx/thebes/gfxFont.cpp:1326
    #4 0x7fdcfe8965da in gfxFontCache::NotifyExpired(gfxFont*) gfx/thebes/gfxFont.cpp:1314
    #5 0x7fdcfe9470da in nsExpirationTracker<gfxFont, 3u>::AgeOneGeneration() ../../dist/include/nsExpirationTracker.h:188
    #6 0x7fdcfe9466b3 in nsExpirationTracker<gfxFont, 3u>::TimerCallback(nsITimer*, void*) ../../dist/include/nsExpirationTracker.h:311
    #7 0x7fdcfe2c6832 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:473
    #8 0x7fdcfe2c7d3a in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:556
    #9 0x7fdcfe28b440 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:612
    #10 0x7fdcfdf1c2bb in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #11 0x7fdcfc8c7fbd in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:117
    #12 0x7fdcfe564ab1 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:208
    #13 0x7fdcfe5648e6 in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:201
    #14 0x7fdcfe5647cb in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:175
    #15 0x7fdcfbd6c3ea in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #16 0x7fdcfa9a3df4 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:290
    #17 0x7fdcf0c7823d in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3782
    #18 0x7fdcf0c7e0b5 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3848
    #19 0x7fdcf0c80f64 in XRE_main toolkit/xre/nsAppRunner.cpp:3923
    #20 0x40baa3 in do_main(int, char**) browser/app/nsBrowserApp.cpp:174
    #21 0x4091e5 in main browser/app/nsBrowserApp.cpp:279
    #22 0x7fdd0f01b76c in ?? ??:0
previously allocated by thread T0 here:
    #0 0x4c3640 in malloc ??:?
    #1 0x7fdd0be9355a in moz_xmalloc memory/mozalloc/mozalloc.cpp:57
    #2 0x7fdcfea9b49c in operator new(unsigned long) ../../dist/include/mozilla/mozalloc.h:200
    #3 0x7fdcfea8c6d0 in gfxFcFontSet::GetFontAt(unsigned int, gfxFontStyle const*) gfx/thebes/gfxPangoFonts.cpp:1203
    #4 0x7fdcfea8b6d8 in gfxPangoFontGroup::GetBaseFontSet() gfx/thebes/gfxPangoFonts.cpp:2503
    #5 0x7fdcfea8b038 in gfxPangoFontGroup::GetBaseFont() gfx/thebes/gfxPangoFonts.cpp:1936
    #6 0x7fdcfea8ca32 in gfxPangoFontGroup::GetFontAt(int) gfx/thebes/gfxPangoFonts.cpp:1954
    #7 0x7fdcf24ae390 in nsFontMetrics::GetMetrics() const gfx/src/nsFontMetrics.cpp:128
    #8 0x7fdcf24af6fe in nsFontMetrics::ExternalLeading() gfx/src/nsFontMetrics.cpp:191
    #9 0x7fdcf3017179 in GetNormalLineHeight(nsFontMetrics*) layout/generic/nsHTMLReflowState.cpp:2331
    #10 0x7fdcf3015eef in ComputeLineHeight(nsStyleContext*, int, float) layout/generic/nsHTMLReflowState.cpp:2389
    #11 0x7fdcf301521c in nsHTMLReflowState::CalcLineHeight(nsStyleContext*, int, float) layout/generic/nsHTMLReflowState.cpp:2411
    #12 0x7fdcf3014f08 in nsHTMLReflowState::CalcLineHeight() const layout/generic/nsHTMLReflowState.cpp:2400
    #13 0x7fdcf2db4b27 in nsBlockReflowState layout/generic/nsBlockReflowState.cpp:113
    #14 0x7fdcf2d21d1c in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:998
    #15 0x7fdcf2dafdb0 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:268
    #16 0x7fdcf2d53101 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3090
    #17 0x7fdcf2d47ee2 in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:2473
    #18 0x7fdcf2d2f2e7 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:1993
    #19 0x7fdcf2d2259d in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1045
    #20 0x7fdcf2dafdb0 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:268
    #21 0x7fdcf2d53101 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3090
    #22 0x7fdcf2d47ee2 in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:2473
    #23 0x7fdcf2d2f2e7 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:1993
    #24 0x7fdcf2d2259d in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1045
Shadow byte and word:
  0x1ffb991e7493: fd
  0x1ffb991e7490: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ffb991e7470: fa fa fa fa fa fa fa fa
  0x1ffb991e7478: fa fa fa fa fa fa fa fa
  0x1ffb991e7480: fa fa fa fa fa fa fa fa
  0x1ffb991e7488: fa fa fa fa fa fa fa fa
=>0x1ffb991e7490: fd fd fd fd fd fd fd fd
  0x1ffb991e7498: fd fd fd fd fd fd fd fd
  0x1ffb991e74a0: fd fd fd fd fd fd fd fd
  0x1ffb991e74a8: fd fd fd fd fd fd fd fd
  0x1ffb991e74b0: fd fd fd fd fd fd fd fd
Stats: 252M malloced (278M for red zones) by 421919 calls
Stats: 43M realloced by 24773 calls
Stats: 228M freed by 300591 calls
Stats: 96M really freed by 189262 calls
Stats: 492M (126043 full pages) mmaped in 123 calls
  mmaps   by size class: 8:294894; 9:32764; 10:12285; 11:14329; 12:3072; 13:1536; 14:1280; 15:256; 16:1152; 17:1280; 18:144; 19:40; 20:24;
  mallocs by size class: 8:351457; 9:34855; 10:9666; 11:16641; 12:2693; 13:1915; 14:1554; 15:317; 16:1289; 17:1310; 18:159; 19:41; 20:22;
  frees   by size class: 8:246111; 9:26679; 10:6463; 11:13396; 12:1839; 13:1717; 14:1390; 15:267; 16:1233; 17:1294; 18:144; 19:39; 20:19;
  rfrees  by size class: 8:165441; 9:9318; 10:2157; 11:8872; 12:662; 13:589; 14:588; 15:146; 16:1045; 17:414; 18:25; 19:4; 20:1;
Stats: malloc large: 1532 small slow: 2354
==6069== ABORTING
Comment 1 Mats Palmgren (:mats) 2012-10-13 20:03:36 PDT
Created attachment 671164 [details]
gfxFontCache::AddNew stack with sizeAdjust==NaN

The root of the problem is the large 'font-size-adjust' value in the
testcase results in gfxFontStyle::sizeAdjust having a NaN float value.
The sizeAdjust is used as part of the hashtable key for entries in
gfxFontCache -- here's the stack when we add this entry.

Since ptr->sizeAdjust == ptr->sizeAdjust is false for a NaN, we're
never going to be able to lookup this entry again.
When the last reference goes away (NotifyReleased), we add it to the
expiration timer queue, and when that expires (NotifyExpired):
http://hg.mozilla.org/mozilla-central/annotate/5c37e5656453/gfx/thebes/gfxFont.cpp#l1310
it calls DestroyFont, where mFonts.GetEntry(key) fails to find it
and we delete it even though its entry is still in mFonts...
with gfxFontCache::HashEntry::mFonts now dangling:
http://hg.mozilla.org/mozilla-central/annotate/5c37e5656453/gfx/thebes/gfxFont.h#l854
Comment 2 Mats Palmgren (:mats) 2012-10-13 20:20:41 PDT
Created attachment 671165 [details] [diff] [review]
fix

Map NaN to std::numeric_limits::max() with the same sign.
Comment 3 Mats Palmgren (:mats) 2012-10-13 20:24:57 PDT
Created attachment 671166 [details] [diff] [review]
fix, part 2

I think we should also fix the style system in the same way -
it seems dangerous to allow it to continue spreading NaN values.

If you agree, we should probably put the ClampNanToLimits template
in a header file somewhere.  Suggestions on where?
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-14 14:50:35 PDT
Comment on attachment 671165 [details] [diff] [review]
fix

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

Actually, I think we should cut these NaNs off much earlier.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-14 14:52:51 PDT
I haven't delved into exactly where these NaNs are coming from, but the CSS parser should clamp numeric values to something reasonable. There is no reason to support a font-size-adjust of more than 100, for example, and clamping it to 100 will keep us far away from trouble.
Comment 6 Mats Palmgren (:mats) 2012-10-15 17:43:07 PDT
Created attachment 671671 [details]
stack

This is where the NaN comes from, it's the result of zero * infinity:
http://hg.mozilla.org/mozilla-central/annotate/cd3270dc35cc/layout/style/nsStyleAnimation.cpp#l1734
for "aCoeff2 * aValue2.GetFloatValue()"
Comment 7 Mats Palmgren (:mats) 2012-10-15 17:56:20 PDT
Created attachment 671676 [details] [diff] [review]
gfx, add a few asserts

I feel slightly uncomfortable to just leave behind a few debug
assertions rather than handling NaNs here.
Comment 8 Mats Palmgren (:mats) 2012-10-15 18:11:40 PDT
Created attachment 671680 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

This fixes the crash by clamping a NaN value to infinity (same sign).

We don't seem to have a general mechanism to clamp values to a specific
range, apart from the CSS_PROPERTY_VALUE_NONNEGATIVE / AT_LEAST_ONE
currently in RestrictValue.  So I don't know where to clamp
font-size-adjust to 0 .. 100 for example.  Seems like it could be done
in a separate bug though.

With this patch the testcase still asserts:
###!!! ASSERTION: Negative ascent???: 'aMetrics.ascent >= 0', file layout/generic/nsTextFrameThebes.cpp, line 7987
###!!! ASSERTION: bad height: 'metrics.height>=0', file layout/generic/nsLineLayout.cpp, line 926

Both are already reported, bug 714568, bug 408744 and bug 576358,
so I'm not sure if it's essential to fix in this bug.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-15 18:20:54 PDT
Comment on attachment 671680 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

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

Looks good to me, but I actually think we should go further and clamp infinities to large finite values as well, to reduce the chances of code getting confused.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-15 18:21:13 PDT
Also, you'll need review from dbaron.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-15 18:22:09 PDT
It looks like the underlying bug here is that nsCSSScanner::ParseNumber clamps over-large numbers to INT32_MAX, but doesn't do anything to avoid floats overflowing to infinity. I think ParseNumber should clamp float values to some limit. We could clamp them to INT32_MIN/MAX for consistency, but that might cause problems for people writing SVG numbers with exponents, so I suggest clamping near the max/min float values instead.
Comment 12 Mats Palmgren (:mats) 2012-10-15 18:25:22 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Looks good to me, but I actually think we should go further and clamp
> infinities to large finite values as well

Yeah, it's a bug in my comment rather than the code.
I'm using std::numeric_limits<float>::max(), which is the max finite value.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-15 22:29:07 PDT
Comment on attachment 671680 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

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

Looks good to me, but I actually think we should go further and clamp infinities to large finite values as well, to reduce the chances of code getting confused.

::: layout/style/nsStyleAnimation.cpp
@@ +866,5 @@
>  
> +// Clamp float/double NaN values to infinity (with same sign) as a general
> +// restriction for floating point values in RestrictValue.  It's also required
> +// for RestrictValue to work as expected when comparing the value,
> +// e.g. -NaN < 0 is false.

Fix the comment
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-16 03:10:32 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Looks good to me, but I actually think we should go further and clamp
> infinities to large finite values as well, to reduce the chances of code
> getting confused.

Ignore this. Silly Splinter.
Comment 15 Mats Palmgren (:mats) 2012-10-16 08:19:50 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> It looks like the underlying bug here is that nsCSSScanner::ParseNumber
> clamps over-large numbers to INT32_MAX, but doesn't do anything to avoid
> floats overflowing to infinity.

Yes, the infinite value originates in nsCSSScanner::ParseNumber.
I don't think it's worth hunting possible causes of infinity in the style
system though, there's just to many of them!  Clamping NaN when assigning
nsCSSValue::mFloat is the best wallpaperish fix I can think of, but perhaps
it's not worth it.  Other than that, I guess the rest of Layout just have to
cope with NaNs -- we should at least make sure no float/double values are used
for hash table lookup!
Comment 16 Mats Palmgren (:mats) 2012-10-16 08:25:24 PDT
Created attachment 671871 [details] [diff] [review]
gfx, add a few asserts and stop using float/double comparisons in gfxFontStyle::Equals

This is a safer fix that copes with the values being NaN.
(I think we should still do the clamping in RestrictValue in the
style system patch.)
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-16 13:52:17 PDT
The discussion of what invariants we should have in layout around infinites and NaNs should probably move to dev.platform.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-10-16 14:04:18 PDT
Comment on attachment 671680 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

>+  return aValue == aValue ? aValue :
>+    (std::signbit(aValue) ? -std::numeric_limits<float>::max() :
>+                             std::numeric_limits<float>::max());

Is the equality test a guaranteed way to detect NaN?  (Is there a clearer way?)

Also, maybe use std::numeric_limits<T>::lowest() instead of -std::numeric_limits<T>::max() ?

r=dbaron
Comment 19 Mats Palmgren (:mats) 2012-10-16 17:40:31 PDT
(In reply to David Baron [:dbaron] from comment #18)
> >+  return aValue == aValue ? aValue :
> >+    (std::signbit(aValue) ? -std::numeric_limits<float>::max() :
> >+                             std::numeric_limits<float>::max());
> 
> Is the equality test a guaranteed way to detect NaN?

I think so, yes.

Fwiw, "x == x is false if the value of x is NaN."
http://www.gnu.org/software/libc/manual/html_node/Infinity-and-NaN.html

"The equality and inequality predicates are non-signaling so x = x returning false can be used to test if x is a quiet NaN."
http://en.wikipedia.org/wiki/NaN

OTOH, some older compilers are apparently buggy when using certain
optimization flags, e.g. g++ 4.4.1 when using --fast-math:
http://stackoverflow.com/a/5441355

> (Is there a clearer way?)

Given the above, I thought I'll use std::isnan(x) instead...
then I found this:
http://mxr.mozilla.org/mozilla-central/source/mfbt/FloatingPoint.h#16

So, I'll the MOZ_DOUBLE_IS_NaN defined there instead.
That relies on converting float(NaN) to a double results in double(NaN)...
but it seems to work for all the platforms/compilers I tested.

That file also says that NaN does not have a sign:
http://mxr.mozilla.org/mozilla-central/source/mfbt/FloatingPoint.h#128

> Also, maybe use std::numeric_limits<T>::lowest() instead of
> -std::numeric_limits<T>::max() ?

This is moot if NaN doesn't have a sign.  I'll just use max().

But, for the record, it seems std::lowest is experimental in the
g++ 4.6.3 header file I checked:

#ifdef __GXX_EXPERIMENTAL_CXX0X__
      static constexpr float 
      lowest() throw() { return -__FLT_MAX__; }
#endif
Comment 20 Mats Palmgren (:mats) 2012-10-16 18:34:59 PDT
Created attachment 672122 [details] [diff] [review]
gfx, add a few asserts and stop using float/double comparisons in gfxFontStyle::Equals

Just changed "x==x" in the assertions to "!MOZ_DOUBLE_IS_NaN(x)"
Comment 21 Mats Palmgren (:mats) 2012-10-16 18:42:22 PDT
Created attachment 672124 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

Changed "x==x" in the assertions to "!MOZ_DOUBLE_IS_NaN(x)".
Renamed the template function to EnsureNotNan and changed it to:

  return NS_LIKELY(!MOZ_DOUBLE_IS_NaN(aValue)) ? aValue :
    std::numeric_limits<T>::max();
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-10-16 20:22:36 PDT
Comment on attachment 672124 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

I guess I don't see why we'd want std::numeric_limits<T>::max() rather than 0.  Why not just use 0?

But r=dbaron either way, I guess
Comment 23 David Bolter [:davidb] 2012-10-18 13:12:20 PDT
Thanks guys. Please request sec-approval on any good-to-go patch.

Does this bug affect anything prior to 18.
Comment 24 Mats Palmgren (:mats) 2012-10-18 15:45:16 PDT
Created attachment 672986 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

Zero is probably better, yes.
Comment 25 Mats Palmgren (:mats) 2012-10-18 15:55:12 PDT
Comment on attachment 672986 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Probably not very hard to figure it out.  The patches reveals it has 
to do with fonts, and NaNs in the style system.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The added comment in gfxFontCache::AddNew is somewhat leading.

Which older supported branches are affected by this flaw?
I'm guessing all, but I haven't checked.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patches probably applies as is, or with minor adjustments.

How likely is this patch to cause regressions; how much testing does it need?
Very low risk.
Comment 27 Daniel Veditz [:dveditz] 2012-10-19 19:39:56 PDT
Comment on attachment 672986 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

sec-approval+
Comment 30 Mats Palmgren (:mats) 2012-10-23 14:47:04 PDT
Comment on attachment 672986 [details] [diff] [review]
style system, more asserts and clamp NaNs in RestrictValue

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: sec-critical crash
Testing completed (on m-c, etc.): on m-c since 2012-10-22
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none

Request is for both patches.  See also comment 25 for additional details.
Comment 33 Mats Palmgren (:mats) 2012-10-25 19:00:02 PDT
Created attachment 675397 [details] [diff] [review]
Import a few mfbt/ files from trunk (verbatim)

The gfx/ and layout/ patches applies as is, with some fuzz.
However, they do require mfbt/FloatingPoint.h which doesn't exist
in esr10.  This patch adds verbatim copies of mfbt/FloatingPoint.h,
mfbt/MSStdInt.h, mfbt/StandardInteger.h
Comment 34 Mats Palmgren (:mats) 2012-10-25 19:09:24 PDT
Created attachment 675402 [details] [diff] [review]
Some minor changes to mfbt/FloatingPoint.h to adapt it to esr10.

FloatingPoint.h use MOZ_ALWAYS_INLINE and MOZ_ASSERT which on
trunk lives in Assertions.h, Attributes.h and I didn't import
these files because they conflict with Util.h (on esr10).
So I changed it to #include Util.h instead, and removed the
message in one MOZ_ASSERT since we only have the one-argument
version on esr10.  MOZ_STATIC_ASSERT doesn't exist on esr10,
so I just "#if 0"-ed those out - I guess I could change them
to PR_STATIC_ASSERT and #include some nsprpub header here, but
it doesn't seem worth it...
Comment 35 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-29 13:11:26 PDT
Comment on attachment 675397 [details] [diff] [review]
Import a few mfbt/ files from trunk (verbatim)

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

StandardInteger.h relies on some configure and js/src/configure checks to work, but really only for the JSAPI embedding case.  So in theory this could work.  But if you're adding this header, you'd also need to update toolkit/content/license.xhtml or whatever as well.

Rather than go through all this hassle, is there any reason you can't just do s/_t// and s/int_fast16_t/int/ on the file?  I think that would remove all the <stdint.h> dependencies in the file.
Comment 36 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-29 13:12:32 PDT
Comment on attachment 675402 [details] [diff] [review]
Some minor changes to mfbt/FloatingPoint.h to adapt it to esr10.

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

Yeah, the static-asserts are more due-diligence here than anything, it should be fine not having them.
Comment 37 Mats Palmgren (:mats) 2012-10-29 15:37:52 PDT
Created attachment 676363 [details] [diff] [review]
Import mfbt/FloatingPoint.h from trunk (verbatim)
Comment 38 Mats Palmgren (:mats) 2012-10-29 15:39:45 PDT
Created attachment 676364 [details] [diff] [review]
Some minor changes to mfbt/FloatingPoint.h to adapt it to esr10.

Updated as you suggested, thanks!

(it compiles on Linux, but I haven't tried other platforms)
Comment 39 Mats Palmgren (:mats) 2012-10-29 16:22:43 PDT
Comment on attachment 676364 [details] [diff] [review]
Some minor changes to mfbt/FloatingPoint.h to adapt it to esr10.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: sec-critical crash
Testing completed (on m-c, etc.): on m-c since 2012-10-22
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none

Request is for all 4 patches.  See also comment 25 for additional details.
Comment 41 Mats Palmgren (:mats) 2013-05-13 07:33:36 PDT
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3971148e35b2
Comment 42 Ryan VanderMeulen [:RyanVM] 2013-05-13 13:45:00 PDT
https://hg.mozilla.org/mozilla-central/rev/3971148e35b2
Comment 43 Tracy Walker [:tracy] 2014-01-10 10:42:04 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.