1644239 - Crash in [@ mozilla::net::Http2Session::RecvPushPromise]

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug is for crash report bp-21789f40-8277-44cf-bb4e-709920200608.

Top 10 frames of crashing thread:

0 xul.dll static mozilla::net::Http2Session::RecvPushPromise netwerk/protocol/http/Http2Session.cpp:2074
1 xul.dll mozilla::net::Http2Session::WriteSegmentsAgain netwerk/protocol/http/Http2Session.cpp:3412
2 xul.dll mozilla::net::nsHttpConnection::OnSocketReadable netwerk/protocol/http/nsHttpConnection.cpp:2153
3 xul.dll mozilla::net::nsHttpConnection::OnInputStreamReady netwerk/protocol/http/nsHttpConnection.cpp:2506
4 xul.dll mozilla::net::nsSocketInputStream::OnSocketReady netwerk/base/nsSocketTransport2.cpp:286
5 xul.dll mozilla::net::nsSocketTransport::OnSocketReady netwerk/base/nsSocketTransport2.cpp:2280
6 xul.dll mozilla::net::nsSocketTransportService::Run netwerk/base/nsSocketTransportService2.cpp:1094
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1211
8 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:332
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308

this browser crash signature is getting more frequent platforms once firefox 78 hit the beta cycle.

Comment 1

[Honza Bambas (:mayhemer)]

Dragana, Michal, did anything here change lately?

Comment 2

[Dragana Damjanovic [:dragana]]

There is bug 1627533 and a follow up bug 1641167.

I will look into 1641167 to see if it is going to fix this one.

Comment 3

[Dragana Damjanovic [:dragana]]

Kershaw, I think this is regression from 1546358?

Comment 4

[Kershaw Chang [:kershaw]]

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

Kershaw, I think this is regression from 1546358?

Right. I think I accidentally removed the null check added in https://phabricator.services.mozilla.com/D33945.

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Bug 1644239, r=dragana

Comment 6

[Kershaw Chang [:kershaw]]

Comment on attachment 9157951 [details]
Bug 1644239, r=dragana

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Probably quite easily.
• 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?: 78
• If not all supported branches, which bug introduced the flaw?: Bug 1546358
• 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?: Not likely. This patch is just adding a null check.

Comment 7

[Daniel Veditz [:dveditz]]

I believe this is just a stability regression. If the value is null then we immediate try to write to (null + small fixed offset) and crash harmlessly. You don't need sec-approval, but we may still want to uplift it because it's a regression and a stability issue.

Comment 8

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

https://hg.mozilla.org/integration/autoland/rev/69e5de33bd0797d51b019982741eb927920dd471

Comment 9

[Kershaw Chang [:kershaw]]

Comment on attachment 9157951 [details]
Bug 1644239, r=dragana

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This crash is easily to be triggered.
• User impact if declined: Firefox could crash when receiving a H2 push response.
• Fix Landed on Version: 79
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch is just a simple null check.
• String or UUID changes made by this patch: N/A

Beta/Release Uplift Approval Request

• User impact if declined: Firefox could crash when receiving a H2 push response.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: N/A
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch is just a simple null check.
• String changes made/needed: N/A

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9157951 [details]
Bug 1644239, r=dragana

I'll keep this on the radar in the event of a RC respin.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/69e5de33bd07

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9157951 [details]
Bug 1644239, r=dragana

crash fix for 78 rc2

Comment 13

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/aecfc014ba0c
https://hg.mozilla.org/releases/mozilla-esr78/rev/aecfc014ba0c7e748cc32423c0ee94e62d5acfea