Bug 872981 - Print a warning whenever something attempts to store more than 4kb of preferences. r=bsmedberg

modules/libpref/src/nsPrefBranch.cpp

@@ -29,8 +29,12 @@
 #include "nsICrashReporter.h"
 #endif
 
+#include "nsIConsoleService.h"
+
 // 1 MB should be enough for everyone.
 static const uint32_t MAX_PREF_LENGTH = 1 * 1024 * 1024;
+// Actually, 4kb should be enough for everyone.
+static const uint32_t MAX_ADVISABLE_PREF_LENGTH = 4 * 1024;
 
 // Definitions
 struct EnumerateData {
@@ -168,6 +172,16 @@ NS_IMETHODIMP nsPrefBranch::GetCharPref(const char *aPrefName, char **_retval)
 }
 
 NS_IMETHODIMP nsPrefBranch::SetCharPref(const char *aPrefName, const char *aValue)
+{
+  nsresult rv = CheckSanityOfStringLength(aPrefName, aValue);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  return SetCharPrefInternal(aPrefName, aValue);
+}
+
+nsresult nsPrefBranch::SetCharPrefInternal(const char *aPrefName, const char *aValue)
+
 {
   if (GetContentChild()) {
     NS_ERROR("cannot set pref from content process");
@@ -342,6 +356,37 @@ NS_IMETHODIMP nsPrefBranch::GetComplexValue(const char *aPrefName, const nsIID &
   return NS_NOINTERFACE;
 }
 
+nsresult nsPrefBranch::CheckSanityOfStringLength(const char* aPrefName, const char* aValue) {
+  return CheckSanityOfStringLength(aPrefName, strlen(aValue));
+}
+
+nsresult nsPrefBranch::CheckSanityOfStringLength(const char* aPrefName, const nsAString& aValue) {
+  return CheckSanityOfStringLength(aPrefName, aValue.Length());
+}
+
+nsresult nsPrefBranch::CheckSanityOfStringLength(const char* aPrefName, const uint32_t aLength) {
+  if (aLength > MAX_PREF_LENGTH) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+  if (aLength <= MAX_ADVISABLE_PREF_LENGTH) {
+    return NS_OK;
+  }
+  nsresult rv;
+  nsCOMPtr<nsIConsoleService> console = do_GetService("@mozilla.org/consoleservice;1", &rv);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  nsAutoCString message(nsPrintfCString("Warning: attempting to write %d bytes to preference %s. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file.",
+                                        aLength,
+                                        aPrefName));
+  rv = console->LogStringMessage(NS_ConvertUTF8toUTF16(message).get());
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  return NS_OK;
+}
+
+
 NS_IMETHODIMP nsPrefBranch::SetComplexValue(const char *aPrefName, const nsIID & aType, nsISupports *aValue)
 {
   if (GetContentChild()) {
@@ -362,7 +407,7 @@ NS_IMETHODIMP nsPrefBranch::SetComplexValue(const char *aPrefName, const nsIID &
 
     rv = file->GetPersistentDescriptor(descriptorString);
     if (NS_SUCCEEDED(rv)) {
-      rv = SetCharPref(aPrefName, descriptorString.get());
+      rv = SetCharPrefInternal(aPrefName, descriptorString.get());
     }
     return rv;
   }
@@ -371,7 +416,7 @@ NS_IMETHODIMP nsPrefBranch::SetComplexValue(const char *aPrefName, const nsIID &
     nsCOMPtr<nsIRelativeFilePref> relFilePref = do_QueryInterface(aValue);
     if (!relFilePref)
       return NS_NOINTERFACE;
-    
+
     nsCOMPtr<nsIFile> file;
     relFilePref->GetFile(getter_AddRefs(file));
     if (!file)
@@ -391,13 +436,13 @@ NS_IMETHODIMP nsPrefBranch::SetComplexValue(const char *aPrefName, const nsIID &
     rv = file->GetRelativeDescriptor(relativeToFile, relDescriptor);
     if (NS_FAILED(rv))
       return rv;
-    
+
     nsAutoCString descriptorString;
     descriptorString.Append('[');
     descriptorString.Append(relativeToKey);
     descriptorString.Append(']');
     descriptorString.Append(relDescriptor);
-    return SetCharPref(aPrefName, descriptorString.get());
+    return SetCharPrefInternal(aPrefName, descriptorString.get());
   }
 
   if (aType.Equals(NS_GET_IID(nsISupportsString))) {
@@ -408,10 +453,12 @@ NS_IMETHODIMP nsPrefBranch::SetComplexValue(const char *aPrefName, const nsIID &
 
       rv = theString->GetData(wideString);
       if (NS_SUCCEEDED(rv)) {
-        if (wideString.Length() > MAX_PREF_LENGTH) {
-          return NS_ERROR_OUT_OF_MEMORY;
+        // Check sanity of string length before any lengthy conversion
+        rv = CheckSanityOfStringLength(aPrefName, wideString);
+        if (NS_FAILED(rv)) {
+          return rv;
         }
-        rv = SetCharPref(aPrefName, NS_ConvertUTF16toUTF8(wideString).get());
+        rv = SetCharPrefInternal(aPrefName, NS_ConvertUTF16toUTF8(wideString).get());
       }
     }
     return rv;
@@ -425,10 +472,12 @@ NS_IMETHODIMP nsPrefBranch::SetComplexValue(const char *aPrefName, const nsIID &
 
       rv = theString->GetData(getter_Copies(wideString));
       if (NS_SUCCEEDED(rv)) {
-        if (wideString.Length() > MAX_PREF_LENGTH) {
-          return NS_ERROR_OUT_OF_MEMORY;
+        // Check sanity of string length before any lengthy conversion
+        rv = CheckSanityOfStringLength(aPrefName, wideString);
+        if (NS_FAILED(rv)) {
+          return rv;
         }
-        rv = SetCharPref(aPrefName, NS_ConvertUTF16toUTF8(wideString).get());
+        rv = SetCharPrefInternal(aPrefName, NS_ConvertUTF16toUTF8(wideString).get());
       }
     }
     return rv;

modules/libpref/src/nsPrefBranch.h

@@ -194,6 +194,12 @@ protected:
     { }
 
   nsresult   GetDefaultFromPropertiesFile(const char *aPrefName, PRUnichar **return_buf);
+  // As SetCharPref, but without any check on the length of |aValue|
+  nsresult   SetCharPrefInternal(const char *aPrefName, const char *aValue);
+  // Reject strings that are more than 1Mb, warn if strings are more than 16kb
+  nsresult   CheckSanityOfStringLength(const char* aPrefName, const nsAString& aValue);
+  nsresult   CheckSanityOfStringLength(const char* aPrefName, const char* aValue);
+  nsresult   CheckSanityOfStringLength(const char* aPrefName, const uint32_t aLength);
   void RemoveExpiredCallback(PrefCallback *aCallback);
   const char *getPrefName(const char *aPrefName);
   void       freeObserverList(void);

modules/libpref/test/unit/head_libPrefs.js

@@ -7,6 +7,7 @@ const NS_APP_USER_PROFILE_50_DIR = "ProfD";
 const Ci = Components.interfaces;
 const Cc = Components.classes;
 const Cr = Components.results;
+const Cu = Components.utils;
 
 function do_check_throws(f, result, stack)
 {

modules/libpref/test/unit/test_warnings.js

@@ -0,0 +1,89 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
+
+let cs = Cc["@mozilla.org/consoleservice;1"].
+  getService(Ci.nsIConsoleService);
+let ps = Cc["@mozilla.org/preferences-service;1"].
+  getService(Ci.nsIPrefService);
+
+function makeBuffer(length) {
+  let string = "x";
+  while (string.length < length) {
+    string = string + string;
+  }
+  if (string.length > length) {
+    string = string.substring(length - string.length);
+  }
+  return string;
+}
+
+/**
+ * @resolves |true| if execution proceeded without warning,
+ * |false| if there was a warning.
+ */
+function checkWarning(pref, buffer) {
+  let deferred = Promise.defer();
+  let complete = false;
+  let listener = {
+    observe: function(event) {
+      let message = event.message;
+      if (!(message.startsWith("Warning: attempting to write")
+            && message.contains(pref))) {
+        return;
+      }
+      if (complete) {
+        return;
+      }
+      complete = true;
+      do_print("Warning while setting " + pref);
+      cs.unregisterListener(listener);
+      deferred.resolve(true);
+    }
+  };
+  do_timeout(1000, function() {
+    if (complete) {
+      return;
+    }
+    complete = true;
+    do_print("No warning while setting " + pref);
+    cs.unregisterListener(listener);
+    deferred.resolve(false);
+  });
+  cs.registerListener(listener);
+  ps.setCharPref(pref, buffer);
+  return deferred.promise;
+}
+
+function run_test() {
+  run_next_test();
+}
+
+add_task(function() {
+  // Very large change, should fail
+  try {
+    ps.setCharPref("string.fail", makeBuffer(16 * 1024 * 1024));
+    do_print("Writing to string.fail should have failed");
+    do_check_true(false); // We should have failed
+  } catch (ex if ex.result == Cr.NS_ERROR_OUT_OF_MEMORY) {
+    do_print("Writing to string.fail failed for the right reasons");
+    do_check_true(true); // Failed for the right reason
+  } catch (ex) {
+    do_print("Writing to string.fail failed for bad reasons");
+    do_check_true(false); // Failed for the wrong reason
+  }
+
+  // Simple change, shouldn't cause a warning
+  do_print("Checking that a simple change doesn't cause a warning");
+  let buf = makeBuffer(100);
+  let warned = yield checkWarning("string.accept", buf);
+  do_check_false(warned);
+
+  // Large change, should cause a warning
+  do_print("Checking that a large change causes a warning");
+  buf = makeBuffer(32 * 1024);
+  warned = yield checkWarning("string.warn", buf);
+  do_check_true(warned);
+});

modules/libpref/test/unit/xpcshell.ini

@@ -2,6 +2,7 @@
 head = head_libPrefs.js
 tail = 
 
+[test_warnings.js]
 [test_bug345529.js]
 [test_bug506224.js]
 [test_bug577950.js]