1662676 - Crash in [@ nsTArray_Impl<T>::Compare<T> | NS_QuickSort | XUL@0x79486f | mozilla::net::CacheIndex::FrecencyArray::SortIfNeeded]

Status: RESOLVED FIXED

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

Description

[Simon Giesecke [:sg] [he/him]]

Crash report: https://crash-stats.mozilla.org/report/index/d634ed01-a060-4993-a124-2734e0200901

Top 10 frames of crashing thread:

0 XUL int nsTArray_Impl<mozilla::net::CacheIndexRecord*, nsTArrayInfallibleAllocator>::Compare<detail::CompareWrapper<mozilla::net:: xpcom/ds/nsTArray.h:2255
1 XUL NS_QuickSort xpcom/ds/nsQuickSort.cpp:117
2 XUL XUL@0x79486f 
3 XUL mozilla::net::CacheIndex::FrecencyArray::SortIfNeeded netwerk/cache2/CacheIndex.cpp:3401
4 XUL mozilla::net::CacheIndexEntryAutoManage::~CacheIndexEntryAutoManage netwerk/cache2/CacheIndex.cpp:108
5 XUL mozilla::net::CacheIndex::MergeJournal netwerk/cache2/CacheIndex.cpp:95
6 XUL mozilla::net::CacheIndex::FinishRead netwerk/cache2/CacheIndex.cpp:2537
7 XUL mozilla::net::CacheIndex::OnDataRead netwerk/cache2/CacheIndex.cpp:3667
8 XUL mozilla::net::ReadEvent::Run netwerk/cache2/CacheFileIOManager.cpp:700
9 XUL mozilla::net::CacheIOThread::LoopOneLevel netwerk/cache2/CacheIOThread.cpp:538

For a Windows crash report I see these inline frames with VS:

[Inline Frame] xul.dll!mozilla::net::`anonymous namespace'::FrecencyComparator::Equals(mozilla::net::CacheIndexRecord * a, mozilla::net::CacheIndexRecord * b) Line 47
	at /builds/worker/checkouts/gecko/netwerk/cache2/CacheIndex.cpp(47)
[Inline Frame] xul.dll!detail::CompareWrapper<mozilla::net::(anonymous namespace)::FrecencyComparator,mozilla::net::CacheIndexRecord *,0>::Equals(mozilla::net::CacheIndexRecord * const & aLeft, mozilla::net::CacheIndexRecord * const & aRight) Line 928
	at /builds/worker/workspace/obj-build/dist/include/nsTArray.h(928)
[Inline Frame] xul.dll!detail::CompareWrapper<mozilla::net::(anonymous namespace)::FrecencyComparator,mozilla::net::CacheIndexRecord *,0>::Compare(mozilla::net::CacheIndexRecord * const & aLeft, mozilla::net::CacheIndexRecord * const & aRight) Line 920
	at /builds/worker/workspace/obj-build/dist/include/nsTArray.h(920)
[Inline Frame] xul.dll!nsTArray_Impl<mozilla::net::CacheIndexRecord *,nsTArrayInfallibleAllocator>::Compare(const void * aE1, const void * aE2, void * aData) Line 2300
	at /builds/worker/workspace/obj-build/dist/include/nsTArray.h(2300)
xul.dll!NS_QuickSort(void * a, unsigned int n, unsigned int es, int(*)(const void *, const void *, void *) cmp, void * data) Line 134
	at /builds/worker/checkouts/gecko/xpcom/ds/nsQuickSort.cpp(134)
[Inline Frame] xul.dll!nsTArray_Impl<mozilla::net::CacheIndexRecord *,nsTArrayInfallibleAllocator>::Sort(const mozilla::net::`anonymous namespace'::FrecencyComparator & aComp) Line 2310
	at /builds/worker/workspace/obj-build/dist/include/nsTArray.h(2310)
xul.dll!mozilla::net::CacheIndex::FrecencyArray::SortIfNeeded() Line 3401
	at /builds/worker/checkouts/gecko/netwerk/cache2/CacheIndex.cpp(3401)

This looks like a UAF on some of the elements within FrecencyArray::mRecs

Comment 1

[Jens Stutte [:jstutte]]

Not sure at all, if this is a possible cause for these crashes, but many functions in CacheIndex do an sLock.AssertCurrentThreadOwns(); effective only in debug mode instead of handling multithreaded access at runtime. CacheIndex itself declares itself to be NS_DECL_THREADSAFE_ISUPPORTS, which might be a hint that it could be used on (or at least destroyed from) different threads?

Comment 2

[Kershaw Chang [:kershaw]]

Attachment: Bug 1662676

Comment 3

[Kershaw Chang [:kershaw]]

