Closed Bug 1502252 Opened Last year Closed Last year

Hit MOZ_CRASH(nsFontCache not thread-safe) at /builds/worker/workspace/build/src/xpcom/base/nsISupportsImpl.cpp:46


(Core :: Layout, defect)

Not set



Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 --- fixed
firefox65 --- fixed


(Reporter: jkratzer, Assigned: jfkthame)


(Blocks 1 open bug)


(Keywords: assertion, sec-moderate, testcase)


(2 files)

Attached file testcase.html
Testcase found while fuzzing mozilla-central rev 3cc04ee79005.

Triggering this issue appears to be a race and the attached testcase is not fully reduced and may require several attempts to trigger.

Hit MOZ_CRASH(nsFontCache not thread-safe) at /builds/worker/workspace/build/src/xpcom/base/nsISupportsImpl.cpp:46

rax = 0x0000560e4d560e80   rdx = 0x0000000000000000
rcx = 0x0000000000000b40   rbx = 0x00007f0fc278a186
rsi = 0x00007f0fcf02f8b0   rdi = 0x00007f0fcf02e680
rbp = 0x00007f0fb47fd130   rsp = 0x00007f0fb47fd120
r8 = 0x00007f0fcf02f8b0    r9 = 0x00007f0fb47ff700
r10 = 0x0000000000000056   r11 = 0x00007f0fcecc07e0
r12 = 0x00007f0fb4326000   r13 = 0x00007f0fb464e100
r14 = 0x00007f0fc1a61e20   r15 = 0x00007f0fb47fd348
rip = 0x0000560e4d323ec2
OS|Linux|0.0.0 Linux 4.15.0-36-generic #39-Ubuntu SMP Mon Sep 24 16:19:09 UTC 2018 x86_64
CPU|amd64|family 6 model 78 stepping 3|1
19|1||nsAutoOwningThread::AssertCurrentThreadOwnsMe(char const*) const||46|0x14
19|3||nsFontCache::GetMetricsFor(nsFont const&, nsFontMetrics::Params const&)||44|0x5
19|4||nsDeviceContext::GetMetricsFor(nsFont const&, nsFontMetrics::Params const&)||297|0x1c
19|5||nsLayoutUtils::GetMetricsFor(nsPresContext*, bool, nsStyleFont const*, int, bool, nsLayoutUtils::FlushUserFontSet)||10141|0x5
19|7||<style::gecko::wrapper::GeckoFontMetricsProvider as style::font_metrics::FontMetricsProvider>::query||1047|0x9
19|9||style::values::computed::length::<impl style::values::computed::ToComputedValue for style::values::specified::calc::CalcLengthOrPercentage>::to_computed_value||274|0xf
19|10||style::values::computed::length::<impl style::values::computed::ToComputedValue for style::values::specified::length::LengthOrPercentage>::to_computed_value||477|0x8
19|15||<style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_style_and_visited||305|0x2c
19|16||<style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_primary_style||215|0x17
19|17||<style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_styles_with_default_parents::{{closure}}||335|0x8
19|18||<style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_styles_with_default_parents||102|0xb
19|20||<style::gecko::traversal::RecalcStyleOnly<'recalc> as style::traversal::DomTraversal<style::gecko::wrapper::GeckoElement<'le>>>::process_preorder||435|0xb
19|21||<std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once||197|0x20
19|24||<rayon_core::job::HeapJob<BODY> as rayon_core::job::Job>::execute||289|0x5
19|29||<F as alloc::boxed::FnBox<A>>::call_box||289|0x5
Ugh, thank you.
Flags: needinfo?(emilio)
(I think this should be s-s for the time being)
Group: core-security
Group: core-security → layout-core-security
How much time does it take for you to repro? I couldn't repro on a debug build for a while. I'll leave rr's chaos mode running for a bit, but maybe I'm doing something wrong?
Flags: needinfo?(emilio) → needinfo?(jkratzer)
Flags: needinfo?(emilio)
Aand I got to repro it in a debug-opt build, nvm. Probably debug no-opt is just too slow.
Flags: needinfo?(jkratzer)
I guess this is caused by bug 1500126.
Sounds very likely...
Blocks: 1500126
Any chance you could look at this Jonathan?
Flags: needinfo?(emilio) → needinfo?(jfkthame)
Ah, so the issue is that when stylo calls GetMetricsFor, and we want to post a task to flush some old cache entries, the Runnable that we create needs to hold a reference to the nsFontCache, just in case it might otherwise get torn down (unlikely though that may be).

But nsFontCache doesn't have threadsafe refcounting, hence this assertion.

AFAICS, just switching nsFontCache from NS_DECL_ISUPPORTS to NS_DECL_THREADSAFE_ISUPPORTS should avoid the problem here.
Flags: needinfo?(jfkthame)
I wasn't able to reproduce the assertion locally (in my opt+debug build), but AFAICS this should fix things.
Attachment #9020802 - Flags: review?(emilio)
Comment on attachment 9020802 [details] [diff] [review]
Use thread-safe refcounting for nsFontCache

Review of attachment 9020802 [details] [diff] [review]:

Thanks! And sorry for throwing the ni? at you, but I imagined you had all this code paged in and you would be able to figure out earlier than I even get to take a proper look at this :)
Attachment #9020802 - Flags: review?(emilio) → review+
Marking this sec-moderate, as although it's certainly a bug, it doesn't look like there'd be any clear path to an attack.
Keywords: sec-moderate
Group: layout-core-security → core-security-release
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Jason, if you could verify whether this issue still reproduces now that the patch has landed, that would be great -- thanks! Provided it is indeed fixed, we'll want to uplift to FF64.
Assignee: nobody → jfkthame
Flags: needinfo?(jkratzer)
(In reply to Jonathan Kew (:jfkthame) from comment #13)
> Jason, if you could verify whether this issue still reproduces now that the
> patch has landed, that would be great -- thanks! Provided it is indeed
> fixed, we'll want to uplift to FF64.

I can no longer reproduce the issue using build c3f01d72374b (from TC).
Flags: needinfo?(jkratzer)
Comment on attachment 9020802 [details] [diff] [review]
Use thread-safe refcounting for nsFontCache

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: 1498316 + 1502252

User impact if declined: Possible race condition during stylo processing, which could lead to corruption or leakage of the device context's font-metrics cache.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just makes nsFontCache use thread-safe refcounting, no other change.

String changes made/needed: none
Attachment #9020802 - Flags: approval-mozilla-beta?
Comment on attachment 9020802 [details] [diff] [review]
Use thread-safe refcounting for nsFontCache

[Triage Comment]
Fixes a moderate sec issue new to Fx64. Approved for 64.0b6.
Attachment #9020802 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jonathan Kew (:jfkthame) from comment #15)
> Has the fix been verified in Nightly?: Yes
> Needs manual test from QE?: No

Setting qe- per Jonathan's assessment.
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.