Bug 1428589: Don't read a destroyed cookie list if the last cookie in the entry expired. r=jdm a=gchang
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 11 Jan 2018 14:12:38 +0200
changeset 445637 000398f8c373407ba621d56cbd3b370cf43469a8
parent 445636 e57848b337fcaeec8a8e23f45354603df11c2b5b
child 445638 15c6742655c07b097d9b8dd3529eea0d62388645
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm, gchang
bugs1428589
milestone58.0
Bug 1428589: Don't read a destroyed cookie list if the last cookie in the entry expired. r=jdm a=gchang Consider the following situation, which is what causes the failure: * `cookies` is an array of length 1 (either from the beginning, or because other cookies have been expired while in this loop). * the only cookie that remains is expired. We append the cookie to removedList, and then we call gCookieService->RemoveCookieFromList, which has the following code: if (aIter.entry->GetCookies().Length() == 1) { mDBState->hostTable.RawRemoveEntry(aIter.entry); } else { aIter.entry->GetCookies().RemoveElementAt(aIter.index); } If we enter the first branch, as it's the case, that will destroy the array. We're effectively removing stuff from the array while mutating it, which is scary. It's fine if we don't delete the array, since we iterate through it using indices, but still it's dangerous as heck. If we're the last element in the array though, we're doomed, because `cookies` is now destroyed. We not only try to access the array length again, but also we try to index on it the next time because we never stopped the loop (`i` is still zero, and the length may very well be garbage). Fix it by keeping the length in sync from the stack and breaking out from the loop if appropriately. MozReview-Commit-ID: 6qaC9yclvP2
netwerk/cookie/nsCookieService.cpp
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -4444,41 +4444,46 @@ nsCookieService::PurgeCookies(int64_t aC
 
   int64_t currentTime = aCurrentTimeInUsec / PR_USEC_PER_SEC;
   int64_t purgeTime = aCurrentTimeInUsec - mCookiePurgeAge;
   int64_t oldestTime = INT64_MAX;
 
   for (auto iter = mDBState->hostTable.Iter(); !iter.Done(); iter.Next()) {
     nsCookieEntry* entry = iter.Get();
 
-    const nsCookieEntry::ArrayType &cookies = entry->GetCookies();
-    for (nsCookieEntry::IndexType i = 0; i < cookies.Length(); ) {
+    const nsCookieEntry::ArrayType& cookies = entry->GetCookies();
+    auto length = cookies.Length();
+    for (nsCookieEntry::IndexType i = 0; i < length; ) {
       nsListIter iter(entry, i);
       nsCookie* cookie = cookies[i];
 
       // check if the cookie has expired
       if (cookie->Expiry() <= currentTime) {
         removedList->AppendElement(cookie);
         COOKIE_LOGEVICTED(cookie, "Cookie expired");
 
-        // remove from list; do not increment our iterator
+        // remove from list; do not increment our iterator unless we're the last
+        // in the list already.
         gCookieService->RemoveCookieFromList(iter, paramsArray);
-
+        if (i == --length) {
+          break;
+        }
       } else {
         // check if the cookie is over the age limit
         if (cookie->LastAccessed() <= purgeTime) {
           purgeList.AppendElement(iter);
 
         } else if (cookie->LastAccessed() < oldestTime) {
           // reset our indicator
           oldestTime = cookie->LastAccessed();
         }
 
         ++i;
       }
+      MOZ_ASSERT(length == cookies.Length());
     }
   }
 
   uint32_t postExpiryCookieCount = mDBState->cookieCount;
 
   // now we have a list of iterators for cookies over the age limit.
   // sort them by age, and then we'll see how many to remove...
   purgeList.Sort(CompareCookiesByAge());