1625749 - Crash in [@ mozilla::SlicedInputStream::Length]

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug is for crash report bp-9ae6ca03-bd87-4681-83b6-82c850200326.

Top 10 frames of crashing thread:

0 xul.dll mozilla::SlicedInputStream::Length xpcom/io/SlicedInputStream.cpp:584
1 xul.dll static mozilla::InputStreamLengthHelper::GetSyncLength xpcom/io/InputStreamLengthHelper.cpp:78
2 xul.dll mozilla::net::HttpChannelParent::DoAsyncOpen netwerk/protocol/http/HttpChannelParent.cpp:527
3 xul.dll mozilla::net::HttpChannelParent::Init netwerk/protocol/http/HttpChannelParent.cpp:135
4 xul.dll mozilla::net::NeckoParent::RecvPHttpChannelConstructor netwerk/ipc/NeckoParent.cpp:301
5 xul.dll mozilla::net::PNeckoParent::OnMessageReceived ipc/ipdl/PNeckoParent.cpp:1857
6 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:6314
7 xul.dll mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2137
8 xul.dll mozilla::ipc::MessageChannel::RunMessage ipc/glue/MessageChannel.cpp:1976
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1220

this crash signature is spiking up in firefox release since version 74 & it's happening across platforms. many comments are saying they were uploading large files when the crash took place - https://www.swisstransfer.com is a url that's frequently showing up in reports.

Comment 1

[Andrew McCreight [:mccr8]]

The crash is happening inside HttpChannelParent::DoAsyncOpen(), at least on the handful of crashes I looked at.

Comment 2

[Julien Cristau [:jcristau]]

Nhi, please find someone to look at this.

Comment 4

[Pascal Chevrel:pascalc]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1628624 there are STRs which consists or creating an account on https://tales.harpy.gg/ and upload this image:

https://cdn.discordapp.com/attachments/697712641698955265/697713242880868402/Lucky_You.png

This was reported publicly here:
https://forums.mozfr.org/viewtopic.php?f=5&t=143720&p=900574#p900574

This person provided a crash report with a this comment (same explanations than in the forums:

In Happens when I try to select a file after clicking an input file.
This image triggers the bug: https://cdn.discordapp.com/attachments/695950877462233138/695951131700101160/sirene.png
This one doesn’t: https://cdn.discordapp.com/attachments/695950877462233138/695951220229275669/sirene_300.png

Tried on my local env and I don’t have the bug.
Tried on Firefox 72 and no bug (only got it after updating to FF 75)

Also the main difference between my local env and the prod is that I have a service worker in production.

Comment 5

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-high with publicly reported possible STR

Comment 6

[Nhi Nguyen (:nhi)]

Valentin, could you take a look? Thanks!

Comment 7

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

Attachment: Bug 1625749 - Make sure SlicedInputStream initializes all its members r=nika

Comment 8

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

Comment on attachment 9140529 [details]
Bug 1625749 - Make sure SlicedInputStream initializes all its members r=nika

Security Approval Request

• How easily could an exploit be constructed based on the patch?: 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?: All
• If not all supported branches, which bug introduced the flaw?: Bug 1434553
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Applies cleanly on esr68
• How likely is this patch to cause regressions; how much testing does it need?: Very low chance of regressions.

Comment 9

[Daniel Veditz [:dveditz]]

• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: Bug 1434553

What happened in 74 that caused this Fx62 regression to suddenly start exhibiting itself? Is there more to this bug? Should we fix it on ESR/Fennec anyway just in case, or can we rely on something else to be protecting us there?

Comment 10

[Daniel Veditz [:dveditz]]

Do we have the same problem of missing initialization in one of multiple constructors with PartiallySeekableInputStream?

https://searchfox.org/mozilla-central/rev/72e3388f74458d369af4f6cdbaeaacb719523b8c/netwerk/base/PartiallySeekableInputStream.cpp#43-44,52-54

Comment 11

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

(In reply to Daniel Veditz [:dveditz] from comment #9)

• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: Bug 1434553

What happened in 74 that caused this Fx62 regression to suddenly start exhibiting itself? Is there more to this bug? Should we fix it on ESR/Fennec anyway just in case, or can we rely on something else to be protecting us there?

I assumed we introduced some code that caused us to call the other constructor in these cases (IPC deserialization)
I didn't look into it too much, since I caught the e4e4e4 pointer in gdb, and it was quite clear what the problem was.
But you're correct to ask why that's happening. I did a mozregression and reduced it to this:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=720c1e5a8dd3f5bda4ae32137d1c624a1ad55301&tochange=be9a6289486a6f366e431782b84a0c0633f8fec2
Not sure why the reporter says it worked for them in 72. I got several crashes in 72 while finding the regression.

I would uplift to ESR just in case - it's still an uninitialized pointer and there are likely codepaths that exercise it in ESR too.

(In reply to Daniel Veditz [:dveditz] from comment #10)

Do we have the same problem of missing initialization in one of multiple constructors with PartiallySeekableInputStream?

https://searchfox.org/mozilla-central/rev/72e3388f74458d369af4f6cdbaeaacb719523b8c/netwerk/base/PartiallySeekableInputStream.cpp#43-44,52-54

Yes, great find! I'll send another patch for that.

Comment 12

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

Attachment: Bug 1625749 - Make sure PartiallySeekableInputStream initializes all its members r=kershaw

Depends on D70919

Comment 13

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

Comment on attachment 9140554 [details]
Bug 1625749 - Make sure PartiallySeekableInputStream initializes all its members r=kershaw

Same as comment 8

(In reply to Valentin Gosu [:valentin] (he/him) from comment #8)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: 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?: All
• If not all supported branches, which bug introduced the flaw?: Bug 1434553
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Applies cleanly on esr68
• How likely is this patch to cause regressions; how much testing does it need?: Very low chance of regressions.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 9140529 [details]
Bug 1625749 - Make sure SlicedInputStream initializes all its members r=nika

sec-approval+ and beta uplift approval for 76. We'll also want this on ESR just in case something shifts and makes this reachable there as well, but you should have a separate uplift request for that. We may want to wait a week for the ESR uplift to land. (comment applies to approval for both patches).

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/47bc9152fc9700044f1ba37d26b94fa520b11621
https://hg.mozilla.org/integration/autoland/rev/0fb0aa5a4cfca316cf15d96fd58e73250a36fc21

Comment 16

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6309243d7304d8bcbf57cb1d7a4e9c1afda2c7ff
https://hg.mozilla.org/releases/mozilla-beta/rev/d2fa90f1c8f187994d18b419526a793609b3a6b0

Comment 17

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: These patches look trivial, and fix an issue which is apparently causing crashing on Twitter (bug 1628624), so we might want to consider this for a ride along if we're doing a dot release anyways.

Comment 18

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/47bc9152fc97
https://hg.mozilla.org/mozilla-central/rev/0fb0aa5a4cfc

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR approval when you get a chance.

Comment 20

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

Comment on attachment 9140529 [details]
Bug 1625749 - Make sure SlicedInputStream initializes all its members r=nika

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high. Uninitialized pointers.
• User impact if declined: Possible crashes and security issues.
• Fix Landed on Version: 77
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Initializes previously uninitialized member pointers to nullptr.
• String or UUID changes made by this patch:

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9140529 [details]
Bug 1625749 - Make sure SlicedInputStream initializes all its members r=nika

Approved for 68.8esr.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/b0fd38afeff7
https://hg.mozilla.org/releases/mozilla-esr68/rev/5f9ff1280e36