1848829 - Assertion failure: sInServoTraversal || NS_IsMainThread(), at /builds/worker/workspace/obj-build/dist/include/mozilla/ServoUtils.h:33

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect, P2)

Description

[Tyson Smith [:tsmith]]

Found with m-c 20230814-a5bcd3edffd7 (--enable-debug)

This was found by visiting a live website with a debug build.

STR:

• Launch browser and visit: www.tiktok.com

I can likely get a Pernosco session if needed.

Assertion failure: sInServoTraversal || NS_IsMainThread(), at /builds/worker/workspace/obj-build/dist/include/mozilla/ServoUtils.h:33

#0 0x7f572e78ec37 in IsInServoTraversal /builds/worker/workspace/obj-build/dist/include/mozilla/ServoUtils.h:33:3
#1 0x7f572e78ec37 in IsInServoTraversal /builds/worker/workspace/obj-build/dist/include/mozilla/ServoStyleSet.h:117:45
#2 0x7f572e78ec37 in pref_Lookup(char const*, bool) /builds/worker/checkouts/gecko/modules/libpref/Preferences.cpp:1789:3
#3 0x7f572e82bbab in mozilla::IsPreferenceSanitized(char const*) /builds/worker/checkouts/gecko/modules/libpref/Preferences.cpp:6198:33
#4 0x7f5731bd78e8 in webgl_override_unmasked_renderer /builds/worker/workspace/obj-build/dist/include/mozilla/StaticPrefList_webgl.h:332:1
#5 0x7f5731bd78e8 in mozilla::ClientWebGLContext::GetParameter(JSContext*, unsigned int, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, bool)::$_2::operator()[abi:cxx11]() const /builds/worker/checkouts/gecko/dom/canvas/ClientWebGLContext.cpp:2269:29
#6 0x7f5731bd5f20 in mozilla::ClientWebGLContext::GetParameter(JSContext*, unsigned int, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, bool) /builds/worker/checkouts/gecko/dom/canvas/ClientWebGLContext.cpp:2299:17
#7 0x7f57313acdd7 in mozilla::dom::WebGLRenderingContext_Binding::getParameter(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/WebGLRenderingContextBinding.cpp:18473:24
#8 0x7f5731a95048 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3327:13

Comment 1

[Timothy Nikkel (:tnikkel)]

Reproduces for me.

Comment 2

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Fuzzers did find a test case for this.

Comment 3

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230816215210-1c8dec8815d5.
The bug appears to have been introduced in the following build range:

Start: 7ca85c53a53df7d84d24662c11934a5022ad5fc7 (20230206191433)
End: f79377e1676571e1f9d5a819362fb7020c38c1be (20230206211401)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7ca85c53a53df7d84d24662c11934a5022ad5fc7&tochange=f79377e1676571e1f9d5a819362fb7020c38c1be

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1799258

:jgilbert, since you are the author of the regressor, bug 1799258, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 5

[Timothy Nikkel (:tnikkel)]

I'm pretty sure this is actually regressed by bug 1811294. I checked by flipping the prefs that it flipped back to confirm no crash (but crash without flipping them).

Comment 6

[Timothy Nikkel (:tnikkel)]

It's a string pref that doesn't have the default value specified in StaticPrefList.yaml or prefs.js, so it is determined that it needs to be sanitized.

We get the pref here

https://searchfox.org/mozilla-central/rev/146638366864f73ee8a697ea76480eb02c00eb3c/dom/canvas/ClientWebGLContext.cpp#2269

and in both place where we use it we check another pref called StaticPrefs::webgl_sanitize_unmasked_renderer

https://searchfox.org/mozilla-central/rev/146638366864f73ee8a697ea76480eb02c00eb3c/dom/canvas/ClientWebGLContext.cpp#2299
https://searchfox.org/mozilla-central/rev/146638366864f73ee8a697ea76480eb02c00eb3c/dom/canvas/ClientWebGLContext.cpp#2335

