Fix a few leaks in SafeBrowsing. These have been around since the beginning, but have been hit by a particular pattern that's being sent down by the SB servers recently.BUG=http://code.google.com/p/chromium/issues/detail?id=4522

Review URL: http://codereview.chromium.org/11336

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@5854 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 76 insertions, 8 deletions

diff --git a/chrome/browser/safe_browsing/protocol_manager.cc b/chrome/browser/safe_browsing/protocol_manager.cc
index af0194f..0f43a03 100644
--- a/chrome/browser/safe_browsing/protocol_manager.cc
+++ b/chrome/browser/safe_browsing/protocol_manager.cc
@@ -299,12 +299,15 @@ bool SafeBrowsingProtocolManager::HandleServiceResponse(const GURL& url,
       // database.
       if (reset) {
         sb_service_->ResetDatabase();
+        delete chunk_deletes;
         return true;
       }
 
       // Chunks to delete from our storage.
       if (!chunk_deletes->empty())
         sb_service_->HandleChunkDelete(chunk_deletes);
+      else
+        delete chunk_deletes;
 
       break;
     }
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
index ff709c7..f7eaeeb 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
@@ -356,14 +356,6 @@ bool SafeBrowsingDatabaseBloom::NeedToCheckUrl(const GURL& url) {
 
 void SafeBrowsingDatabaseBloom::InsertChunks(const std::string& list_name,
                                              std::deque<SBChunk>* chunks) {
-  // We've going to be updating the bloom filter, so delete the on-disk
-  // serialization so that if the process crashes we'll generate a new one on
-  // startup, instead of reading a stale filter.
-  // TODO(erikkay) - is this correct?  Since we can no longer fall back to
-  // database lookups, we need a reasonably current bloom filter at startup.
-  // I think we need some way to indicate that the bloom filter is out of date
-  // and needs to be rebuilt, but we shouldn't delete it.
-  // DeleteBloomFilter();
   if (chunks->empty())
     return;
 
@@ -401,6 +393,11 @@ void SafeBrowsingDatabaseBloom::InsertChunks(const std::string& list_name,
         add_chunk_cache_.insert(encoded);
       else
         sub_chunk_cache_.insert(encoded);
+    } else {
+      while (!chunk.hosts.empty()) {
+        chunk.hosts.front().entry->Destroy();
+        chunk.hosts.pop_front();
+      }
     }
 
     chunks->pop_front();
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_impl.cc b/chrome/browser/safe_browsing/safe_browsing_database_impl.cc
index 34adc83..6d61fd9 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_impl.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_impl.cc
@@ -584,6 +584,11 @@ bool SafeBrowsingDatabaseImpl::ProcessAddChunks(std::deque<SBChunk>* chunks) {
       AddChunkInformation(list_id, ADD_CHUNK, chunk_id,
                           add_chunk_modified_hosts_);
       add_chunk_modified_hosts_.clear();
+    } else {
+      while (!chunk.hosts.empty()) {
+        chunk.hosts.front().entry->Destroy();
+        chunk.hosts.pop_front();
+      }
     }
 
     chunks->pop_front();
@@ -614,6 +619,11 @@ bool SafeBrowsingDatabaseImpl::ProcessSubChunks(std::deque<SBChunk>* chunks) {
       }
 
       AddChunkInformation(list_id, SUB_CHUNK, chunk_id, "");
+    } else {
+      while (!chunk.hosts.empty()) {
+        chunk.hosts.front().entry->Destroy();
+        chunk.hosts.pop_front();
+      }
     }
 
     chunks->pop_front();
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
index 41b8f9c..15d437a 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
@@ -350,6 +350,36 @@ TEST(SafeBrowsingDatabase, Database) {
                                      &matching_list, &prefix_hits,
                                      &full_hashes, now));
 
+
+
+  // Attempt to re-add the first chunk (should be a no-op).
+  // see bug: http://code.google.com/p/chromium/issues/detail?id=4522
+  host.host = Sha256Prefix("www.evil.com/");
+  host.entry = SBEntry::Create(SBEntry::ADD_PREFIX, 2);
+  host.entry->set_chunk_id(1);
+  host.entry->SetPrefixAt(0, Sha256Prefix("www.evil.com/phishing.html"));
+  host.entry->SetPrefixAt(1, Sha256Prefix("www.evil.com/malware.html"));
+
+  chunk.chunk_number = 1;
+  chunk.is_add = true;
+  chunk.hosts.clear();
+  chunk.hosts.push_back(host);
+
+  chunks = new std::deque<SBChunk>;
+  chunks->push_back(chunk);
+  database->UpdateStarted();
+  GetListsInfo(database, &lists);
+  database->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+  database->UpdateFinished(true);
+  lists.clear();
+
+  GetListsInfo(database, &lists);
+  EXPECT_TRUE(lists[0].name == safe_browsing_util::kMalwareList);
+  EXPECT_EQ(lists[0].adds, "1-3");
+  EXPECT_TRUE(lists[0].subs.empty());
+  lists.clear();
+
+
   // Test removing a single prefix from the add chunk.
   host.host = Sha256Prefix("www.evil.com/");
   host.entry = SBEntry::Create(SBEntry::SUB_PREFIX, 1);
@@ -399,6 +429,34 @@ TEST(SafeBrowsingDatabase, Database) {
   EXPECT_EQ(lists[0].subs, "4");
   lists.clear();
 
+  // Test the same sub chunk again.  This should be a no-op.
+  // see bug: http://code.google.com/p/chromium/issues/detail?id=4522
+  host.host = Sha256Prefix("www.evil.com/");
+  host.entry = SBEntry::Create(SBEntry::SUB_PREFIX, 1);
+  host.entry->set_chunk_id(2);
+  host.entry->SetChunkIdAtPrefix(0, 2);
+  host.entry->SetPrefixAt(0, Sha256Prefix("www.evil.com/notevil1.html"));
+
+  chunk.is_add = false;
+  chunk.chunk_number = 4;
+  chunk.hosts.clear();
+  chunk.hosts.push_back(host);
+
+  chunks = new std::deque<SBChunk>;
+  chunks->push_back(chunk);
+
+  database->UpdateStarted();
+  database->GetListsInfo(&lists);
+  database->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+  database->UpdateFinished(true);
+  lists.clear();
+
+  GetListsInfo(database, &lists);
+  EXPECT_TRUE(lists[0].name == safe_browsing_util::kMalwareList);
+  EXPECT_EQ(lists[0].subs, "4");
+  lists.clear();
+
+
   // Test removing all the prefixes from an add chunk.
   database->UpdateStarted();
   database->GetListsInfo(&lists);