Fix read-after-free when loading two search engines with the same keyword.

chrome/browser/search_engines/template_url_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/search_engines/template_url_service.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 1517 matching lines @@
   }
 
   if (!template_url->sync_guid().empty())
     guid_to_template_map_[template_url->sync_guid()] = template_url;
   if (loaded_) {
     UIThreadSearchTermsData search_terms_data(profile_);
     provider_map_->Add(template_url, search_terms_data);
   }
 }
 
-void TemplateURLService::SetTemplateURLs(const TemplateURLVector& urls) {
-  // Add mappings for the new items.
+// Helper for partition() call in next function.
+bool HasValidID(TemplateURL* t_url) {
+  return t_url->id() != kInvalidTemplateURLID;
+}
 
-  // First, add the items that already have id's, so that the next_id_
-  // gets properly set.
-  for (TemplateURLVector::const_iterator i = urls.begin(); i != urls.end();
+void TemplateURLService::SetTemplateURLs(TemplateURLVector* urls) {
+  // Partition the URLs first, instead of implementing the loops below by simply
+  // scanning the input twice.  While it's not supposed to happen normally, it's
+  // possible for corrupt databases to return multiple entries with the same
+  // keyword.  In this case, the first loop may delete the first entry when
+  // adding the second.  If this happens, the second loop must not attempt to
+  // access the deleted entry.  Partitioning ensures this constraint.
+  TemplateURLVector::iterator first_invalid(
+      std::partition(urls->begin(), urls->end(), HasValidID));

[beaudoin, 2013/09/14 03:24:03]

Clever use of partition!

+
+  // First, add the items that already have id's, so that the next_id_ gets
+  // properly set.
+  for (TemplateURLVector::const_iterator i = urls->begin(); i != first_invalid;
        ++i) {
-    if ((*i)->id() != kInvalidTemplateURLID) {
-      next_id_ = std::max(next_id_, (*i)->id());
-      AddNoNotify(*i, false);
-    }
+    next_id_ = std::max(next_id_, (*i)->id());
+    AddNoNotify(*i, false);
   }
 
   // Next add the new items that don't have id's.
-  for (TemplateURLVector::const_iterator i = urls.begin(); i != urls.end();
-       ++i) {
-    if ((*i)->id() == kInvalidTemplateURLID)
-      AddNoNotify(*i, true);
-  }
+  for (TemplateURLVector::const_iterator i = first_invalid; i != urls->end();
+       ++i)
+    AddNoNotify(*i, true);
+
+  // Clear the input vector to reduce the chance callers will try to use a
+  // (possibly deleted) entry.
+  urls->clear();
 }
 
 void TemplateURLService::ChangeToLoadedState() {
   DCHECK(!loaded_);
 
   UIThreadSearchTermsData search_terms_data(profile_);
   provider_map_->Init(template_urls_, search_terms_data);
   loaded_ = true;
 }
 
@@ skipping 959 matching lines @@
 
   // Remove entries that were created because of policy as they may have
   // changed since the database was saved.
   RemoveProvidersCreatedByPolicy(template_urls,
                                  &default_search_provider,
                                  default_from_prefs.get());
 
   PatchMissingSyncGUIDs(template_urls);
 
   if (is_default_search_managed_) {
-    SetTemplateURLs(*template_urls);
+    SetTemplateURLs(template_urls);
 
     if (TemplateURLsHaveSamePrefs(default_search_provider,
                                   default_from_prefs.get())) {
       // The value from the preferences was previously stored in the database.
       // Reuse it.
     } else {
       // The value from the preferences takes over.
       default_search_provider = NULL;
       if (default_from_prefs.get()) {
         TemplateURLData data(default_from_prefs->data());
@@ skipping 27 matching lines @@
     }
 
     // If the default search provider existed previously, then just
     // set the member variable. Otherwise, we'll set it using the method
     // to ensure that it is saved properly after its id is set.
     if (default_search_provider &&
         (default_search_provider->id() != kInvalidTemplateURLID)) {
       default_search_provider_ = default_search_provider;
       default_search_provider = NULL;
     }
-    SetTemplateURLs(*template_urls);
+    SetTemplateURLs(template_urls);
 
     if (default_search_provider) {
       // Note that this saves the default search provider to prefs.
       SetDefaultSearchProvider(default_search_provider);
     } else {
       // Always save the default search provider to prefs. That way we don't
       // have to worry about it being out of sync.
       if (default_search_provider_)
         SaveDefaultSearchProviderToPrefs(default_search_provider_);
     }
@@ skipping 31 matching lines @@
     const ExtensionKeyword& extension_keyword) const {
   TemplateURLData data;
   data.short_name = UTF8ToUTF16(extension_keyword.extension_name);
   data.SetKeyword(UTF8ToUTF16(extension_keyword.extension_keyword));
   // This URL is not actually used for navigation. It holds the extension's
   // ID, as well as forcing the TemplateURL to be treated as a search keyword.
   data.SetURL(std::string(extensions::kExtensionScheme) + "://" +
       extension_keyword.extension_id + "/?q={searchTerms}");
   return new TemplateURL(profile_, data);
 }