1979955 - Crash in [@ mozilla::net::Http2Session::RemoveStreamFromTables]

Status: RESOLVED FIXED

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

Description

[Ryan VanderMeulen [:RyanVM]]

Crash report: https://crash-stats.mozilla.com/report/index/cd182de3-ade4-4c71-99b7-92af10250729

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  xul.dll  PLDHashTable::ComputeKeyHash(void const*) const  xpcom/ds/PLDHashTable.cpp:479
0  xul.dll  PLDHashTable::Search(void const*) const  xpcom/ds/PLDHashTable.cpp:499
1  xul.dll  nsTHashtable<nsBaseHashtableET<nsPtrHashKey<mozilla::net::nsAHttpTransaction>...  xpcom/ds/nsTHashtable.h:290
1  xul.dll  nsRefCountedHashtable<nsPtrHashKey<mozilla::net::nsAHttpTransaction>, RefPtr<...  xpcom/ds/nsRefCountedHashtable.h:229
1  xul.dll  mozilla::net::Http2Session::RemoveStreamFromTables(mozilla::net::Http2StreamB...  netwerk/protocol/http/Http2Session.cpp:1417
1  xul.dll  mozilla::net::Http2Session::CleanupStream(mozilla::net::Http2StreamBase*, nsr...  netwerk/protocol/http/Http2Session.cpp:1386
2  xul.dll  mozilla::net::Http2Session::ReadSegmentsAgain(mozilla::net::nsAHttpSegmentRea...  netwerk/protocol/http/Http2Session.cpp:2717
3  xul.dll  mozilla::net::nsHttpConnection::OnSocketWritable()  netwerk/protocol/http/nsHttpConnection.cpp:1715
4  xul.dll  mozilla::net::nsHttpConnection::OnOutputStreamReady(nsIAsyncOutputStream*)  netwerk/protocol/http/nsHttpConnection.cpp:2262
5  xul.dll  mozilla::net::nsSocketOutputStream::OnSocketReady(nsresult)  netwerk/base/nsSocketTransport2.cpp:528

Comment 1

[Ryan VanderMeulen [:RyanVM]]

This appears to still be crashing with a UAF pattern after bug 1976376 landed.

Comment 2

[Kershaw Chang [:kershaw]]

A short summery:

1. We are retrieving a WeakPtr<Http2StreamBase> from a queue and promoting it to a RefPtr:

 RefPtr<Http2StreamBase> stream =
      mQueueManager.GetNextStreamFromQueue(Http2StreamQueueType::ReadyForWrite);

2. Then, this line failed

rv = stream->ReadSegments(this, count, countRead);

3. We call CleanupStream because rv is an error

CleanupStream(stream, rv, CANCEL_ERROR);

4. Inside CleanupStream, we call RemoveStreamFromTables and we do:

 mStreamTransactionHash.Remove(aStream->Transaction());

The crash seems to happen here, but it's not clear which object is destroyed. The stream should be kept alive by the RefPtr in step 1 and the Http2Session should be also alive at this point.

I’m currently out of ideas and could really use a fresh pair of eyes. Randell, would you mind taking a look? Thanks!

Comment 3

[Andrew McCreight [:mccr8]]

Any chance that nsHttpConnection::mTransaction could get mutated somewhere in here? That is often a problem in DOM code, so it is good to avoid complex method calls on data members. I don't know if that's a similar concern in networking code. (The fix for that problem is to create a strong stack reference to the transaction, and do the call on that.)

Comment 4

[Kershaw Chang [:kershaw]]

(In reply to Andrew McCreight [:mccr8] from comment #3)

Any chance that nsHttpConnection::mTransaction could get mutated somewhere in here? That is often a problem in DOM code, so it is good to avoid complex method calls on data members. I don't know if that's a similar concern in networking code. (The fix for that problem is to create a strong stack reference to the transaction, and do the call on that.)

I think this is a good suggestion and worth trying. Let me write a patch for this.
Thanks.

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: (secure)

Comment 6

[Kershaw Chang [:kershaw]]

Comment on attachment 9504123 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unknown. This patch only ensures nsHttpConnection::mTransaction is alive when executing WriteSegment/ReadSegment.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: 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?:
• How likely is this patch to cause regressions; how much testing does it need?: Low. This patch is straightforward.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 7

[Tom Ritter [:tjr]]

Should this patch be marked leave-open if we need to come back and confirm the fix? Also, does this code run in the parent or socket process? If so, it should be keyworded as csectype-sandbox-escape

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6a97d603b644
https://hg.mozilla.org/integration/autoland/rev/5ec5a52c53a6
ensure transaction is alive, r=necko-reviewers,jesup

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/5ec5a52c53a6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

No Nightly crashes since this landed when there used to be a semi-steady trickle of them basically every day. How are we feeling about declaring victory here? :-)

Comment 11

[Kershaw Chang [:kershaw]]

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

No Nightly crashes since this landed when there used to be a semi-steady trickle of them basically every day. How are we feeling about declaring victory here? :-)

I'd wait a few more days, but I think we can uplift this low-risk patch to beta and ESR first.

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D259321

Comment 13

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Possible crash
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: This patch is straightforward.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D259321

Comment 15

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Possible crash
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: This patch is really simple.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 16

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #11)

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

No Nightly crashes since this landed when there used to be a semi-steady trickle of them basically every day. How are we feeling about declaring victory here? :-)

I'd wait a few more days, but I think we can uplift this low-risk patch to beta and ESR first.

To clarify, I meant we can leave this bug open for a few more days before closing it.

Comment 17

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/98c574403c3f
https://hg.mozilla.org/releases/mozilla-beta/rev/9db036e8770b

Comment 18

[Daniel Veditz [:dveditz]]

Going to add this to the advisories list assuming we will close this in the next couple of days. So far so good.

Comment 19

[Kershaw Chang [:kershaw]]

Attachment: (secure)

Comment 20

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Possible crash
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: This patch is simple: adding null check and holding another reference.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/599f65f07573