Closed Bug 798853 (CVE-2012-4216) Opened 12 years ago Closed 12 years ago

Heap-use-after-free in gfxFont::GetFontEntry

Categories

(Core :: Graphics: Text, defect)

Product:
Core
Component:
Graphics: Text
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox17 + fixed
firefox18 + fixed
firefox19 + fixed
firefox-esr10 17+ fixed

People

(Reporter: inferno, Assigned: MatsPalmgren_bugz)

References

Details

(5 keywords, Whiteboard: [asan][adv-track-main17+][adv-track-esr17+])

Attachments

(7 files, 8 obsolete files)

Testcase
289 bytes, text/html
Details
gfxFontCache::AddNew stack with sizeAdjust==NaN
8.49 KB, text/html
Details
stack
3.66 KB, text/plain
Details
gfx, add a few asserts and stop using float/double comparisons in gfxFontStyle::Equals
4.22 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
style system, more asserts and clamp NaNs in RestrictValue
6.53 KB, patch
MatsPalmgren_bugz
: review+
lsblakk
: approval-mozilla-aurora+
lsblakk
: approval-mozilla-beta+
dveditz
: sec-approval+
Details | Diff | Splinter Review
Import mfbt/FloatingPoint.h from trunk (verbatim)
8.31 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Some minor changes to mfbt/FloatingPoint.h to adapt it to esr10.
6.33 KB, patch
Waldo
: review+
akeybl
: approval-mozilla-esr10+
Details | Diff | Splinter Review
Attached file 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
Assignee: nobody → matspal
Component: General → Layout
Product: Firefox → Core
Severity: normal → critical
Component: Layout → Graphics: Text
Flags: in-testsuite?
Keywords: reproducible, testcase
Hardware: x86_64 → All
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
Attached patch fix (obsolete) — — Splinter Review
Map NaN to std::numeric_limits::max() with the same sign.
Attachment #671165 - Flags: review?(roc)
Attached patch fix, part 2 (obsolete) — — Splinter Review
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?
Attachment #671166 - Flags: review?(roc)
Whiteboard: [asan]
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.
Attachment #671165 - Flags: review+ → review-
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.
Attached file 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()"
Attached patch gfx, add a few asserts (obsolete) — — Splinter Review
I feel slightly uncomfortable to just leave behind a few debug assertions rather than handling NaNs here.
Attachment #671165 - Attachment is obsolete: true
Attachment #671166 - Attachment is obsolete: true
Attachment #671166 - Flags: review?(roc)
Attachment #671676 - Flags: review?(roc)
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.
Attachment #671680 - Flags: review?(roc)
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.
Also, you'll need review from dbaron.
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.
(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 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
Attachment #671680 - Flags: review?(roc) → review+
(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.
(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!
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.)
Attachment #671676 - Attachment is obsolete: true
Attachment #671871 - Flags: review?(roc)
The discussion of what invariants we should have in layout around infinites and NaNs should probably move to dev.platform.
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
Attachment #671680 - Flags: review?(dbaron) → review+
(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
Just changed "x==x" in the assertions to "!MOZ_DOUBLE_IS_NaN(x)"
Attachment #671871 - Attachment is obsolete: true
Attachment #672122 - Flags: review+
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();
Attachment #671680 - Attachment is obsolete: true
Attachment #672124 - Flags: review?(dbaron)
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
Attachment #672124 - Flags: review?(dbaron) → review+
Thanks guys. Please request sec-approval on any good-to-go patch. Does this bug affect anything prior to 18.
Zero is probably better, yes.
Attachment #672124 - Attachment is obsolete: true
Attachment #672986 - Flags: review+
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.
Attachment #672986 - Flags: sec-approval?
Comment on attachment 672986 [details] [diff] [review] style system, more asserts and clamp NaNs in RestrictValue sec-approval+
Attachment #672986 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/e23d2e5ad482 https://hg.mozilla.org/mozilla-central/rev/91fce6c467dc
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox19: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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.
Attachment #672986 - Flags: approval-mozilla-beta?
Attachment #672986 - Flags: approval-mozilla-aurora?
Attachment #672986 - Flags: approval-mozilla-beta?
Attachment #672986 - Flags: approval-mozilla-beta+
Attachment #672986 - Flags: approval-mozilla-aurora?
Attachment #672986 - Flags: approval-mozilla-aurora+
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
Attachment #675397 - Flags: review?(jwalden+bmo)
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...
Attachment #675402 - Flags: review?(jwalden+bmo)
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.
Attachment #675397 - Flags: review?(jwalden+bmo) → review-
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.
Attachment #675402 - Flags: review?(jwalden+bmo) → review+
Attachment #675397 - Attachment is obsolete: true
Attachment #676363 - Flags: review?(jwalden+bmo)
Updated as you suggested, thanks! (it compiles on Linux, but I haven't tried other platforms)
Attachment #675402 - Attachment is obsolete: true
Attachment #676364 - Flags: review?(jwalden+bmo)
Attachment #676363 - Flags: review?(jwalden+bmo) → review+
Attachment #676364 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #676364 - Flags: approval-mozilla-esr10?
Attachment #676364 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Whiteboard: [asan] → [asan][adv-track-main17+][adv-track-esr17+]
Keywords: verifyme
Alias: CVE-2012-4216
Flags: sec-bounty+
Depends on: 822877
Group: core-security
Crash test: https://hg.mozilla.org/integration/mozilla-inbound/rev/3971148e35b2
Flags: in-testsuite? → in-testsuite+
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: