1141555 - HTTP cache v2 generates corrupted entries

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

Analyzing of logs generated for bug 1139014 showed that we failed to read about 3.6% of entries that were previously written during the same session. Parsing failed with "Metadata hash mismatch" or "Invalid realOffset" error.

This happens when an existing entry is updated and the new metadata is smaller. The file is not truncated and the excessive part of the old metadata makes the entry invalid.

We also don't truncate the file when parsing of the entry fails, so we're unable to write a new correct entry if it is smaller than the invalid one.

Comment 1

[Michal Novotny [:michal]]

Attachment: patch v1

Changes made by this patch:

- TruncFile() now takes int64_t
- CacheFileIOManager::Write() has new argument aTruncate, so CacheFileMetadata can truncate the file correctly
- entry file is truncated when parsing of the entry fails and we're creating a new entry
- CacheFileIOManager::TruncateSeekSetEOFInternal() now respects browser.cache.disk.free_space_hard_limit
- CacheFileIOManager::TruncateSeekSetEOFInternal() updates entry's size in index

Comment 2

[Michal Novotny [:michal]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af3301a4f125

Comment 3

[Jason Duell]

Comment on attachment 8575331 [details] [diff] [review]
patch v1

Review of attachment 8575331 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8575331 [details] [diff] [review]
patch v1

Review of attachment 8575331 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +1902,5 @@
> +    NS_ERROR("SetEndOfFile failed");
> +    return NS_ERROR_FAILURE;
> +  }
> +#else
> +  MOZ_ASSERT(false, "Not implemented!");

Shouldn't we STATIC_ASSERT here?

Comment 5

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b374964865c

Comment 6

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> > +#else
> > +  MOZ_ASSERT(false, "Not implemented!");
> 
> Shouldn't we STATIC_ASSERT here?

I've added return NS_ERROR_NOT_IMPLEMENTED here so the function fails in a release build.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b374964865c