Bug 526789 - UMR in nsCookieService::CountCookiesFromHostInternal, GetCookiesFromHost, and
authorDan Witte <dwitte@mozilla.com>
Fri, 13 Nov 2009 13:05:50 -0800
changeset 34835 a47840c2d74264c0a402986fd68cca8b50f5094c
parent 34834 ad0cbdbcd37aa845d8b4805db5544926b7557399
child 34836 bd37d4d849c49896b45827c2931ab545cbcd1746
push idunknown
push userunknown
push dateunknown
bugs526789
milestone1.9.3a1pre
Bug 526789 - UMR in nsCookieService::CountCookiesFromHostInternal, GetCookiesFromHost, and GetCookieInternal when given empty host string. r=biesi
extensions/cookie/test/unit/test_bug481775.js
extensions/cookie/test/unit/test_bug526789.js
netwerk/cookie/public/nsICookieManager.idl
netwerk/cookie/src/nsCookieService.cpp
--- a/extensions/cookie/test/unit/test_bug481775.js
+++ b/extensions/cookie/test/unit/test_bug481775.js
@@ -1,24 +1,22 @@
 const Cc = Components.classes;
 const Ci = Components.interfaces;
 
 function run_test() {
-  var cs = Cc["@mozilla.org/cookieService;1"].getService(Ci.nsICookieService);
   var cm = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager2);
-  var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
   var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
   var pb = null;
   try {
     pb = Cc["@mozilla.org/privatebrowsing;1"].getService(Ci.nsIPrivateBrowsingService);
   } catch (e) {}
 
   // accept all cookies and clear the table
   prefs.setIntPref("network.cookie.lifetimePolicy", 0);
-  cs.removeAll();
+  cm.removeAll();
 
   // saturate the cookie table
   addCookies(0, 5000);
 
   // check how many cookies we have
   var count = getCookieCount();
   do_check_neq(count, 0);
 
@@ -32,17 +30,17 @@ function run_test() {
 
     // saturate the cookie table again
     addCookies(5000, 5000);
 
     // check we have the same number of cookies
     do_check_eq(getCookieCount(), count);
 
     // remove them all
-    cs.removeAll();
+    cm.removeAll();
     do_check_eq(getCookieCount(), 0);
 
     // leave private browsing mode
     pb.privateBrowsingEnabled = false;
   }
 
   // make sure our cookies are back
   do_check_eq(getCookieCount(), count);
