CacheIndex::ParseRecords() a possible bug

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dragana, Assigned: michal)

Tracking

unspecified
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

2108   if (pos != mRWBufPos) {
2109     memmove(mRWBuf, mRWBuf + pos, mRWBufPos - pos);
2110     mRWBufPos -= pos;
2111     pos = 0;
2112   }
2113 
2114   int64_t fileOffset = sizeof(CacheIndexHeader) +
2115                        mSkipEntries * sizeof(CacheIndexRecord) + mRWBufPos;

I think it is a bug.
If pos == mRWBufPos the fileoffset will be 2 times what it should be.
can we guaranty that they are always different?

the size of pos will be sizeof(CacheIndexHeader) + mSkipEntries * sizeof(CacheIndexRecord) before this lines.
Yes, this is a bug. But luckily it is never triggered because of buffer size and sizeof(CacheIndexHeader). When we have all remaining data in the buffer there is always a hash at the end, so pos is mRWBufPos - 4. When this is a beginning of the file, we parse the header, 454 entries and 28 bytes is left unparsed. All other ParseRecords() iterations parse 455 entries and 4 bytes is always left unparsed.
Assignee: nobody → michal.novotny
Posted patch fixSplinter Review
Attachment #8542564 - Flags: review?(honzab.moz)
Comment on attachment 8542564 [details] [diff] [review]
fix

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

I can only trust you on this.  All the ptr arithmetic in this code should one nice day [ :)) ] go to a nice class so that we won't need to worry...
Attachment #8542564 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/2ba79118e686
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.