diff options
| author | gavinp <gavinp@chromium.org> | 2016-02-25 09:20:36 -0800 |
|---|---|---|
| committer | Commit bot <commit-bot@chromium.org> | 2016-02-25 17:21:36 +0000 |
| commit | b7129635933594a6eb91e82e8139235e6a843a05 (patch) | |
| tree | 7c78b4943fb743eed84d60924d221e978241e849 | |
| parent | d53461d6e82d2052b9a50a06554ef99e1f910a7e (diff) | |
| download | chromium_src-b7129635933594a6eb91e82e8139235e6a843a05.zip chromium_src-b7129635933594a6eb91e82e8139235e6a843a05.tar.gz chromium_src-b7129635933594a6eb91e82e8139235e6a843a05.tar.bz2 | |
Fix use after free in memory only backend.
Writing to a sparse entry could result that entry being evicted while
the write was still in progress. This fix considers sparse entries in
use if any of their children are in use; this can drive up memory use
but avoids tickling the dragon by expanding the hand rolled
refcounting implementation in the in memory cache.
R=mmenke@chromium.org
BUG=589186
Review URL: https://codereview.chromium.org/1725363005
Cr-Commit-Position: refs/heads/master@{#377598}
| -rw-r--r-- | net/disk_cache/backend_unittest.cc | 38 | ||||
| -rw-r--r-- | net/disk_cache/memory/mem_entry_impl.cc | 12 |
2 files changed, 41 insertions, 9 deletions
diff --git a/net/disk_cache/backend_unittest.cc b/net/disk_cache/backend_unittest.cc index ebd2f57..7f2259d 100644 --- a/net/disk_cache/backend_unittest.cc +++ b/net/disk_cache/backend_unittest.cc @@ -2947,6 +2947,44 @@ TEST_F(DiskCacheBackendTest, MemoryOnlyBackendEviction) { // TODO(gavinp): Enable BackendEviction test for simple cache after performance // problems are addressed. See crbug.com/588184 for more information. +// This overly specific looking test is a regression test aimed at +// crbug.com/589186. +TEST_F(DiskCacheBackendTest, MemoryOnlyUseAfterFree) { + SetMemoryOnlyMode(); + + const int kMaxSize = 200 * 1024; + const int kMaxEntryCount = 20; + const int kWriteSize = kMaxSize / kMaxEntryCount; + + SetMaxSize(kMaxSize); + InitCache(); + + scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kWriteSize)); + CacheTestFillBuffer(buffer->data(), kWriteSize, false); + + // Create an entry to be our sparse entry that gets written later. + disk_cache::Entry* entry; + ASSERT_EQ(net::OK, CreateEntry("first parent", &entry)); + disk_cache::ScopedEntryPtr first_parent(entry); + + // Create a ton of entries, and keep them open, to put the cache well above + // its eviction threshhold. + const int kTooManyEntriesCount = kMaxEntryCount * 2; + std::list<disk_cache::ScopedEntryPtr> open_entries; + std::string key_prefix("prefix"); + for (int i = 0; i < kTooManyEntriesCount; ++i) { + ASSERT_EQ(net::OK, CreateEntry(key_prefix + base::IntToString(i), &entry)); + EXPECT_EQ(kWriteSize, + WriteData(entry, 1, 0, buffer.get(), kWriteSize, false)); + open_entries.push_back(disk_cache::ScopedEntryPtr(entry)); + } + EXPECT_LT(kMaxSize, CalculateSizeOfAllEntries()); + + // Writing this sparse data should not crash. + EXPECT_EQ(1024, first_parent->WriteSparseData(32768, buffer.get(), 1024, + net::CompletionCallback())); +} + TEST_F(DiskCacheTest, Backend_UsageStatsTimer) { MessageLoopHelper helper; diff --git a/net/disk_cache/memory/mem_entry_impl.cc b/net/disk_cache/memory/mem_entry_impl.cc index c28a7fa..bcbebb3 100644 --- a/net/disk_cache/memory/mem_entry_impl.cc +++ b/net/disk_cache/memory/mem_entry_impl.cc @@ -103,16 +103,10 @@ void MemEntryImpl::Open() { } bool MemEntryImpl::InUse() const { - if (type() == PARENT_ENTRY) { - return ref_count_ > 0; - } else { - // TODO(gavinp): Can't this just be a DCHECK? How would ref_count_ not be - // zero? + if (type() == CHILD_ENTRY) + return parent_->InUse(); - // A child entry is never in use. Thus one can always be evicted, even while - // its parent entry is open and in use. - return false; - } + return ref_count_ > 0; } int MemEntryImpl::GetStorageSize() const { |