1448612 - Double free in nsStreamLoader (presumably because of FetchConsumer stuff).

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

So I got this interesting crash while I played with https://github.com/servo/buildbotstatus. That uses a worker to pull data from github and stuff:

  https://crash-stats.mozilla.com/report/index/71da80a3-76c9-4e45-bda4-79d000180325

Anyway, looking into how could this happen, I found a very suspicious piece of code. The data is presumably freed the second time here (well, on the ReleaseData call just below):

  https://searchfox.org/mozilla-central/rev/8220783953b0311e1d64c2366f732a159f05ed7e/netwerk/base/nsStreamLoader.cpp#113

But something has freed it already.

  https://searchfox.org/mozilla-central/rev/8220783953b0311e1d64c2366f732a159f05ed7e/dom/fetch/FetchConsumer.cpp#270

Looks highly suspicious. If it's not it, that needs fixing as well.

In particular, look how the constructor of AbortConsumeBodyControlRunnable frees the data, yet it doesn't return NS_SUCCESS_ADOPTED_DATA, which means that the nsStreamLoader code will assume it still owns it.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Don't take ownership of the stream data too early and then lie.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

In the case we couldn't dispatch the runnable, we'd double-free it, which is no good.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962115 [details] [diff] [review]
Don't take ownership of the stream data too early and then lie.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Somewhat hard I'd think, at least intentionally. You need to make the network event arrive at the right time, when the worker thread is already gone.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It's not obvious, but you can see it if you grep for the involved return codes.

Which older supported branches are affected by this flaw?
FF59 and 60.

If not all supported branches, which bug introduced the flaw?
bug 1422036

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?
Not much, the patch is really small and somewhat trivial.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1422036
[User impact if declined]: crashes w/ security implication.
[Is this code covered by automated tests?]: seems like none caught it.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: simple (as in, small and easy to prove correct) resource management fix.
[String changes made/needed]: none

Comment 5

[Daniel Veditz [:dveditz]]

Are you sure about the regressor? All the crashes I see in the last month with "mozilla::net::nsStreamLoader::OnStopRequest" in the signature happen in 61.0a1 -- none in 60.0 betas.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Looking at bug 1448635 comment 6, looks like the switch to WorkerRef exposed this, though the code was incorrect before in case dispatch failed. Probably it just didn't fail.

Comment 7

[Nathan Froyd [:froydnj]]

To echo Dan's comment 5, one can look at a lot of double-free signatures that don't necessarily have OnStopRequest in the signatures, but wind up being (almost all) OnStopRequest-related:

https://crash-stats.mozilla.com/signature/?moz_crash_reason=~Double-free&signature=arena_t%3A%3ADallocSmall%20%7C%20static%20void%20arena_dalloc&date=%3E%3D2018-03-13T06%3A38%3A17.000Z&date=%3C2018-03-27T06%3A38%3A17.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1

https://crash-stats.mozilla.com/signature/?_sort=-date&moz_crash_reason=%7EDouble-free&signature=static%20void%20arena_dalloc&date=%3E%3D2018-03-13T10%3A38%3A17.000Z&date=%3C2018-03-27T10%3A38%3A17.000Z

https://crash-stats.mozilla.com/signature/?moz_crash_reason=~Double-free&signature=free.cold.86%20%7C%20mozilla%3A%3Anet%3A%3AnsStreamLoader%3A%3AOnStopRequest&date=%3E%3D2018-03-13T06%3A38%3A17.000Z&date=%3C2018-03-27T06%3A38%3A17.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1

https://crash-stats.mozilla.com/signature/?moz_crash_reason=~Double-free&signature=arena_dalloc%20%7C%20Allocator%3CT%3E%3A%3Afree%20%7C%20mozilla%3A%3AVector%3CT%3E%3A%3AclearAndFree&date=%3E%3D2018-03-13T06%3A38%3A17.000Z&date=%3C2018-03-27T06%3A38%3A17.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

https://crash-stats.mozilla.com/signature/?_sort=-date&moz_crash_reason=%7EDouble-free&signature=huge_dalloc%20%7C%20mozilla%3A%3Anet%3A%3AnsStreamLoader%3A%3AOnStopRequest&date=%3E%3D2018-03-13T10%3A38%3A17.000Z&date=%3C2018-03-27T10%3A38%3A17.000Z

https://crash-stats.mozilla.com/signature/?_sort=-date&moz_crash_reason=%7EDouble-free&signature=static%20void%20huge_dalloc&date=%3E%3D2018-03-13T10%3A38%3A17.000Z&date=%3C2018-03-27T10%3A38%3A17.000Z

https://crash-stats.mozilla.com/signature/?_sort=-date&moz_crash_reason=%7EDouble-free&signature=arena_t%3A%3ADallocSmall%20%7C%20arena_dalloc%20%7C%20Allocator%3CT%3E%3A%3Afree%20%7C%20mozilla%3A%3Anet%3A%3AnsStreamLoader%3A%3AOnStopRequest&date=%3E%3D2018-03-13T10%3A38%3A17.000Z&date=%3C2018-03-27T10%3A38%3A17.000Z

...and none of them have any crashes outside of 61.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8962115 [details] [diff] [review]
Don't take ownership of the stream data too early and then lie.

Clearing sec-approval. As a sec-moderate, it doesn't need sec-approval to go into trunk. That also means it may not be back ported to Beta.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

remote: View your change here:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/54b5c168cc34fb0f8793fcca70174b8effb3d734
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=54b5c168cc34fb0f8793fcca70174b8effb3d734

Comment 10

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/mozilla-central/rev/54b5c168cc34fb0f8793fcca70174b8effb3d734

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8962115 [details] [diff] [review]
Don't take ownership of the stream data too early and then lie.

Approved for 60.0b8.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Looks like this'll need a rebased patch for Beta uplift.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Rebased for beta.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/901288518dfd

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Had to add an explicit to make the static analysis happy too.
https://hg.mozilla.org/releases/mozilla-beta/rev/da6d3b5f248b