Fix possible map::end() dereference in AppCacheUpdateJob triggered by a compromised renderer.
BUG=551044
Review URL: https://codereview.chromium.org/1418783005
Cr-Commit-Position: refs/heads/master@{#358815}
diff --git a/content/browser/appcache/appcache_backend_impl.cc b/content/browser/appcache/appcache_backend_impl.cc
index e055f1a..01eed000 100644
--- a/content/browser/appcache/appcache_backend_impl.cc
+++ b/content/browser/appcache/appcache_backend_impl.cc
@@ -68,32 +68,29 @@
const int64 cache_document_was_loaded_from,
const GURL& manifest_url) {
AppCacheHost* host = GetHost(host_id);
- if (!host || host->was_select_cache_called())
+ if (!host)
return false;
- host->SelectCache(document_url, cache_document_was_loaded_from,
+ return host->SelectCache(document_url, cache_document_was_loaded_from,
manifest_url);
- return true;
}
bool AppCacheBackendImpl::SelectCacheForWorker(
int host_id, int parent_process_id, int parent_host_id) {
AppCacheHost* host = GetHost(host_id);
- if (!host || host->was_select_cache_called())
+ if (!host)
return false;
- host->SelectCacheForWorker(parent_process_id, parent_host_id);
- return true;
+ return host->SelectCacheForWorker(parent_process_id, parent_host_id);
}
bool AppCacheBackendImpl::SelectCacheForSharedWorker(
int host_id, int64 appcache_id) {
AppCacheHost* host = GetHost(host_id);
- if (!host || host->was_select_cache_called())
+ if (!host)
return false;
- host->SelectCacheForSharedWorker(appcache_id);
- return true;
+ return host->SelectCacheForSharedWorker(appcache_id);
}
bool AppCacheBackendImpl::MarkAsForeignEntry(
@@ -104,8 +101,7 @@
if (!host)
return false;
- host->MarkAsForeignEntry(document_url, cache_document_was_loaded_from);
- return true;
+ return host->MarkAsForeignEntry(document_url, cache_document_was_loaded_from);
}
bool AppCacheBackendImpl::GetStatusWithCallback(
diff --git a/content/browser/appcache/appcache_host.cc b/content/browser/appcache/appcache_host.cc
index dbdaf308..9ec45f61 100644
--- a/content/browser/appcache/appcache_host.cc
+++ b/content/browser/appcache/appcache_host.cc
@@ -80,18 +80,21 @@
observers_.RemoveObserver(observer);
}
-void AppCacheHost::SelectCache(const GURL& document_url,
+bool AppCacheHost::SelectCache(const GURL& document_url,
const int64 cache_document_was_loaded_from,
const GURL& manifest_url) {
+ if (was_select_cache_called_)
+ return false;
+
DCHECK(pending_start_update_callback_.is_null() &&
pending_swap_cache_callback_.is_null() &&
pending_get_status_callback_.is_null() &&
- !is_selection_pending() && !was_select_cache_called_);
+ !is_selection_pending());
was_select_cache_called_ = true;
if (!is_cache_selection_enabled_) {
FinishCacheSelection(NULL, NULL);
- return;
+ return true;
}
origin_in_use_ = document_url.GetOrigin();
@@ -111,7 +114,7 @@
if (cache_document_was_loaded_from != kAppCacheNoCacheId) {
LoadSelectedCache(cache_document_was_loaded_from);
- return;
+ return true;
}
if (!manifest_url.is_empty() &&
@@ -132,7 +135,7 @@
0,
false /*is_cross_origin*/));
frontend_->OnContentBlocked(host_id_, manifest_url);
- return;
+ return true;
}
// Note: The client detects if the document was not loaded using HTTP GET
@@ -141,49 +144,62 @@
set_preferred_manifest_url(manifest_url);
new_master_entry_url_ = document_url;
LoadOrCreateGroup(manifest_url);
- return;
+ return true;
}
// TODO(michaeln): If there was a manifest URL, the user agent may report
// to the user that it was ignored, to aid in application development.
FinishCacheSelection(NULL, NULL);
+ return true;
}
-void AppCacheHost::SelectCacheForWorker(int parent_process_id,
+bool AppCacheHost::SelectCacheForWorker(int parent_process_id,
int parent_host_id) {
+ if (was_select_cache_called_)
+ return false;
+
DCHECK(pending_start_update_callback_.is_null() &&
pending_swap_cache_callback_.is_null() &&
pending_get_status_callback_.is_null() &&
- !is_selection_pending() && !was_select_cache_called_);
+ !is_selection_pending());
was_select_cache_called_ = true;
parent_process_id_ = parent_process_id;
parent_host_id_ = parent_host_id;
FinishCacheSelection(NULL, NULL);
+ return true;
}
-void AppCacheHost::SelectCacheForSharedWorker(int64 appcache_id) {
+bool AppCacheHost::SelectCacheForSharedWorker(int64 appcache_id) {
+ if (was_select_cache_called_)
+ return false;
+
DCHECK(pending_start_update_callback_.is_null() &&
pending_swap_cache_callback_.is_null() &&
pending_get_status_callback_.is_null() &&
- !is_selection_pending() && !was_select_cache_called_);
+ !is_selection_pending());
was_select_cache_called_ = true;
if (appcache_id != kAppCacheNoCacheId) {
LoadSelectedCache(appcache_id);
- return;
+ return true;
}
FinishCacheSelection(NULL, NULL);
+ return true;
}
// TODO(michaeln): change method name to MarkEntryAsForeign for consistency
-void AppCacheHost::MarkAsForeignEntry(const GURL& document_url,
+bool AppCacheHost::MarkAsForeignEntry(const GURL& document_url,
int64 cache_document_was_loaded_from) {
+ if (was_select_cache_called_)
+ return false;
+
// The document url is not the resource url in the fallback case.
storage()->MarkEntryAsForeign(
main_resource_was_namespace_entry_ ? namespace_entry_url_ : document_url,
cache_document_was_loaded_from);
SelectCache(document_url, kAppCacheNoCacheId, GURL());
+ return true;
}
void AppCacheHost::GetStatusWithCallback(const GetStatusCallback& callback,
diff --git a/content/browser/appcache/appcache_host.h b/content/browser/appcache/appcache_host.h
index 82029ba..3818f30 100644
--- a/content/browser/appcache/appcache_host.h
+++ b/content/browser/appcache/appcache_host.h
@@ -33,6 +33,7 @@
FORWARD_DECLARE_TEST(AppCacheHostTest, ForDedicatedWorker);
FORWARD_DECLARE_TEST(AppCacheHostTest, SelectCacheAllowed);
FORWARD_DECLARE_TEST(AppCacheHostTest, SelectCacheBlocked);
+FORWARD_DECLARE_TEST(AppCacheHostTest, SelectCacheTwice);
FORWARD_DECLARE_TEST(AppCacheTest, CleanupUnusedCache);
class AppCache;
class AppCacheFrontend;
@@ -76,13 +77,13 @@
void RemoveObserver(Observer* observer);
// Support for cache selection and scriptable method calls.
- void SelectCache(const GURL& document_url,
+ bool SelectCache(const GURL& document_url,
const int64 cache_document_was_loaded_from,
const GURL& manifest_url);
- void SelectCacheForWorker(int parent_process_id,
+ bool SelectCacheForWorker(int parent_process_id,
int parent_host_id);
- void SelectCacheForSharedWorker(int64 appcache_id);
- void MarkAsForeignEntry(const GURL& document_url,
+ bool SelectCacheForSharedWorker(int64 appcache_id);
+ bool MarkAsForeignEntry(const GURL& document_url,
int64 cache_document_was_loaded_from);
void GetStatusWithCallback(const GetStatusCallback& callback,
void* callback_param);
@@ -163,7 +164,6 @@
AppCacheStorage* storage() const { return storage_; }
AppCacheFrontend* frontend() const { return frontend_; }
AppCache* associated_cache() const { return associated_cache_.get(); }
- bool was_select_cache_called() const { return was_select_cache_called_; }
void enable_cache_selection(bool enable) {
is_cache_selection_enabled_ = enable;
@@ -336,6 +336,7 @@
FRIEND_TEST_ALL_PREFIXES(content::AppCacheHostTest, ForDedicatedWorker);
FRIEND_TEST_ALL_PREFIXES(content::AppCacheHostTest, SelectCacheAllowed);
FRIEND_TEST_ALL_PREFIXES(content::AppCacheHostTest, SelectCacheBlocked);
+ FRIEND_TEST_ALL_PREFIXES(content::AppCacheHostTest, SelectCacheTwice);
FRIEND_TEST_ALL_PREFIXES(content::AppCacheTest, CleanupUnusedCache);
DISALLOW_COPY_AND_ASSIGN(AppCacheHost);
diff --git a/content/browser/appcache/appcache_host_unittest.cc b/content/browser/appcache/appcache_host_unittest.cc
index 817e8c0..255bfa9 100644
--- a/content/browser/appcache/appcache_host_unittest.cc
+++ b/content/browser/appcache/appcache_host_unittest.cc
@@ -530,4 +530,17 @@
service_.set_quota_manager_proxy(NULL);
}
+TEST_F(AppCacheHostTest, SelectCacheTwice) {
+ AppCacheHost host(1, &mock_frontend_, &service_);
+ const GURL kDocAndOriginUrl(GURL("http://whatever/").GetOrigin());
+
+ EXPECT_TRUE(host.SelectCache(kDocAndOriginUrl, kAppCacheNoCacheId, GURL()));
+
+ // Select methods should bail if cache has already been selected.
+ EXPECT_FALSE(host.SelectCache(kDocAndOriginUrl, kAppCacheNoCacheId, GURL()));
+ EXPECT_FALSE(host.SelectCacheForWorker(0, 0));
+ EXPECT_FALSE(host.SelectCacheForSharedWorker(kAppCacheNoCacheId));
+ EXPECT_FALSE(host.MarkAsForeignEntry(kDocAndOriginUrl, kAppCacheNoCacheId));
+}
+
} // namespace content
diff --git a/content/browser/appcache/appcache_update_job.cc b/content/browser/appcache/appcache_update_job.cc
index 8720fe78..ff06550 100644
--- a/content/browser/appcache/appcache_update_job.cc
+++ b/content/browser/appcache/appcache_update_job.cc
@@ -1105,10 +1105,10 @@
// The host is about to be deleted; remove from our collection.
PendingMasters::iterator found =
pending_master_entries_.find(host->pending_master_entry_url());
- DCHECK(found != pending_master_entries_.end());
+ CHECK(found != pending_master_entries_.end());
PendingHosts& hosts = found->second;
PendingHosts::iterator it = std::find(hosts.begin(), hosts.end(), host);
- DCHECK(it != hosts.end());
+ CHECK(it != hosts.end());
hosts.erase(it);
}
