Fix use after free in memory only backend.

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
Committed: https://crrev.com/b7129635933594a6eb91e82e8139235e6a843a05
Cr-Commit-Position: refs/heads/master@{#377598}

[gavinp, 2016-02-24 20:30:34]

Matt,

Another way to fix this is to extend the refcounting in to child entries. That
scares me.

The problem with this approach is that it will allow individual entries to get
quite large at times.

[gavinp, 2016-02-24 20:31:16]

Description was changed from

==========
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=588184
==========

to

==========
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=588186
==========

[gavinp, 2016-02-24 20:31:32]

Description was changed from

==========
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=588186
==========

to

==========
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
==========

[mmenke, 2016-02-25 07:14:06]

This LGTM.  This does mean one parent entry with a bunch of sparse entries could
evict everything else from the cache, though I suppose that could happen even
before, if all its children were the most recently created entries.

https://codereview.chromium.org/1725363005/diff/80001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1725363005/diff/80001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:2952: TEST_F(DiskCacheBackendTest,
MemoryCacheUseAfterFree) {
Should we run similar tests for disk cache implementations?  Looks to me like
even the block file cache doesn't have this issue, but still seems like
something worth testing.  Fine to worry about it in another CL.

https://codereview.chromium.org/1725363005/diff/80001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:2969: EXPECT_EQ(1024,
first_parent->WriteSparseData(0, buffer.get(), 1024,
Should have a comment about why this first write is needed.

https://codereview.chromium.org/1725363005/diff/80001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:2975: disk_cache::ScopedEntryPtr
open_entries[kTooManyEntriesCount];
optional:  I'd suggest just using a std::vector<disk_cache::ScopedEntryPtr> or
std::list, just to make the test more flexible for future changes.

[gavinp, 2016-02-25 15:55:32]

Thanks for the quick review. I'll upload the merge version of this fix shortly.

https://codereview.chromium.org/1725363005/diff/80001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1725363005/diff/80001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:2952: TEST_F(DiskCacheBackendTest,
MemoryCacheUseAfterFree) {
On 2016/02/25 07:14:05, mmenke wrote:
> Should we run similar tests for disk cache implementations?  Looks to me like
> even the block file cache doesn't have this issue, but still seems like
> something worth testing.  Fine to worry about it in another CL.

Acknowledged.

https://codereview.chromium.org/1725363005/diff/80001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:2969: EXPECT_EQ(1024,
first_parent->WriteSparseData(0, buffer.get(), 1024,
On 2016/02/25 07:14:06, mmenke wrote:
> Should have a comment about why this first write is needed.

Actually, it's not needed, so it's removed.

https://codereview.chromium.org/1725363005/diff/80001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:2975: disk_cache::ScopedEntryPtr
open_entries[kTooManyEntriesCount];
On 2016/02/25 07:14:06, mmenke wrote:
> optional:  I'd suggest just using a std::vector<disk_cache::ScopedEntryPtr> or
> std::list, just to make the test more flexible for future changes.

Done.

https://codereview.chromium.org/1725363005/diff/100001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1725363005/diff/100001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:2952: TEST_F(DiskCacheBackendTest,
MemoryOnlyUseAfterFree) {
The rename of the test makes it way more consistent with other tests.

[gavinp, 2016-02-25 15:55:50]

The CQ bit was checked by gavinp@chromium.org

[gavinp, 2016-02-25 15:55:50]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1725363005/#ps100001
(title: "remediate")

[commit-bot: I haz the power, 2016-02-25 15:57:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1725363005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1725363005/100001

[gavinp, 2016-02-25 15:58:18]

On 2016/02/25 07:14:06, mmenke wrote:
> This LGTM.  This does mean one parent entry with a bunch of sparse entries
could
> evict everything else from the cache, though I suppose that could happen even
> before, if all its children were the most recently created entries.
> 


Fair point Matt. My hope is that it's a short term thing; the future API with
actually proper refcounting (post blockfile rm) will make it much easier to make
a safe in memory cache.

[commit-bot: I haz the power, 2016-02-25 17:20:43]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-02-25 17:20:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-25 17:21:43]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/b7129635933594a6eb91e82e8139235e6a843a05
Cr-Commit-Position: refs/heads/master@{#377598}
==========

[commit-bot: I haz the power, 2016-02-25 17:21:44]

Patchset 6 (id:??) landed as
https://crrev.com/b7129635933594a6eb91e82e8139235e6a843a05
Cr-Commit-Position: refs/heads/master@{#377598}