@@ -50,17 +48,17 @@ function run_test() {
   // set a few more, to trigger a purge
   addCookies(10000, 1000);
 
   // check we have the same number of cookies
   var count = getCookieCount();
   do_check_eq(getCookieCount(), count);
 
   // remove them all
-  cs.removeAll();
+  cm.removeAll();
   do_check_eq(getCookieCount(), 0);
 }
 
 function getCookieCount() {
   var count = 0;
   var cm = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager2);
   var enumerator = cm.enumerator;
   while (enumerator.hasMoreElements()) {
new file mode 100644
--- /dev/null
+++ b/extensions/cookie/test/unit/test_bug526789.js
@@ -0,0 +1,95 @@
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+const Cr = Components.results;
+
+function do_check_throws(f, result, stack)
+{
+  if (!stack)
+    stack = Components.stack.caller;
+
+  try {
+    f();
+  } catch (exc) {
+    if (exc.result == result)
+      return;
+    do_throw("expected result " + result + ", caught " + exc, stack);
+  }
+  do_throw("expected result " + result + ", none thrown", stack);
+}
+
+function run_test() {
+  var cs = Cc["@mozilla.org/cookieService;1"].getService(Ci.nsICookieService);
+  var cm = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager2);
+  var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
+
+  cm.removeAll();
+
+  // test that an empty host results in a no-op
+  var uri = ios.newURI("http://baz.com/", null, null);
+  var emptyuri = ios.newURI("http:///", null, null);
+  var doturi = ios.newURI("http://./", null, null);
+  do_check_eq(emptyuri.asciiHost, "");
+  do_check_eq(doturi.asciiHost, ".");
+  cs.setCookieString(emptyuri, null, "foo2=bar", null);
+  do_check_eq(getCookieCount(), 0);
+  cs.setCookieString(doturi, null, "foo3=bar", null);
+  do_check_eq(getCookieCount(), 0);
+  cs.setCookieString(uri, null, "foo=bar", null);
+  do_check_eq(getCookieCount(), 1);
+
+  do_check_eq(cs.getCookieString(uri, null), "foo=bar");
+  do_check_eq(cs.getCookieString(emptyuri, null), null);
+  do_check_eq(cs.getCookieString(doturi, null), null);
+
+  // test that an empty host to add() or remove() throws
+  var expiry = (Date.now() + 1000) * 1000;
+  do_check_throws(function() {
+    cm.add("", "/", "foo2", "bar", false, false, true, expiry);
+  }, Cr.NS_ERROR_ILLEGAL_VALUE);
+  do_check_eq(getCookieCount(), 1);
+  do_check_throws(function() {
+    cm.add(".", "/", "foo3", "bar", false, false, true, expiry);
+  }, Cr.NS_ERROR_ILLEGAL_VALUE);
+  do_check_eq(getCookieCount(), 1);
+  cm.add("test.com", "/", "foo", "bar", false, false, true, expiry);
+  do_check_eq(getCookieCount(), 2);
+
+  do_check_throws(function() {
+    cm.remove("", "foo2", "/", false);
+  }, Cr.NS_ERROR_ILLEGAL_VALUE);
+  do_check_eq(getCookieCount(), 2);
+  do_check_throws(function() {
+    cm.remove(".", "foo3", "/", false);
+  }, Cr.NS_ERROR_ILLEGAL_VALUE);
+  do_check_eq(getCookieCount(), 2);
+  cm.remove("test.com", "foo", "/", false);
+  do_check_eq(getCookieCount(), 1);
+
+  do_check_eq(cm.countCookiesFromHost("baz.com"), 1);
+  do_check_eq(cm.countCookiesFromHost(""), 0);
+  do_check_eq(cm.countCookiesFromHost("."), 0);
+
+  var e = cm.getCookiesFromHost("baz.com");
+  do_check_true(e.hasMoreElements());
+  do_check_eq(e.getNext().QueryInterface(Ci.nsICookie2).name, "foo");
+  do_check_false(e.hasMoreElements());
+  e = cm.getCookiesFromHost("");
+  do_check_false(e.hasMoreElements());
+  e = cm.getCookiesFromHost(".");
+  do_check_false(e.hasMoreElements());
+
+  cm.removeAll();
+}
+
+function getCookieCount() {
+  var count = 0;
+  var cm = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager2);
+  var enumerator = cm.enumerator;
+  while (enumerator.hasMoreElements()) {
+    if (!(enumerator.getNext() instanceof Ci.nsICookie2))
+      throw new Error("not a cookie");
+    ++count;
+  }
+  return count;
+}
+
--- a/netwerk/cookie/public/nsICookieManager.idl
+++ b/netwerk/cookie/public/nsICookieManager.idl
@@ -61,13 +61,14 @@ interface nsICookieManager : nsISupports
    */
   readonly attribute nsISimpleEnumerator enumerator;
 
   /**
    * Called to remove an individual cookie from the cookie list
    *
    * @param aDomain The host or domain for which the cookie was set
    * @param aName The name specified in the cookie
+   * @param aPath The path for which the cookie was set
    * @param aBlocked Indicates if cookies from this host should be permanently blocked
    *
    */
   void remove(in AUTF8String aDomain, in ACString aName, in AUTF8String aPath, in boolean aBlocked);
 };
--- a/netwerk/cookie/src/nsCookieService.cpp
+++ b/netwerk/cookie/src/nsCookieService.cpp
@@ -52,16 +52,17 @@
 #include "nsIChannel.h"
 #include "nsIFile.h"
 #include "nsIObserverService.h"
 #include "nsILineInputStream.h"
 #include "nsIEffectiveTLDService.h"
 
 #include "nsCOMArray.h"
 #include "nsArrayEnumerator.h"
+#include "nsEnumeratorUtils.h"
 #include "nsAutoPtr.h"
 #include "nsReadableUtils.h"
 #include "nsCRT.h"
 #include "prtime.h"
 #include "prprf.h"
 #include "nsNetUtil.h"
 #include "nsNetCID.h"
 #include "nsAppDirectoryServiceDefs.h"
