Skip to content

Commit

Permalink
Fix potential buffer vs. IO size mismatch in StorageBlock.
Browse files Browse the repository at this point in the history
I don't think this happens in practice, or we would likely have
seen crashes or corruption. When calling CopyFrom(), the other
storage block must have the same num_buffers() value, otherwise
we will allocate buffers when calling the Data() method based
on the old num_buffers() value, and then update it to the other's
num_buffer() value. Later on, Store() may calculate a size based
upon the new value, but the buffer is still sized according to the
old value.

-- Add test to show this situation.

Change-Id: Ib72215dd6c51d90be0483158543daf6a1d814b69
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5039450
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1226956}
  • Loading branch information
tsepez authored and Chromium LUCI CQ committed Nov 20, 2023
1 parent 486644b commit 3b5de73
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 2 deletions.
4 changes: 2 additions & 2 deletions net/disk_cache/blockfile/storage_block-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ void StorageBlock<T>::CopyFrom(StorageBlock<T>* other) {
DCHECK(!modified_);
DCHECK(!other->modified_);
Discard();
*Data() = *other->Data();
file_ = other->file_;
address_ = other->address_;
extended_ = other->extended_;
file_ = other->file_;
*Data() = *other->Data();
}

template<typename T> void* StorageBlock<T>::buffer() const {
Expand Down
19 changes: 19 additions & 0 deletions net/disk_cache/blockfile/storage_block_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,22 @@ TEST_F(DiskCacheTest, StorageBlock_SetModified) {
EXPECT_TRUE(entry2.Load());
EXPECT_TRUE(0x45687912 == entry2.Data()->hash);
}

TEST_F(DiskCacheTest, StorageBlock_DifferentNumBuffers) {
base::FilePath filename = cache_path_.AppendASCII("a_test");
auto file = base::MakeRefCounted<disk_cache::MappedFile>();
ASSERT_TRUE(CreateCacheTestFile(filename));
ASSERT_TRUE(file->Init(filename, 8192));

// 2 buffers at index 1.
CacheEntryBlock entry1(file.get(), disk_cache::Addr(0xa1010001));
EXPECT_TRUE(entry1.Load());

// 1 buffer at index 3.
CacheEntryBlock entry2(file.get(), disk_cache::Addr(0xa0010003));
EXPECT_TRUE(entry2.Load());

// Now specify 2 buffers at index 1.
entry2.CopyFrom(&entry1);
EXPECT_TRUE(entry2.Load());
}

0 comments on commit 3b5de73

Please sign in to comment.