1354613 - Registering multiple observers for a non-root pref branch is probably broken

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Eric Rahm [:erahm]]

When you register a pref observer [1] for a non-root-pref-branch we calculate the full pref name [2]. Unfortunately when we do this we append the observer prefix to the pref branch name. So each time you add an observer the pref name grows, and is generally incorrect.

Imagine you have prefBranch(foo), you register for "bar" now we have "foo.bar". Alright now imagine you register for "baz". Now you have "foo.barbaz". This is referred to as the "domain" in pref lingo.

Alright now in pref_DoCallback we check if the changed pref prefix matches the domain [4], this will probably never match for any observers after the first one.

I'll follow up with a gtest.

[1] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/modules/libpref/nsPrefBranch.cpp#706
[2] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/modules/libpref/nsPrefBranch.cpp#741
[3] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/modules/libpref/nsPrefBranch.cpp#871-883
[4] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/modules/libpref/prefapi.cpp#993-1002

Comment 1

[Eric Rahm [:erahm]]

It also turns out test_libPrefs.js was implemented incorrectly and didn't actually test observing non-root pref branches.

Comment 2

[Eric Rahm [:erahm]]

Okay I missed where we truncate the branch name back to it's original size [1], so the values are fine (I've added a test). Where things get tricky are if we allow setting prefs off main thread (I think we do) then you can get into a racy state. 

Additionally in CollectReports we loop over prefs storing the name in a non-owning nsDependentCString [2] which could lead to invalid strings as well.

[1] http://searchfox.org/mozilla-central/source/modules/libpref/nsPrefBranch.cpp#880
[2] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/modules/libpref/Preferences.cpp#355

Comment 3

[Benjamin Smedberg]

The pref service is supposed to be main-thread only, both reading and writing.

Comment 4

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #2)
> Okay I missed where we truncate the branch name back to it's original size
> [1], so the values are fine (I've added a test). Where things get tricky are
> if we allow setting prefs off main thread (I think we do) then you can get
> into a racy state. 

And it turns out they're main thread only (except on B2G), the assertions are down in prefapi.cpp. It sounds like we might let stylo have off-main-thread access so perhaps we should still fix it.

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 1: Fix the observer portion of test_libPrefs.js

This updates test_libPrefs.js so that it actually runs all the observer
portion of it's tests. Previously the observer callback would get hit before
the next do_test_pending call could be run, thus causing the test to end.

An additional test was added to confirm that registering for multiple prefs on
a non-root pref branch works as intended.

Comment 6

[Eric Rahm [:erahm]]

|getFloatPref| [1] is rather busted as well for non-root pref branches, we get the full pref name, then pass that to another function that gets the full pref name. So we run into a possible UAF if the string mutates before appending or just a weird name. The function isn't widely used, but should be fixed.

[1] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/modules/libpref/nsPrefBranch.cpp#194-196

Comment 7

[Eric Rahm [:erahm]]

Attachment: Part 2: Fix GetFloatPref on non-root branches

MozReview-Commit-ID: 1NLBQQ40sae

Comment 8

[Eric Rahm [:erahm]]

Attachment: Stop mutating the pref branch name when retrieving it

This adds a PrefName wrapper that allows use to avoid copying the pref name
string in the fast root pref branch case, but also allows to create a new
string in the slower non-root pref branch case. By creating a new string we
avoid the odd behavior of mutating the pref branch name with each retrieval.

Nathan, this does some trickery with Variants and move operators that I'm not
super familiar with so feedback there would be helpful. Also some confirmation
that this should be reasonably fast would be great.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8855972 [details] [diff] [review]
Stop mutating the pref branch name when retrieving it

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

This seems like a decent cleanup.  r=me with some discussion below.

::: modules/libpref/nsPrefBranch.cpp
@@ +875,2 @@
>  
> +  return PrefName(mPrefRoot + nsDependentCString(aPrefName));

This will allocate a new string every time, in contrast to the old code, which would allocate up to the longest string requested.  So that's a minus.

But given that nsPrefBranch is basically a JS-only API, I'm inclined to guess that the small inefficiency here doesn't matter very much in the grand scheme of things.

::: modules/libpref/nsPrefBranch.h
@@ +192,2 @@
>  
> +  int32_t GetRootLength() { return mPrefRoot.Length(); }

This can be a `const` method.

@@ +210,5 @@
> +    typedef mozilla::Variant<const char*, const nsCString> StorageType;
> +
> +    explicit PrefName(const char* aName) : mValue(aName) {}
> +    explicit PrefName(const nsCString& aName) : mValue(aName) {}
> +    PrefName(PrefName&& aOther) : mValue(mozilla::Forward<StorageType>(aOther.mValue)) {}

Are you defining the move constructor because the copy constructor is deleted, and the compiler complains about returning PrefName values otherwise?

Do you think it would hurt to just let these be defaulted, or are you trying to make this maximally efficient if nsCString ever grows a move constructor?

@@ +211,5 @@
> +
> +    explicit PrefName(const char* aName) : mValue(aName) {}
> +    explicit PrefName(const nsCString& aName) : mValue(aName) {}
> +    PrefName(PrefName&& aOther) : mValue(mozilla::Forward<StorageType>(aOther.mValue)) {}
> +    PrefName(const PrefName&) = delete;

If you're going to delete the copy constructor, you should delete operator=(const PrefName&) as well.

@@ +245,5 @@
>    nsresult   CheckSanityOfStringLength(const char* aPrefName, const nsACString& aValue);
>    nsresult   CheckSanityOfStringLength(const char* aPrefName, const char* aValue);
>    nsresult   CheckSanityOfStringLength(const char* aPrefName, const uint32_t aLength);
>    void RemoveExpiredCallback(PrefCallback *aCallback);
> +  PrefName getPrefName(const char *aPrefName);

This can be a `const` method now too.

Comment 10

[Eric Rahm [:erahm]]

Attachment: Part 3: Stop mutating the pref branch name when retrieving it

Updated per review, should probably take a second look at the header. I switched to deriving from Variant and using the |match| function.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8856775 [details] [diff] [review]
Part 3: Stop mutating the pref branch name when retrieving it

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

Thanks.

Comment 12

[Benjamin Smedberg]

Comment on attachment 8855947 [details] [diff] [review]
Part 2: Fix GetFloatPref on non-root branches

wow

Comment 13

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f256046af6059da5ae9a8e3e23426944f40975e1
Bug 1354613 - Part 1: Fix the observer portion of test_libPrefs.js. r=bsmedberg

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6ed9355ff926f4f56506186bf4690f7bfb252b
Bug 1354613 - Part 2: Fix GetFloatPref on non-root branches. r=bsmedberg

https://hg.mozilla.org/integration/mozilla-inbound/rev/89aacb54a8826d09a2b362e2aa61dde08631f175
Bug 1354613 - Part 3: Stop mutating the pref branch name when retrieving it. r=froydnj

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f256046af605
https://hg.mozilla.org/mozilla-central/rev/9c6ed9355ff9
https://hg.mozilla.org/mozilla-central/rev/89aacb54a882