Bug 1418846 - Fix string value leaks in libpref. r=glandium
authorNicholas Nethercote <nnethercote@mozilla.com>
Sat, 18 Nov 2017 13:05:02 +1100
changeset 437509 2f6e831ec27afcbc922f201a7250c8f696d8c97f
parent 437508 982fdc58cd44094a87149a56c100cc894e8e7a1b
child 437510 1a42490adc0d9444c8c49f7624fa221dc43ae47c
push id117
push userfmarier@mozilla.com
push dateTue, 28 Nov 2017 20:17:16 +0000
reviewersglandium
bugs1418846
milestone59.0a1
Bug 1418846 - Fix string value leaks in libpref. r=glandium There's an "XXX" comment suggesting a possible leak when string user values are cleared. Turns out a lot of the time there won't be a leak, because the string pointer isn't overwritten and ClearEntry() frees the string when the pref is destroyed. However, if the user value is subsequently overwritten with a different string, there will be a leak. Also, even in the non-leak case, we currently hold onto the string for longer than necessary. This patch introduces ClearUserValue() -- which frees the string when appropriate -- and uses it in all the places where values are cleared. MozReview-Commit-ID: ARuWUNzPTfy
modules/libpref/Preferences.cpp
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -469,16 +469,24 @@ private:
       MOZ_ASSERT(aNewValue.mStringVal);
       value->mStringVal = moz_xstrdup(aNewValue.mStringVal);
     } else {
       *value = aNewValue;
     }
   }
 
 public:
+  void ClearUserValue()
+  {
+    if (Type() == PrefType::String) {
+      free(const_cast<char*>(mUserValue.mStringVal));
+      mUserValue.mStringVal = nullptr;
+    }
+  }
+
   void SetValue(PrefType aType,
                 PrefValue aValue,
                 uint32_t aFlags,
                 bool* aValueChanged,
                 bool* aDirty)
   {
     if (aFlags & kPrefSetDefault) {
       if (!IsLocked()) {
@@ -498,17 +506,17 @@ public:
       }
     } else {
       // If new value is same as the default value and it's not a "sticky"
       // pref, then un-set the user value. Otherwise, set the user value only
       // if it has changed.
       if (HasDefaultValue() && !IsSticky() &&
           mDefaultValue.Equals(aType, aValue) && !(aFlags & kPrefForceSet)) {
         if (HasUserValue()) {
-          // XXX should we free a user-set string value if there is one?
+          ClearUserValue();
           SetHasUserValue(false);
           if (!IsLocked()) {
             *aDirty = true;
             *aValueChanged = true;
           }
         }
       } else if (!HasUserValue() || !IsType(aType) ||
                  !mUserValue.Equals(aType, aValue)) {
@@ -659,16 +667,17 @@ static nsresult
 PREF_ClearUserPref(const char* aPrefName)
 {
   if (!gHashTable) {
     return NS_ERROR_NOT_INITIALIZED;
   }
 
   PrefHashEntry* pref = pref_HashTableLookup(aPrefName);
   if (pref && pref->HasUserValue()) {
+    pref->ClearUserValue();
     pref->SetHasUserValue(false);
 
     if (!pref->HasDefaultValue()) {
       gHashTable->RemoveEntry(pref);
     }
 
     NotifyCallbacks(aPrefName);
     Preferences::HandleDirty();
@@ -690,16 +699,17 @@ PREF_ClearAllUserPrefs()
   for (auto iter = gHashTable->Iter(); !iter.Done(); iter.Next()) {
     auto pref = static_cast<PrefHashEntry*>(iter.Get());
 
     if (pref->HasUserValue()) {
       if (!prefNames.append(pref->Name())) {
         return NS_ERROR_OUT_OF_MEMORY;
       }
 
+      pref->ClearUserValue();
       pref->SetHasUserValue(false);
       if (!pref->HasDefaultValue()) {
         iter.Remove();
       }
     }
   }
 
   for (const char* prefName : prefNames) {