74766 - Trashing memory in nsDiskCacheDevice::evictDiskCacheEntries()

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Simon Fraser [no longer active]]

evictDiskCacheEntries() seems to be running off the end of the sortedRecords 
array (which shows up as a trashed trailer marker in the memory allocators). This 
prevents me from running my build.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Stack of badness in delete[], if you care

Comment 2

[Simon Fraser [no longer active]]

sortedRecords is |mCacheMap->EntryCount()| big, |count| gets bigger than this, so 
we trash the array at 

  sortedRecords[count++] = *record;

Comment 3

[Simon Fraser [no longer active]]

This could easily cause smoketest-blocking crashes

Comment 4

[Patrick C. Beard]

This will happen when the _CACHE_MAP_ file gets corrupted. I'm working on changes 
in the branch that will make this less likely. I obviously need to make the 
current code more robust against this corruption. I'll come up with a minimal 
patch for now.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm seeing this crash frequently on Linux (debug build from yesterday, and
presumably the frequent crashes in my opt build are caused by the same thing).
This is keeping my mean time between failures down around 5-10 minutes, which
isn't exactly usable.

Comment 6

[Jeffrey Baker]

I was seeing this at launch on Linux 2001-04-05 build until I blew away my cache
directory.

Comment 7

[Patrick C. Beard]

This crash is fixed in the DISKCACHE2_BRANCH, which is now MUCH more robust 
against corruption/crashes/etc. I've bumped the cache version number, so in 
future, whenever there's an incompatible format change, the cache you have will 
be clobbered. There's also a dirty flag that gets written into the cache map file 
(_CACHE_MAP_) which gets cleared only if the cache is shutdown cleanly. When the 
disk cache starts up again, it will clobber the cache if the dirty flag is set. 
To try this build, pull like so:

cvs co -r DISKCACHE2_BRANCH mozilla/netwerk/cache/src
cvs co -r DISKCACHE2_BRANCH mozilla/netwerk/cache/public

We should be landing this today, if luck is with us.

Comment 8

[Daniel (Leaf) Nunes]

How sure are you that the branch isn't going to regress us further? Since this
isn't keeping the builds from being tested by qa (afaik), i'm inclined to
perform blake's carpool (which has been pushed back three days), and then have
you land your branch, if it's ready.

How extensive are the changes on the branch, and is it really ready to go in?

Comment 9

[gordon]

The branch is very small.  We are just treating it as a regular checkin, after 
getting an sr; we don't need a car pool.  We only have a branch so Patrick and I 
can share our work and verify it on multiple platforms before checking it into 
the trunk.  It's our method of team programming.  Maybe we should have spelled 
the branch name in all lower-case, so as not to arouse suspicion. :-)

Comment 10

[Daniel (Leaf) Nunes]

the sheriff would like this checked in before the tree opens... are you sr'd?

Comment 11

[gordon]

Yes. sr=darin.

Comment 12

[gordon]

Marking FIXED.

Comment 13

[Tom Everingham]

verified