so it seems that this pref has already been considered to be sanitized when necessary. Do we just add it to the sDynamicPrefOverrideList allow list?

Comment 7

[Timothy Nikkel (:tnikkel)]

This only affects debug builds, so this feels more like an S3.

Comment 8

[Tom Ritter [:tjr]]

It's a string pref that doesn't have the default value specified in StaticPrefList.yaml or prefs.js, so it is determined that it needs to be sanitized.

This is a good inference, and it took me a few hours to disprove.

Do we just add it to the sDynamicPrefOverrideList allow list?

This won't fix it.

The problem is actually that we are on a Worker Thread, and we're crashing on this assertion because we call pref_lookup here.

My understanding is that StaticPrefs can be read off a main thread, but calling Preferences::GetString/GetFloat/etc are not allowed off main thread. This isn't documented in the API, but a few call stacks deep you'll hit the same assertion in pref_lookup.

We hit the problem here because we are accessing a pref off-main-thread (which is uncommon), and the pref is not a mirror:once pref, it is a mirror:always pref, which is also uncommon. In this scenario we wind up calling pref_Lookup which triggers the assertion.

Stylo also accesses prefs off the main thread, which is discussed in Bug 1474789 (and this comment in 1351200) - there's some contention about that behavior.

I think we can add a different codepath for prefs in StaticPrefList so that we compare their name against the blocklist, but we do not sanitize based on the heuristic. (Because the heuristic is seeing if there's a default value, and if it's a static pref, then it does have one.) If we avoid the heuristic, we should be able to avoid the call to pref_lookUp, and the thread issue.

I'm not going to be able to work on this immediately, but because this is a debug-only non-security assertion it's not super urgent.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

It's not quite non-security right? If the main thread is changing prefs you can easily trigger UAF etc. The only reason style can access prefs off the main thread is that the main thread is paused when style is running.

Comment 10

[Tom Ritter [:tjr]]

That seems correct. The relatively (?) obscure combination of mirrored, string-value, StaticPref should limit the exposure of this, but I will flag it.

Comment 11

[Andrew McCreight [:mccr8]]

Is this actually a security issue? webgl.override-unmasked-renderer isn't actually a String, it is a DataMutexString, so it seems like this was already anticipated and it should be fine?

Comment 12

[Kelsey Gilbert [:jgilbert]]

Yes, I think this is a false-positive assert that's probably just a bug in our glue between APIs.

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Tom Ritter [:tjr] from comment #8)

It's a string pref that doesn't have the default value specified in StaticPrefList.yaml or prefs.js, so it is determined that it needs to be sanitized.

This is a good inference, and it took me a few hours to disprove.

Apologies for sending you on the wrong track. I was starting to realize it was different then that but my investigation didn't get far enough to leave another comment.

Comment 14

[Tom Ritter [:tjr]]