Comment on attachment 9193039 [details]
Bug 1662676

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, since this is basically a diagnostic patch. We don't know how to reproduce this crash currently.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: ESR 78
• If not all supported branches, which bug introduced the flaw?: N/A
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The crash rate is low, but this is a UAF.
• How likely is this patch to cause regressions; how much testing does it need?: Should be low, but we still add a pref to control whether to enable this diagnostic check.

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9193039 [details]
Bug 1662676

I think this should probably be nightly only for now, approved to land.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/9b4ab01fda40b7bdc73d6850d5152631fa912730

Comment 6

[Atila Butkovits]

Backed out for gv-junit failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/a31fbf5a409f59a5b8994a17d0f77bfb2f444e3a

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=android%2C7.0%2Cx86-64%2Copt%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-junit-e10s-multi-e10s%2Cgv-junit-e10s-multi&revision=9b4ab01fda40b7bdc73d6850d5152631fa912730&selectedTaskRun=YyZvhu0rSYelwsxFseo2qA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=325150505&repo=autoland&lineNumber=15020

Comment 7

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/jobs?repo=try&revision=514b1cffeb6dbe0335c6d7fff8fe472ed236880f

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/266e441e2f1694fe4bc530c7796b75fd053e96c9

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/266e441e2f16

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Bug 1662676 - Enable diagnostic assertion on early beta

Comment 11

[Kershaw Chang [:kershaw]]

Comment on attachment 9197832 [details]
Bug 1662676 - Enable diagnostic assertion on early beta

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, since this is basically a diagnostic patch. We don't know how to reproduce this crash currently.
  Note this patch only changes the pref to enable this diagnostic assertion on early beta, but it's possible to find out the security problem by looking into the code related to this pref.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: ESR78
• If not all supported branches, which bug introduced the flaw?: N/A
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The crash rate is low, but this is a UAF.
• How likely is this patch to cause regressions; how much testing does it need?: Should be low. The previous patch is already on nightly for a while, but we didn't catch the problem on nightly. Therefore, I'd like to let this diagnostic assertion be enabled on early beta.

Comment 12

[Kershaw Chang [:kershaw]]

Tom, could you take a look at this sec-approval?
Thanks.

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I'll leave the NI; but we don't land anything during code freeze/this close tot he release so sec-approvals tend to pause during that time and pick back up once the merge is done.

Comment 14

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9197832 [details]
Bug 1662676 - Enable diagnostic assertion on early beta

Approved to land and uplift to Beta (no point to letting this site in Nightly for an entire cycle to ride the trains.)

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Enable diagnostic assertion on early beta r=dragana DONTBUILD
https://hg.mozilla.org/integration/autoland/rev/1d444bdee7ec2e2991b26a983073a6557ab33c1d
https://hg.mozilla.org/mozilla-central/rev/1d444bdee7ec

Comment 16

[Kershaw Chang [:kershaw]]

Hi Sebastian,

Could you also uplift this patch to beta?

Thanks.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

RelMan will take care of the uplift.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 9197832 [details]
Bug 1662676 - Enable diagnostic assertion on early beta

https://hg.mozilla.org/releases/mozilla-beta/rev/27fb93caaa4a

[clearing uplift flag to get this off the needs-uplift queries]

Comment 19

[Kershaw Chang [:kershaw]]

Attachment: Bug 1662676 - Add diagnostic assertion at more places

Comment 20

[Kershaw Chang [:kershaw]]

Comment on attachment 9206373 [details]
Bug 1662676 - Add diagnostic assertion at more places

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, since this is a diagnostic patch. It's unclear why this crash happens.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: ESR78
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The crash rate is low, but this is a UAF.
• How likely is this patch to cause regressions; how much testing does it need?: Should be low. This patch adds MOZ_DIAGNOSTIC_ASSERT at more places.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 9206373 [details]
Bug 1662676 - Add diagnostic assertion at more places

sec-approval=dveditz

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Add diagnostic assertion at more places r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/6bd907c1fc39e6f5fd9919305bdaf00352dceec7
https://hg.mozilla.org/mozilla-central/rev/6bd907c1fc39

Comment 23

[Kershaw Chang [:kershaw]]

Attachment: Bug 1662676 - Wrap CacheIndexRecord, r=#necko

Comment 24

[Kershaw Chang [:kershaw]]

Comment on attachment 9217727 [details]
Bug 1662676 - Wrap CacheIndexRecord, r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it's not easy. We still don't know what triggers this crash.
  This patch basically avoids accessing the raw pointer of CacheIndexRecord.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: ESR 78
