SimpleCache: Tolerate weird file sizes when doing index recovery.

net/disk_cache/simple/simple_index_file.cc

Index: net/disk_cache/simple/simple_index_file.cc
diff --git a/net/disk_cache/simple/simple_index_file.cc b/net/disk_cache/simple/simple_index_file.cc
index d31d2446c60914b64c73039a77881804fcddc1fe..56344cf6c9e564f937fc1644f607f429facf3685 100644
--- a/net/disk_cache/simple/simple_index_file.cc
+++ b/net/disk_cache/simple/simple_index_file.cc
@@ -160,14 +160,29 @@ void ProcessEntryFile(SimpleIndex::EntrySet* entries,
   SimpleIndex::EntrySet::iterator it = entries->find(hash_key);
   base::CheckedNumeric<uint32_t> total_entry_size = size;
 
+  // When the entry size is nonsense we can't use it, since don't want to
+  // crash, but we actually do want to proceed with the entry in the index: if
+  // it's not in index, and is never accessed again, then the file will never
+  // get removed. So we just make something up; if it's unused it will get

[pasko, 2017/05/11 12:15:07]

not really "never get removed", it will be removed after those unfortunate cache
dir rescans that we do when we loose the up to date index.

[morlovich, 2017/05/19 11:35:05]

It would be never removed if we decided to skip over it rather than include it
here, since this *is* the dir rescan.

+  // kicked out eventually; if it's used we will either delete the file due to
+  // file format error, or set this correctly.
+  const int kPlaceHolderSizeWhenInvalid = 32768;

[pasko, 2017/05/11 12:15:07]

I think it's worth mentioning that the entry size in the index is updated on
every successful entry open operation. I did not remember that. This is
important as it guarantees that we would not produce cache read errors from
perfectly valid entries.

I guess we could choose the placeholder to be the average size of a cache entry,
so that it minimizes the effect on the time when eviction kicks in. But it's all
rare and hence moot. Up to you.

[morlovich, 2017/05/19 11:35:05]

OK, will tweak the comment to make it more explicit once I get in the office.

[Maks Orlovich, 2017/05/19 13:18:06]

Hmm, thinking some more, maybe we should range-check the size in
SimpleSynchronousEntry::OpenFiles as well, since if it's also invalid there we
will do a lot of silly stuff.

[pasko, 2017/05/19 14:38:07]

ah, you are right. That basically brings us back to my comment of avoiding
effects of silly reads early. Feel free to land this crash mitigation separately
before the proper fix in SSE::OpenFiles materializes.

[Maks Orlovich, 2017/05/19 15:09:58]

True, that would save work, but the check is probably a good idea anyway.
Hmm, the actual fix there is probably just:
    if (!base::IsValueInRangeForNumericType<int>(file_info.size))
      return false;

Though would probably want a new UMA enum for OpenEntryResult. I think I'll just
do that, this isn't exactly a CL with things depending on it.

+  if (!total_entry_size.IsValid()) {
+    LOG(WARNING) << "Invalid file size while restoring index from disk: "
+                 << size << " on file:" << file_name;
+  }
+
   if (it == entries->end()) {
     SimpleIndex::InsertInEntrySet(
-        hash_key, EntryMetadata(last_used_time, total_entry_size.ValueOrDie()),
+        hash_key,
+        EntryMetadata(last_used_time, total_entry_size.ValueOrDefault(
+                                          kPlaceHolderSizeWhenInvalid)),
         entries);
   } else {
     // Summing up the total size of the entry through all the *_[0-1] files
     total_entry_size += it->second.GetEntrySize();
-    it->second.SetEntrySize(total_entry_size.ValueOrDie());
+    it->second.SetEntrySize(
+        total_entry_size.ValueOrDefault(kPlaceHolderSizeWhenInvalid));
   }
 }