1814217 - Crash releasing RefPtr<mozilla::net::HttpTransactionShell> in [@ mozilla::net::nsHttpChannel::~nsHttpChannel ]

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Randell Jesup [:jesup] (needinfo me)]

Crash releasing RefPtr<mozilla::net::HttpTransactionShell>, https://crash-stats.mozilla.org/report/index/b6d53475-f344-4f03-ba2a-9b2790230130

e5e5 address, not sure how that apparently got into the RefPtr or if the memory for the member got mass-overwritten.

+++ This bug was initially created as a clone of Bug #1544127 +++

Comment 1

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:jesup, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

I'll take a look into the crash dumps for some clue about what's wrong.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

I was looking at this bug for a while.
One thing I noticed is that mEarlyHintObserver and mChannelClassifier are not thread safe. However, these should be released before the ~nsHttpChannel destuctor - when the call to ReleaseListeners happens from OnStopRequest.

There are a few of the crashes that point to freed memory.
https://crash-stats.mozilla.org/report/index/e633a6a0-1e67-4da7-ada4-031840230208
https://crash-stats.mozilla.org/report/index/2fde231f-ebc7-4d9c-abc8-cfd690230221
https://crash-stats.mozilla.org/report/index/b6d53475-f344-4f03-ba2a-9b2790230130
Some of the Android ones report Available Page File: 0

So far this seems to be similar to bug 1544127.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1814217 - Dispatch release of main thread objects in ~nsHttpChannel r=#necko

The patch also adds a diagnostic assert that the objects have actually been released in nsHttpChannel::ReleaseListeners

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9321370 [details]
Bug 1814217 - Dispatch release of main thread objects in ~nsHttpChannel r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This is more of a speculative patch. We now dispatch the release of non-threadsafe objects to the main thread instead.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• 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?: Yes
• If not, how different, hard to create, and risky will they be?: Applies cleanly to ESR 102.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely to cause regressions - with the exception of early beta and nightly where we intentionally diagnostic assert to be able to detect such cases and fix the root cause.
• Is Android affected?: Yes

Comment 6

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

Comment on attachment 9321370 [details]
Bug 1814217 - Dispatch release of main thread objects in ~nsHttpChannel r=#necko

Approved to land and uplift if desired.

Comment 7

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

Dispatch release of main thread objects in ~nsHttpChannel r=necko-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/dbdfcfadb6e36730be99415f356c4a7cafa6a11c
https://hg.mozilla.org/mozilla-central/rev/dbdfcfadb6e3

Comment 8

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

The patch landed in nightly and beta is affected.
:valentin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9321370 [details]
Bug 1814217 - Dispatch release of main thread objects in ~nsHttpChannel r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Possible UAF
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Doesn't cause a behaviour change - just dispatches release of main thread objects if nsHttpChannel is released off-main-thread.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9321370 [details]
Bug 1814217 - Dispatch release of main thread objects in ~nsHttpChannel r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Possible UAF
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Doesn't cause a behaviour change - just dispatches release of main thread objects if nsHttpChannel is released off-main-thread.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 11

[Valentin Gosu [:valentin] (he/him)]

The few crashes with this signature on ESR 102 seem to all be bitflips:

https://crash-stats.mozilla.org/report/index/92a34638-d328-46b3-81a5-befb10230314
https://crash-stats.mozilla.org/report/index/dcc89eec-c35b-48bf-b01f-293220230312
https://crash-stats.mozilla.org/report/index/adf6d111-d8c7-43af-ae4c-1db0a0230315

Comment 12

[Dianna Smith [:diannaS]]

Comment on attachment 9321370 [details]
Bug 1814217 - Dispatch release of main thread objects in ~nsHttpChannel r=#necko

Approved for 112.0b3

Comment 13

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5892068efe45

Comment 14

[Dianna Smith [:diannaS]]

Comment on attachment 9321370 [details]
Bug 1814217 - Dispatch release of main thread objects in ~nsHttpChannel r=#necko

Approved for 102.10esr

Comment 15

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/9aa1db544ca9