• If not all supported branches, which bug introduced the flaw?: N/A
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This crash should not be easy to reproduce.
• How likely is this patch to cause regressions; how much testing does it need?: The risk should be low, since this patch only introduces a wrapper to CacheIndexRecord. The current behavior is not changed.

Comment 25

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9217727 [details]
Bug 1662676 - Wrap CacheIndexRecord, r=#necko

Approved to land. I'll let you decide if you want to request uplift or if you want to let ir run in Nightly first.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/dc1a00f252c842d6e4315e5aec5dd43a801d9991

Backed out for cpp failures in CacheIndexRecordWrapper:

https://hg.mozilla.org/integration/autoland/rev/d8b9b9ecf422bb9255edb229fa090b80f94a3a3e

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=cv-F9ktpQ22sWvFz0uxfjw.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=dc1a00f252c842d6e4315e5aec5dd43a801d9991
Failure log: https://treeherder.mozilla.org/logviewer?job_id=339903615&repo=autoland
netwerk/cache2/CacheIndexIterator.h:43:26: error: unknown type name 'CacheIndexRecordWrapper'; did you mean 'CacheIndexRecord'?
and more failures

Comment 27

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/jobs?repo=try&revision=15b9308f2bb05691c4568d6bdbc02844a5a4bf22

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Wrap CacheIndexRecord, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/41aa644c25e903647d39812e8aa7fd928c659292
https://hg.mozilla.org/mozilla-central/rev/41aa644c25e9

Comment 29

[Ryan VanderMeulen [:RyanVM]]

IMO, we should let this ride 90 to Beta and then make a decision about ESR uplift rather than trying to get it into 89/78.11esr this cycle. Do you agree, Kershaw?

Comment 30

[Kershaw Chang [:kershaw]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #29)

IMO, we should let this ride 90 to Beta and then make a decision about ESR uplift rather than trying to get it into 89/78.11esr this cycle. Do you agree, Kershaw?

Yes, I agree. Since the crash rate is not high and we already live with this issue for a long time, I think we can let this ride the train.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 32

[Kershaw Chang [:kershaw]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #31)

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Sorry that I can't fill this form, since I still don't understand the root cause of this crash. The landed patch is some kind of workaround.

Comment 33

[Kershaw Chang [:kershaw]]

Unfortunately, we have to reopen this bug, since there are still two crashes in beta.
Even though we already make CacheIndexRecord ref-counted by wrapping it in CacheIndexRecordWrapper, we sill have this crash. I think this means that there is a race on the ref-count of CacheIndexRecordWrapper.

Comment 34

[Kershaw Chang [:kershaw]]

Attachment: Bug 1662676 - Use a proof of lock everywhere, r=#necko

Comment 35

[Kershaw Chang [:kershaw]]

Comment on attachment 9227663 [details]
Bug 1662676 - Use a proof of lock everywhere, r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, since this crash is caused by a race on a ref-counter.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The crash rate is not high, so the risk of not uplifting should be low.
• How likely is this patch to cause regressions; how much testing does it need?: Low risk. This patch doesn't change any logic. It only forces every AddRef/Release on CacheIndexRecordWrapper locked.

Comment 36

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9227663 [details]
Bug 1662676 - Use a proof of lock everywhere, r=#necko

Approved to land

Comment 37

[Julien Cristau [:jcristau]]

Use a proof of lock everywhere, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/cb11ebf5f319eefad2895f755fb887dc3cf03fec

Comment 38

[Julien Cristau [:jcristau]]

[Tracking Requested - why for this release]: we're getting close to 90 and this was reopened, so we should probably track for a later esr release.

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cb11ebf5f319

Comment 40

[Pascal Chevrel:pascalc (PTO until April 26)]

Is that a patch we should uplift in esr 78.13?

Comment 41

[Dragana Damjanovic [:dragana]]

I will ask for a uplift.

Comment 42

[Kershaw Chang [:kershaw]]

Attachment: Bug 1662676 - For ESR uplift, r=#necko

Comment 43

[Kershaw Chang [:kershaw]]

(In reply to Dragana Damjanovic [:dragana] from comment #41)

I will ask for a uplift.

The original patches should not be able to applied cleanly on esr 78, so I made another patch for uplift.

Comment 44

[Kershaw Chang [:kershaw]]

Comment on attachment 9233393 [details]
Bug 1662676 - For ESR uplift, r=#necko

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-high bug.
• User impact if declined: Possible UAF.
• Fix Landed on Version: 91
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch is straightforward. It doesn't change any logic.
• String or UUID changes made by this patch:

Comment 45

[Dragana Damjanovic [:dragana]]

Thank you.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9233393 [details]
Bug 1662676 - For ESR uplift, r=#necko

Approved for 78.13esr.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/fffc73b23669