HTTP cache v2 generates corrupted entries

RESOLVED FIXED in Firefox 39

Status

()

defect
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

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.
Posted patch patch v1Splinter Review
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
Attachment #8575331 - Flags: review?(honzab.moz)
Attachment #8575331 - Flags: review?(honzab.moz) → review?(jduell.mcbugs)
Comment on attachment 8575331 [details] [diff] [review]
patch v1

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

Looks good.
Attachment #8575331 - Flags: review?(jduell.mcbugs) → review+
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?
(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.
https://hg.mozilla.org/mozilla-central/rev/1b374964865c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.