Closed Bug 1386915 Opened 2 years ago Closed 2 years ago

stylo: AddressSanitizer: attempting double-free [@ gtk_css_node_ensure_style.part.0]

Categories

(Core :: CSS Parsing and Computation, defect, P1, critical)

Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- disabled
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: truber, Assigned: manishearth)

References

(Blocks 2 open bugs)

Details

(5 keywords)

Attachments

(4 files, 2 obsolete files)

Attached file testcase.html
Attached testcase causes various crashes in m-c rev 52285ea5e54c with stylo enabled by pref. Other signatures below. This doesn't repro in gecko. It happens consistently with e10s disabled, and with e10s it manifests at shutdown.

Gtk version is 3.22.17

==26544==AddressSanitizer: while reporting a bug found another one. Ignoring.
==26544==ERROR: AddressSanitizer: attempting double-free on 0x6030005d2470 in thread T36 (StyleThread#2):
    #0 0x4bb6cb in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:47:3
    #1 0x7fe527d544ab in gtk_css_node_ensure_style.part.0 (/usr/lib/libgtk-3.so.0+0x1814ab)
    #2 0x7fe527d546d5 in gtk_css_node_get_style (/usr/lib/libgtk-3.so.0+0x1816d5)
    #3 0x7fe527eb2242 in gtk_style_context_get_property (/usr/lib/libgtk-3.so.0+0x2df242)
    #4 0x7fe527eb2407 in gtk_style_context_get_valist (/usr/lib/libgtk-3.so.0+0x2df407)
    #5 0x7fe527eb2785 in gtk_style_context_get (/usr/lib/libgtk-3.so.0+0x2df785)
    #6 0x7fe527eb5379 in gtk_style_context_get_background_color (/usr/lib/libgtk-3.so.0+0x2e2379)
    #7 0x7fe5154b19fb in nsLookAndFeel::NativeGetColor(mozilla::LookAndFeel::ColorID, unsigned int&) /home/worker/workspace/build/src/widget/gtk/nsLookAndFeel.cpp:419:9
    #8 0x7fe51541bef1 in nsXPLookAndFeel::GetColorImpl(mozilla::LookAndFeel::ColorID, bool, unsigned int&) /home/worker/workspace/build/src/widget/nsXPLookAndFeel.cpp:828:27
    #9 0x7fe5157a7643 in Gecko_GetLookAndFeelSystemColor /home/worker/workspace/build/src/layout/style/ServoBindings.cpp:877:3
    #10 0x7fe51b68a4c2 in style::properties::longhands::color::{{impl}}::to_computed_value /home/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-58e9e45a1cbf378c/out/properties.rs:18062
    #11 0x7fe51b68a4c2 in _$LT$style..values..specified..color..Color$u20$as$u20$style..values..computed..ToComputedValue$GT$::to_computed_value::h40df0ea321501a0d /home/worker/workspace/build/src/servo/components/style/values/specified/color.rs:259
    #12 0x7fe51b66c363 in style::properties::longhands::column_rule_color::cascade_property::hec284e5ba02df032 /home/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-58e9e45a1cbf378c/out/properties.rs:18901
    #13 0x7fe51b82a358 in style::properties::apply_declarations<closure,core::iter::FlatMap<style::rule_tree::SelfAndAncestors, core::iter::FilterMap<core::iter::Rev<core::slice::Iter<(style::properties::PropertyDeclaration, style::properties::declaration_block::Importance)>>, closure>, closure>> /home/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-58e9e45a1cbf378c/out/properties.rs:137730
    #14 0x7fe51b82a358 in style::properties::cascade::h1decb469c0360a21 /home/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-58e9e45a1cbf378c/out/properties.rs:137377
    #15 0x7fe51b4b784a in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::cascade_style::h239f8fc195f8dbba /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:476
    #16 0x7fe51b4b6ee9 in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_primary_style::h865e57febbb64917 /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:124
    #17 0x7fe51b4b33d7 in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_style::hf238718fcb092dbe /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:144
    #18 0x7fe51b4ad022 in style::style_resolver::{{impl}}::resolve_style_with_default_parents::{{closure}}<style::gecko::wrapper::GeckoElement> /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:178
    #19 0x7fe51b4ad022 in style::style_resolver::with_default_parent_styles<style::gecko::wrapper::GeckoElement,closure,style::data::ElementStyles> /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:66
    #20 0x7fe51b4ad022 in style::style_resolver::{{impl}}::resolve_style_with_default_parents<style::gecko::wrapper::GeckoElement> /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:177
    #21 0x7fe51b4ad022 in style::traversal::compute_style::h00a4eb976c467bf8 /home/worker/workspace/build/src/servo/components/style/traversal.rs:684
    #22 0x7fe51b4c8e00 in style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,closure> /home/worker/workspace/build/src/servo/components/style/traversal.rs:501
  

Other signatures:

**
Gtk:ERROR:gtkcssinheritvalue.c:33:gtk_css_value_inherit_free: code should not be reached
**
Gtk:ERROR:gtkcssinheritvalue.c:33:gtk_css_value_inherit_free: code should not be reached
[Exit code: -6]



==26436==ERROR: AddressSanitizer: SEGV on unknown address 0x7efc3f0000c3 (pc 0x7efce039f4c3 bp 0x6040000988d0 sp 0x7ffcdd310388 T0)
==26436==The signal is caused by a READ memory access.
    #0 0x7efce039f4c2 in _gtk_css_value_compute (/usr/lib/libgtk-3.so.0+0x19c4c2)
    #1 0x7efce03978d2 in gtk_css_static_style_compute_value (/usr/lib/libgtk-3.so.0+0x1948d2)
    #2 0x7efce038313b in _gtk_css_lookup_resolve (/usr/lib/libgtk-3.so.0+0x18013b)



==24807==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fc530a784c3 bp 0x00000000002f sp 0x7ffd3bd32c18 T0)
==24807==The signal is caused by a READ memory access.
==24807==Hint: address points to the zero page.
    #0 0x7fc530a784c2 in _gtk_css_value_compute (/usr/lib/libgtk-3.so.0+0x19c4c2)
    #1 0x7fc530a62534 in gtk_css_value_position_compute (/usr/lib/libgtk-3.so.0+0x186534)
    #2 0x7fc530a48700 in gtk_css_value_array_compute (/usr/lib/libgtk-3.so.0+0x16c700)
    #3 0x7fc530a708d2 in gtk_css_static_style_compute_value (/usr/lib/libgtk-3.so.0+0x1948d2)