@@ -915,16 +916,19 @@ nsCookieService::Add(const nsACString &a
                      const nsACString &aPath,
                      const nsACString &aName,
                      const nsACString &aValue,
                      PRBool            aIsSecure,
                      PRBool            aIsHttpOnly,
                      PRBool            aIsSession,
                      PRInt64           aExpiry)
 {
+  NS_ENSURE_TRUE(!aDomain.IsEmpty() && !aDomain.EqualsLiteral("."),
+                 NS_ERROR_INVALID_ARG);
+
   PRInt64 currentTimeInUsec = PR_Now();
 
   nsRefPtr<nsCookie> cookie =
     nsCookie::Create(aName, aValue, aDomain, aPath,
                      aExpiry,
                      currentTimeInUsec,
                      currentTimeInUsec,
                      aIsSession,
@@ -939,33 +943,36 @@ nsCookieService::Add(const nsACString &a
 }
 
 NS_IMETHODIMP
 nsCookieService::Remove(const nsACString &aHost,
                         const nsACString &aName,
                         const nsACString &aPath,
                         PRBool           aBlocked)
 {
+  NS_ENSURE_TRUE(!aHost.IsEmpty() && !aHost.EqualsLiteral("."),
+                 NS_ERROR_INVALID_ARG);
+
   nsListIter matchIter;
   if (FindCookie(PromiseFlatCString(aHost),
                  PromiseFlatCString(aName),
                  PromiseFlatCString(aPath),
                  matchIter,
                  PR_Now() / PR_USEC_PER_SEC)) {
     nsRefPtr<nsCookie> cookie = matchIter.current;
     RemoveCookieFromList(matchIter);
     NotifyChanged(cookie, NS_LITERAL_STRING("deleted").get());
   }
 
   // check if we need to add the host to the permissions blacklist.
   if (aBlocked && mPermissionService) {
     nsCAutoString host(NS_LITERAL_CSTRING("http://"));
     
     // strip off the domain dot, if necessary
-    if (!aHost.IsEmpty() && aHost.First() == '.')
+    if (aHost.First() == '.')
       host.Append(Substring(aHost, 1, aHost.Length() - 1));
     else
       host.Append(aHost);
 
     nsCOMPtr<nsIURI> uri;
     NS_NewURI(getter_AddRefs(uri), host);
 
     if (uri)
@@ -1242,16 +1249,20 @@ nsCookieService::GetCookieInternal(nsIUR
   nsCAutoString hostFromURI, pathFromURI;
   if (NS_FAILED(aHostURI->GetAsciiHost(hostFromURI)) ||
       NS_FAILED(aHostURI->GetPath(pathFromURI))) {
     COOKIE_LOGFAILURE(GET_COOKIE, aHostURI, nsnull, "couldn't get host/path from URI");
     return;
   }
   // trim trailing dots
   hostFromURI.Trim(".");
+  if (hostFromURI.IsEmpty()) {
+    COOKIE_LOGFAILURE(GET_COOKIE, aHostURI, nsnull, "empty host");
+    return;
+  }
   // insert a leading dot, so we begin the hash lookup with the
   // equivalent domain cookie host
   hostFromURI.Insert(NS_LITERAL_CSTRING("."), 0);
 
   // check if aHostURI is using an https secure protocol.
   // if it isn't, then we can't send a secure cookie over the connection.
   // if SchemeIs fails, assume an insecure connection, to be on the safe side
   PRBool isSecure;
@@ -1830,16 +1841,18 @@ nsCookieService::IsForeign(nsIURI *aHost
   if (NS_FAILED(aHostURI->GetAsciiHost(currentHost)) ||
       NS_FAILED(aFirstURI->GetAsciiHost(firstHost))) {
     // assume foreign
     return PR_TRUE;
   }
   // trim trailing dots
   currentHost.Trim(".");
   firstHost.Trim(".");
+  if (currentHost.IsEmpty() || firstHost.IsEmpty())
+    return PR_TRUE;
 
   // fast path: check if the two hosts are identical.
   // this also covers two special cases:
   // 1) if we're dealing with IP addresses, require an exact match. this
   // eliminates any chance of IP address funkiness (e.g. the alias 127.1
   // domain-matching 99.54.127.1). bug 105917 originally noted the requirement
   // to deal with IP addresses. note that GetBaseDomain() below will return an
   // error if the URI is an IP address.
@@ -1936,16 +1949,18 @@ nsCookieService::CheckDomain(nsCookieAtt
 
   // get host from aHostURI
   nsCAutoString hostFromURI;
   if (NS_FAILED(aHostURI->GetAsciiHost(hostFromURI))) {
     return PR_FALSE;
   }
   // trim trailing dots
   hostFromURI.Trim(".");
+  if (hostFromURI.IsEmpty())
+    return PR_FALSE;
 
   // if a domain is given, check the host has permission
   if (!aCookieAttributes.host.IsEmpty()) {
     aCookieAttributes.host.Trim(".");
     // switch to lowercase now, to avoid case-insensitive compares everywhere
     ToLowerCase(aCookieAttributes.host);
 
     // get the base domain for the host URI.
@@ -2164,18 +2179,19 @@ nsCookieService::CookieExists(nsICookie2
 }
 
 // count the number of cookies from a given host, and simultaneously find the
 // oldest cookie from the host.
 PRUint32
 nsCookieService::CountCookiesFromHostInternal(const nsACString  &aHost,
                                               nsEnumerationData &aData)
 {
+  NS_ASSERTION(!aHost.IsEmpty() && !aHost.EqualsLiteral("."), "empty host");
+
   PRUint32 countFromHost = 0;
-
   nsCAutoString hostWithDot(NS_LITERAL_CSTRING(".") + aHost);
 
   const char *currentDot = hostWithDot.get();
   const char *nextDot = currentDot + 1;
   do {
     nsCookieEntry *entry = mDBState->hostTable.GetEntry(currentDot);
     for (nsListIter iter(entry); iter.current; ++iter) {
       // only count non-expired cookies
@@ -2200,29 +2216,36 @@ nsCookieService::CountCookiesFromHostInt
 }
 
 // count the number of cookies stored by a particular host. this is provided by the
 // nsICookieManager2 interface.
 NS_IMETHODIMP
 nsCookieService::CountCookiesFromHost(const nsACString &aHost,
                                       PRUint32         *aCountFromHost)
 {
+  if (aHost.IsEmpty() || aHost.EqualsLiteral(".")) {
+    *aCountFromHost = 0;
+    return NS_OK;
+  }
+
   // we don't care about finding the oldest cookie here, so disable the search
   nsEnumerationData data(PR_Now() / PR_USEC_PER_SEC, LL_MININT);
-  
   *aCountFromHost = CountCookiesFromHostInternal(aHost, data);
   return NS_OK;
 }
 
 // get an enumerator of cookies stored by a particular host. this is provided by the
 // nsICookieManager2 interface.
 NS_IMETHODIMP
 nsCookieService::GetCookiesFromHost(const nsACString     &aHost,
                                     nsISimpleEnumerator **aEnumerator)
 {
+  if (aHost.IsEmpty() || aHost.EqualsLiteral("."))
+    return NS_NewEmptyEnumerator(aEnumerator);
+
   nsCOMArray<nsICookie> cookieList(mMaxCookiesPerHost);
   nsCAutoString hostWithDot(NS_LITERAL_CSTRING(".") + aHost);
   PRInt64 currentTime = PR_Now() / PR_USEC_PER_SEC;
 
   const char *currentDot = hostWithDot.get();
   const char *nextDot = currentDot + 1;
   do {
     nsCookieEntry *entry = mDBState->hostTable.GetEntry(currentDot);
@@ -2244,16 +2267,18 @@ nsCookieService::GetCookiesFromHost(cons
 // find an exact cookie specified by host, name, and path that hasn't expired.
 PRBool
 nsCookieService::FindCookie(const nsAFlatCString &aHost,
                             const nsAFlatCString &aName,
                             const nsAFlatCString &aPath,
                             nsListIter           &aIter,
                             PRInt64               aCurrentTime)
 {
+  NS_ASSERTION(!aHost.IsEmpty() && !aHost.EqualsLiteral("."), "empty host");
+
   nsCookieEntry *entry = mDBState->hostTable.GetEntry(aHost.get());
   for (aIter = nsListIter(entry); aIter.current; ++aIter) {
     if (aIter.current->Expiry() > aCurrentTime &&
         aPath.Equals(aIter.current->Path()) &&
         aName.Equals(aIter.current->Name())) {
       return PR_TRUE;
     }
   }