disk_cache/blockfile: Fix issues involving trailing nulls for keys

The code for reading entries' keys has trusted that the supposed-to-be
on-disk nul termination was there, which is a mistake since sometimes
files get corrupted  ...And also because in some cases we've failed to
write it out. So fix both.

This has mostly stopped being an active issue since May when our
Windows build has switched to libc++, as that fixes up nuls on
std::string copy (Microsoft's version doesn't), but it seems
worth fixing regardless for other embedders and good hygiene.

Bug: 973943
Change-Id: I86c6e44471447671004efd37aeecd760b3972183
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1718844Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#681483}

net/disk_cache/blockfile/entry_impl.cc

@@ -458,7 +458,7 @@ bool EntryImpl::CreateEntry(Addr node_address,
     if (address.is_block_file())
       offset = address.start_block() * address.BlockSize() + kBlockHeaderSize;
 
-    if (!key_file || !key_file->Write(key.data(), key.size(), offset)) {
+    if (!key_file || !key_file->Write(key.data(), key.size() + 1, offset)) {
       DeleteData(address, kKeyFileIndex);
       return false;
     }
@@ -789,7 +789,7 @@ std::string EntryImpl::GetKey() const {
   CacheEntryBlock* entry = const_cast<CacheEntryBlock*>(&entry_);
   int key_len = entry->Data()->key_len;
   if (key_len <= kMaxInternalKeyLength)
-    return std::string(entry->Data()->key);
+    return std::string(entry->Data()->key, key_len);
 
   // We keep a copy of the key so that we can always return it, even if the
   // backend is disabled.
@@ -808,12 +808,18 @@ std::string EntryImpl::GetKey() const {
   if (!key_file)
     return std::string();
 
-  ++key_len;  // We store a trailing \0 on disk that we read back below.
+  ++key_len;  // We store a trailing \0 on disk.
   if (!offset && key_file->GetLength() != static_cast<size_t>(key_len))
     return std::string();
 
-  if (!key_file->Read(base::WriteInto(&key_, key_len), key_len, offset))
+  // WriteInto will ensure that key_.length() == key_len - 1, and so
+  // key_.c_str()[key_len] will be '\0'. Taking advantage of this, do not
+  // attempt read up to the expected on-disk '\0' --- which would be |key_len|
+  // bytes total --- as if due to a corrupt file it isn't |key_| would get its
+  // internal nul messed up.
+  if (!key_file->Read(base::WriteInto(&key_, key_len), key_len - 1, offset))
     key_.clear();
+  DCHECK_LE(strlen(key_.data()), static_cast<size_t>(key_len));
   return key_;
 }
 

net/disk_cache/entry_unittest.cc

@@ -2656,6 +2656,67 @@ TEST_F(DiskCacheEntryTest, KeySanityCheck) {
   DisableIntegrityCheck();
 }
 
+TEST_F(DiskCacheEntryTest, KeySanityCheck2) {
+  UseCurrentThread();
+  InitCache();
+  std::string key("the first key");
+  disk_cache::Entry* entry;
+  ASSERT_THAT(CreateEntry(key, &entry), IsOk());
+
+  disk_cache::EntryImpl* entry_impl =
+      static_cast<disk_cache::EntryImpl*>(entry);
+  disk_cache::EntryStore* store = entry_impl->entry()->Data();
+
+  // Fill in the rest of inline key store with non-nulls. Unlike in
+  // KeySanityCheck, this does not change the length to identify it as
+  // stored under |long_key|.
+  memset(store->key + key.size(), 'k', sizeof(store->key) - key.size());
+  entry_impl->entry()->set_modified();
+  entry->Close();
+
+  // We have a corrupt entry. Now reload it. We should NOT read beyond the
+  // allocated buffer here.
+  ASSERT_NE(net::OK, OpenEntry(key, &entry));
+  DisableIntegrityCheck();
+}
+
+TEST_F(DiskCacheEntryTest, KeySanityCheck3) {
+  const size_t kVeryLong = 40 * 1024;
+  UseCurrentThread();
+  InitCache();
+  std::string key(kVeryLong, 'a');
+  disk_cache::Entry* entry;
+  ASSERT_THAT(CreateEntry(key, &entry), IsOk());
+
+  disk_cache::EntryImpl* entry_impl =
+      static_cast<disk_cache::EntryImpl*>(entry);
+  disk_cache::EntryStore* store = entry_impl->entry()->Data();
+
+  // Test meaningful when using long keys; and also want this to be
+  // an external file to avoid needing to duplicate offset math here.
+  disk_cache::Addr key_addr(store->long_key);
+  ASSERT_TRUE(key_addr.is_initialized());
+  ASSERT_TRUE(key_addr.is_separate_file());
+
+  // Close the entry before messing up its files.
+  entry->Close();
+
+  // Mess up the terminating null in the external key file.
+  auto key_file =
+      base::MakeRefCounted<disk_cache::File>(true /* want sync ops*/);
+  ASSERT_TRUE(key_file->Init(cache_impl_->GetFileName(key_addr)));
+
+  ASSERT_TRUE(key_file->Write("b", 1u, kVeryLong));
+  key_file = nullptr;
+
+  // This case gets graceful recovery.
+  ASSERT_THAT(OpenEntry(key, &entry), IsOk());
+
+  // Make sure the key object isn't messed up.
+  EXPECT_EQ(kVeryLong, strlen(entry->GetKey().data()));
+  entry->Close();
+}
+
 TEST_F(DiskCacheEntryTest, SimpleCacheInternalAsyncIO) {
   SetSimpleCacheMode();
   InitCache();