1248958 - CacheIndex mRWBuf ownership too fragile, read-after-free

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/logviewer.html#?job_id=16831081&repo=try (includes log, 3b053cc6-70be-4be8-ac2b-ce2f874677c6)
https://bugzilla.mozilla.org/show_bug.cgi?id=1247432#c7

With bug 1247432's patch we are getting to the following situation:

- CacheIndex::WriteRecords() allocates mRWBuf and gives it the io manager to write
- later CacheIndex::FinishWrite() is called as part of PreShutdownInternal call (on the cache thread)
- finally CacheFileIOManager::WriteInternal for the buffer is executed, already on a freed buffer


2016-02-17 13:00:09.663842 UTC - [Cache2 I/O]: D/cache2 CacheIndex::WriteRecords()
2016-02-17 13:00:09.664074 UTC - [Cache2 I/O]: D/cache2 CacheFileIOManager::Write() [handle=608000245f20, offset=0, count=13408, validate=1, truncate=0, listener=613000064500]
  buffer passed down to be written


2016-02-17 13:00:09.685871 UTC - [Main Thread]: D/cache2 CacheStorageService::Shutdown - done
2016-02-17 13:00:09.687958 UTC - [Main Thread]: D/cache2 CacheFileIOManager::Shutdown() [gInstance=6110000efb40]
2016-02-17 13:00:09.687995 UTC - [Main Thread]: D/cache2 CacheIndex::PreShutdown() [gInstance=613000064500]
2016-02-17 13:00:09.688009 UTC - [Main Thread]: D/cache2 CacheIndex::PreShutdown() - [state=2, indexOnDiskIsValid=1, dontMarkIndexClean=0]
2016-02-17 13:00:09.688100 UTC - [Main Thread]: D/cache2 CacheIndex::PreShutdown() - Closing iterators.
  PreShutdownInternal scheduled (XPCOM priority = sooner than write)


Maybe the simplest solution is to post this to the WRITE level?


2016-02-17 13:00:09.689942 UTC - [Cache2 I/O]: D/cache2 CacheIndex::PreShutdownInternal() - [state=2, indexOnDiskIsValid=1, dontMarkIndexClean=0]
2016-02-17 13:00:09.689956 UTC - [Cache2 I/O]: D/cache2 CacheIndex::FinishWrite() [succeeded=0]
  this also releases the buffer


2016-02-17 13:00:09.748298 UTC - [Cache2 I/O]: D/cache2 CacheFileIOManager::WriteInternal() [handle=608000245f20, offset=0, count=13408, validate=1, truncate=0]
2016-02-17 13:00:09.748321 UTC - [Cache2 I/O]: D/cache2 CacheFileIOManager::OpenNSPRHandle BEGIN, handle=608000245f20
2016-02-17 13:00:09.748376 UTC - [Cache2 I/O]: D/cache2 CacheFileIOManager::OpenNSPRHandle END, handle=608000245f20
  => crash, read-after-free

Comment 1

[Honza Bambas (:mayhemer)]

Ups..  What did my subconscious want to tell me?

Comment 2

[Michal Novotny [:michal]]

Attachment: fix

(In reply to Honza Bambas (:mayhemer) from comment #0)
> CacheIndex::PreShutdown() [gInstance=613000064500]
> 2016-02-17 13:00:09.688009 UTC - [Main Thread]: D/cache2
> CacheIndex::PreShutdown() - [state=2, indexOnDiskIsValid=1,
> dontMarkIndexClean=0]
> 2016-02-17 13:00:09.688100 UTC - [Main Thread]: D/cache2
> CacheIndex::PreShutdown() - Closing iterators.
>   PreShutdownInternal scheduled (XPCOM priority = sooner than write)
> 
> 
> Maybe the simplest solution is to post this to the WRITE level?

Yes, this seems to be the simplest solution. Other would be to release the buffer in OnDataRead/OnDataWritten instead of FinishRead/FinishWrite, but I prefer changing the level.

Comment 3

[Honza Bambas (:mayhemer)]

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

Comment 4

[Honza Bambas (:mayhemer)]

Michal, seem OK on TSan (no try -a run tho).  Will you land please?

Comment 5

[Michal Novotny [:michal]]

Attachment: patch v2

I just removed the last sentence from the comment, because if we interrupt the update process we're not going to write journal to disk.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09b52ee53960

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09b52ee53960

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8720739 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: none, but is a prerequisity for bug 1247432
[User impact if declined]: after landing an important bug 1247432 it may lead to crash or store of a freed memory to disk that will be used on next browser start
[Describe test coverage new/current, TreeHerder]: this code is executed on every shutdown, landed on m-c a while ago
[Risks and why]: seems none
[String/UUID change made/needed]: none

IMPORTANT: Land on m-a before bug 1247432 !!!

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected for 46

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8720739 [details] [diff] [review]
patch v2

Needed for uplift of shutdown hang bug 1247432.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/604f765ddad8