==25740==ERROR: AddressSanitizer: SEGV on unknown address 0x7f1a338000e7 (pc 0x7f1a8cd77415 bp 0x61700021dd38 sp 0x7ffc434ffd18 T0)
==25740==The signal is caused by a READ memory access.
    #0 0x7f1a8cd77414 in _gtk_css_value_unref (/usr/lib/libgtk-3.so.0+0x19c414)
    #1 0x7f1a8cd6f644 in gtk_css_static_style_dispose (/usr/lib/libgtk-3.so.0+0x194644)
    #2 0x7f1a8a265a02 in g_object_unref (/usr/lib/libgobject-2.0.so.0+0x14a02)
    #3 0x7f1a8cd5baf1 in gtk_css_node_finalize (/usr/lib/libgtk-3.so.0+0x180af1)
    #4 0x7f1a8a265a71 in g_object_unref (/usr/lib/libgobject-2.0.so.0+0x14a71)
    #5 0x7f1a8ceb9b06 in gtk_style_context_finalize (/usr/lib/libgtk-3.so.0+0x2deb06)
Flags: in-testsuite?
Attached file log.txt
Flags: needinfo?(manishearth)
Priority: -- → P1
Without having had a look at the code (I'm outside now, will do later) I would guess that GTK does some nonatomic refcounting or something similar.

We could add a mutex here, which would not be great. Though IIRC system colors have a (thread safe?) cache somewhere so if we only lock in a cache miss we should be fine.
OS: Unspecified → Linux
Yep.

https://github.com/GNOME/gtk/blob/edbe6b3360b3f3c3fd73a5a13760948ef57c5737/gtk/gtkcssnode.c#L1009

http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/widget/gtk/nsLookAndFeel.cpp#1187

Fortunately, this is in the once-init code, so we won't have to deal with caching or anything.

Perhaps calling nsLookAndFeel::EnsureInit() in the pre traversal is enough? Is this call very expensive when not initialized?
Flags: needinfo?(manishearth)
Assigning to Manish.
Assignee: nobody → manishearth
We actually don't seem to be calling nsXPLookAndFeel::Init() off MT either?

http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/widget/nsXPLookAndFeel.cpp#445

That seems wrong. Perhaps it gets called earlier for whatever reason?

Fortunately there's a MOZ_RELEASE_ASSERT(IsMainThread()) there so we'd know immediately if we were calling it off MT.
Patches untested so far (waiting on a linux build), but this should likely work. Probably should r?jfkthame as well; but I'm not sure what the policy is on security bugs.

I'm calling NativeInit() from Init() directly. It seems like Init() is already always called before we run the traversal since it has a main thread release assert in it and I don't think we've ever hit that. We can also add an Init() call to nsLayoutStatics if we want but I feel it's unnecessary.

I'm not sure if there will be a major perf issue with always calling NativeInit() from Init(). On non-linux it does nothing, but on linux it does a bunch of GTK stuff and I'm unsure how expensive that is. If a page doesn't use system colors it might be too much of a cost.
Comment on attachment 8893561 [details] [diff] [review]
[PATCH 1/2] Bug 1386915 - Assert when nsLookAndFeel calls GTK off  main thread;

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

Can you move the release-assert a few lines down (after the mInitialized early-return), and turn this assertion into a debug assertion? I'd rather not do the TLS lookup on every call, that should still give us runtime safety.

r=me with that.
Attachment #8893561 - Flags: review?(bobbyholley) → review+
Comment on attachment 8893562 [details] [diff] [review]
[PATCH 2/2] Bug 1386915 - Add nsLookAndFeel::NativeInit() virtual  call for initializing native-side state;

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

(In reply to Manish Goregaokar [:manishearth] from comment #8)
> Patches untested so far (waiting on a linux build), but this should likely
> work. Probably should r?jfkthame as well; but I'm not sure what the policy
> is on security bugs.

CC him and flag him for review.

> I'm not sure if there will be a major perf issue with always calling
> NativeInit() from Init(). On non-linux it does nothing, but on linux it does
> a bunch of GTK stuff and I'm unsure how expensive that is. If a page doesn't
> use system colors it might be too much of a cost.

I don't understand. Init is only called once for the entire runtime, right?
Attachment #8893562 - Flags: review?(bobbyholley) → review+
Attachment #8893562 - Flags: review?(jfkthame)
> I don't understand. Init is only called once for the entire runtime, right?


Yes. I'm wondering if there are cases where we *avoid* NativeInit() (well, EnsureInit()) entirely. It only gets called for a certain set of system colors. If the GTK code is super expensive this might be a problem. I doubt it though.
Confirmed on a linux build that we're hitting EnsureInit() off main thread.
Patches work; can't repro the crash anymore.
(In reply to Manish Goregaokar [:manishearth] from comment #8)

> It seems like Init() is
> already always called before we run the traversal since it has a main thread
> release assert in it and I don't think we've ever hit that

Is there anything that actually guarantees this? It's not immediately clear to me that we can rely on it. AFAICS, the initialization in Init() happens whenever the first call to one of the nsXPLookAndFeel::Get{Color,Int,Float}Impl methods happens, but I'm not sure what assurances we have that this will always be before the first stylo traversal.

Can we call nsLookAndFeel::GetInstance()->Init() (from the main thread) immediately before stylo traversal starts, to make really sure it has been done?

Wait, though... what about the case where nsLookAndFeel::Refresh() gets called (which will happen if there's a theme change while the browser is running)? This will clear the mInitialized flag in the gtk implementation, which means there'll once again (IIUC) be a risk of calling EnsureInit() unsafely during style traversal.

So maybe we should call NativeInit() (not Init(), which will trivially exit if called a second time) before traversal, as we can't trust that the one-time initialization done via Init() hasn't been undone since?
> Is there anything that actually guarantees this?


No, aside from the fact that we've *never* hit that release assert.



> before traversal, as we can't trust that the one-time initialization done via Init() hasn't been undone since?

works for me. Should I also keep the call around in Init()?
Comment on attachment 8894013 [details] [diff] [review]
[PATCH 2/2] Bug 1386915 - Add nsLookAndFeel::NativeInit() virtual  call for initializing native-side state;

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

::: widget/nsXPLookAndFeel.cpp
@@ +490,5 @@
>      cc->LookAndFeelCache().Clear();
>    }
> +
> +  // GTK has some extra initialization code
> +  NativeInit();

I'd be inclined to drop this now, as the call in ServoStyleSet::PreTraverseSync() should make it unnecessary.
Attachment #8894013 - Flags: review?(jfkthame) → review+
Comment on attachment 8894013 [details] [diff] [review]
[PATCH 2/2] Bug 1386915 - Add nsLookAndFeel::NativeInit() virtual  call for initializing native-side state;

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

::: widget/LookAndFeel.h
@@ +619,5 @@
>    static void Refresh();
>  
>    /**
> +   * GTK's initialization code can't be run off main thread, call this
> +   * if you plan on using LookAndFeel off main thread later.

Actually, please expand on this comment a bit. Note that this initialization may get reset (due to a theme change during the session), so it is not sufficient to call it once during startup; it must be called again prior to each off-main-thread use of LookAndFeel, if there has been any chance that LookAndFeel::Refresh() may have happened.
Should we uplift these patches to beta?
Flags: needinfo?(bobbyholley)
https://hg.mozilla.org/mozilla-central/rev/f2a82baba93a
https://hg.mozilla.org/mozilla-central/rev/813810854eea
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(In reply to Manish Goregaokar [:manishearth] from comment #20)
> Should we uplift these patches to beta?

Yes, given that we're using pref experiments to pref stylo on for a sizeable portion of our beta audience, uplifting sec fixes is important. This change is linux-only, which reduces risk.
Flags: needinfo?(bobbyholley)
Comment on attachment 8894013 [details] [diff] [review]
[PATCH 2/2] Bug 1386915 - Add nsLookAndFeel::NativeInit() virtual  call for initializing native-side state;

Approval Request Comment

Please uplift the actual patch landed on m-c at https://hg.mozilla.org/mozilla-central/rev/813810854eea , not this patch

[Feature/Bug causing the regression]: N/A
[User impact if declined]: Potential security issue for those in the stylo nightly experiment
[Is this code covered by automated tests?]: Assertion from the previous patch will cause deterministic test failures all over
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: The other patch on this bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: Calls an initialization routine more often
[String changes made/needed]: N/A
Attachment #8894013 - Flags: approval-mozilla-beta?
Comment on attachment 8893567 [details] [diff] [review]
[PATCH 1/2] Bug 1386915 - Assert when nsLookAndFeel calls GTK off  main thread

Approval Request Comment

[See previous comment]
Attachment #8893567 - Flags: approval-mozilla-beta?
Group: dom-core-security → core-security-release
Can you request sec-approval please?
Flags: needinfo?(manishearth)
Comment on attachment 8894013 [details] [diff] [review]
[PATCH 2/2] Bug 1386915 - Add nsLookAndFeel::NativeInit() virtual  call for initializing native-side state;

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

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?

How likely is this patch to cause regressions; how much testing does it need?
Flags: needinfo?(manishearth)
Attachment #8894013 - Flags: sec-approval?
Comment on attachment 8894013 [details] [diff] [review]
[PATCH 2/2] Bug 1386915 - Add nsLookAndFeel::NativeInit() virtual  call for initializing native-side state;

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily as far as I can tell.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
They paint a bulls eye on the potential UAF but not on any easy way to exploit this.

Which older supported branches are affected by this flaw?
beta.

If not all supported branches, which bug introduced the flaw?
Introduced a while back, but we have not been shipping the ability to pref stylo since beta.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The patches landed on m-c ought to apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.
To be clear, this has already landed on m-c.
(In reply to Manish Goregaokar [:manishearth] from comment #28)
> To be clear, this has already landed on m-c.

Why did this land without sec-approval?

I'll approve now but only because it makes no difference except for the information in the request. Normally, given that we're releasing tomorrow, this would be approved to land for a few weeks.
Attachment #8894013 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #29)
> Why did this land without sec-approval?

I think this patch doesn't need sec-approval, strictly speaking. My understanding is that it is Stylo-only, and Stylo is currently disabled on beta. This just needs to be uplifted because they want to enable it (for some users) on beta. (Of course, once the experiment for beta is enabled, Stylo patches will need sec-approval.)
Ah, yes, that is correct. Stylo is pref'd off so this could technically land on trunk without issues. Good point, Andrew.
We should probably fix this crash before starting the Stylo experiment on Beta 56.
Comment on attachment 8893567 [details] [diff] [review]
[PATCH 1/2] Bug 1386915 - Assert when nsLookAndFeel calls GTK off  main thread

Avoid a crash once we turn on stylo for 56
Attachment #8893567 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8894013 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Manish Goregaokar [:manishearth] from comment #23)
> [Is this code covered by automated tests?]: Assertion from the previous
> patch will cause deterministic test failures all over
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Manish's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Group: core-security-release
See Also: → 1403690
You need to log in before you can comment on or make changes to this bug.