From c2a9e0866133ccc4c24c0807b56b0356862b348d Mon Sep 17 00:00:00 2001 From: David Rajchenbach-Teller Date: Mon, 3 Jun 2013 15:30:57 -0400 Subject: [PATCH] Bug 872981 - Print a warning whenever something attempts to store more than 4kb of preferences. r=bsmedberg --- modules/libpref/src/nsPrefBranch.cpp | 69 ++++++++++++++--- modules/libpref/src/nsPrefBranch.h | 6 ++ modules/libpref/test/unit/head_libPrefs.js | 1 + modules/libpref/test/unit/test_warnings.js | 89 ++++++++++++++++++++++ modules/libpref/test/unit/xpcshell.ini | 1 + 5 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 modules/libpref/test/unit/test_warnings.js diff --git a/modules/libpref/src/nsPrefBranch.cpp b/modules/libpref/src/nsPrefBranch.cpp index 6514ceed04ed..298bfc908847 100644 --- a/modules/libpref/src/nsPrefBranch.cpp +++ b/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 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 relFilePref = do_QueryInterface(aValue); if (!relFilePref) return NS_NOINTERFACE; - + nsCOMPtr 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; diff --git a/modules/libpref/src/nsPrefBranch.h b/modules/libpref/src/nsPrefBranch.h index 15d72e215952..b58173611921 100644 --- a/modules/libpref/src/nsPrefBranch.h +++ b/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); diff --git a/modules/libpref/test/unit/head_libPrefs.js b/modules/libpref/test/unit/head_libPrefs.js index d8a8ccfacb5b..18a0cb6919b5 100644 --- a/modules/libpref/test/unit/head_libPrefs.js +++ b/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) { diff --git a/modules/libpref/test/unit/test_warnings.js b/modules/libpref/test/unit/test_warnings.js new file mode 100644 index 000000000000..9c0eed961a83 --- /dev/null +++ b/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); +}); diff --git a/modules/libpref/test/unit/xpcshell.ini b/modules/libpref/test/unit/xpcshell.ini index 7015a04738b1..97a3e9bd7399 100644 --- a/modules/libpref/test/unit/xpcshell.ini +++ b/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]