(In reply to Timothy Nikkel (:tnikkel) from comment #13)

(In reply to Tom Ritter [:tjr] from comment #8)

It's a string pref that doesn't have the default value specified in StaticPrefList.yaml or prefs.js, so it is determined that it needs to be sanitized.

This is a good inference, and it took me a few hours to disprove.

Apologies for sending you on the wrong track. I was starting to realize it was different then that but my investigation didn't get far enough to leave another comment.

No need to apologize!

Yes, I think this is a false-positive assert that's probably just a bug in our glue between APIs.

The problem isn't really with this WebGL pref, it's with the Sanitization code using a not-threadsafe method...

That said, I am wondering if we are safe because of what we're doing here. I think the risk is calling pref_Lookup, and either (a) using that reference after another thread destroys the Pref or (b) using that reference to get a reference to an object (e.g. the UserValue) that another thread destroys before it is used. B isn't an issue, we only check the mIsSanitized value. A could (maybe?) still happen, but if it does it's just a 'Is this byte zero or non-zero?' info leak, and you'd have to be jumping through a ton of hoops to do this and that's a fairly useless leak for all the effort.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:KrisWright, could you have a look please?

For more information, please visit BugBot documentation.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

I hit this on https://lowes.com (probably an ad iframe; you have to sit there for a minute or two, generally). Caused me to go on a wild goose chase trying to figure out how my networking patch could cause this. :-)

Pernosco session: https://pernos.co/debug/qt6VzhOqFju6Ah3Dudd9Bg/index.html

Comment 17

[Nika Layzell [:nika] (ni? for response)]

I believe the assertion is happening from this explicit expansion: https://searchfox.org/mozilla-central/rev/37d9d6f0a77147292a87ab2d7f5906a62644f455/modules/libpref/init/StaticPrefListBegin.h#42-46.

This is a bit of an odd piece of code. We are defining a static preference getter for an explicitly thread-safe preference, and yet we are checking IsPreferenceSanitized, which is a non-threadsafe action. I believe that this was probably originally done because it was impossible for a string preference value to be threadsafe, and then the code was copied into the ALWAYS_DATAMUTEX_PREF code when it was added in bug 1783299 without noticing that the behaviour would now be broken.

The code should be changed to not dynamically check if the preference is sanitized from off-main-thread for string preferences guarded by a data mutex, as this is fundamentally thread-unsafe. Instead we should perhaps do 1 of 3 things (though there might be other options I didn't think of):

1. Consider changing the storage for DataMutex string preferences to contain both the pref's value and a boolean flag for whether the preference is sanitized, such that we can atomically check the sanitized status from off-main-thread when we acquire the lock. This might be difficult as it appears we expose the bare DataMutexBase::ConstAutoLock from the getter.
   • As I don't believe a VoidCString is a valid preference value, we might be able to use that string value as a sentinel for a sanitized preference to avoid changing the mutex type.
2. Require that a pref stored in a DataMutex can never be sanitized, and crash if it is detected to be sanitized in a content process. This runs the risk of crashing spuriously.
3. Give up on crashing when accessing a sanitized string preference which was blocklisted via a DataMutex. This runs the risk of missing bugs.

Comment 18

[Tom Ritter [:tjr]]

I'm testing a patch here: https://treeherder.mozilla.org/jobs?repo=try&revision=a13d644143566f66a1bbdfaa716305a91eadd939

Comment 19

[Tom Ritter [:tjr]]

Attachment: Bug 1848829: Do not check pref sanitization for Static Prefs r?nika

If IsPreferenceSanitized(char*) is called, it will call pref_Lookup
which is not thread-safe. StaticPrefs use that function to check
pref sanitization.

Prefs are sanitized if they are a no-default-value string-type preference
or their name is in sRestrictFromWebContentProcesses. The first check
is intended to catch prefs set dynamically, ones we don't know about
and may want to sanitize. But Static Prefs we know all about. So we can
omit that check for them. We only (in theory) need to check them against
the blocklist.

In practice: no Static Prefs are sanitized. If they were (and they were
not mirror-never prefs), it would have exposed the bug that makes content
processes crash on startup, because we would crash during initialization.

That initialization happens while reading the prefs from the Shared Map -
sanitized prefs can't be in the shared map. So we can take a shortcut and
instead of building in support for Static Prefs to be initialized from
the HashMap instead of the shared map (perhaps as simple as
s/GetSharedPrefValue/GetPrefValue), we can just crash if someone adds a
Static Pref to the list of things to be sanitized.

Comment 20

[Tyson Smith [:tsmith]]

This is the most frequently reported issue found by live testing.

Setting ni? so it does not get forgotten.

Comment 21

[Tom Ritter [:tjr]]

Sorry about that, I've updated it and reflagged it for review.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e186e1a4608
Do not check pref sanitization for Static Prefs r=nika

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7e186e1a4608

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:tjr, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox122 to wontfix.

For more information, please visit BugBot documentation.

Comment 25

[Tom Ritter [:tjr]]

I don't think this needs to be uplifted unless someone particularly wants to.

Comment 26

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20231219050609-ad0cefd58a2a.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.