Closed Bug 1003208 Opened 5 years ago Closed 5 years ago

HTTP cache v2: CacheFileHandle::mHash is accessed after the memory is freed

Categories

(Core :: Networking: Cache, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: michal, Assigned: michal)

Details

Attachments

(1 file)

Attached patch patch v1Splinter Review
We close all handles during shutdown in CacheFileIOManager::ShutdownInternal() and after this point all hashes are freed and no handle that lives longer must dereference its mHash member, since it points to a freed memory.

The hash was dereferenced by CacheFileHandle::Log() called from CacheFileIOManager::CloseHandleInternal(). We must not call CacheFileIOManager::CloseHandleInternal() from the destructor if the handle is already closed.

The patch fixes this and also nulls out mHash member so that the code crashes if there is yet another use of the freed memory.

https://tbpl.mozilla.org/?tree=Try&rev=650179bf6ebc
Attachment #8414516 - Flags: review?(honzab.moz)
Comment on attachment 8414516 [details] [diff] [review]
patch v1

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

Hmm.. kinda dirty fix.  Can you please file a bug for clean up and make it block cache2afterenable?
Attachment #8414516 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #1)
> Hmm.. kinda dirty fix.  Can you please file a bug for clean up and make it
> block cache2afterenable?

What exactly is dirty according to you and should be cleaned up?
(In reply to Michal Novotny (:michal) from comment #3)
> (In reply to Honza Bambas (:mayhemer) from comment #1)
> > Hmm.. kinda dirty fix.  Can you please file a bug for clean up and make it
> > block cache2afterenable?
> 
> What exactly is dirty according to you and should be cleaned up?

"Null out the pointer so that we crash if there is a bug in this code" - I think we should fix the code so that it can never crash.  This is low priority, but when we change something around here I think it can occasionally start null-crashing or be somehow racy and do bad memory access again.
https://hg.mozilla.org/mozilla-central/rev/0941e6502ee8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.