1213979 - Heap-use-after-free [@ mozilla::net::Http2Stream::AdjustInitialWindow]

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jesse Ruderman]

Attachment: stack

My ASan build is crashing very frequently at [@ mozilla::net::Http2Stream::AdjustInitialWindow], especially when browsing Wikipedia.

Today's nightly is also crashing frequently, at seemingly random locations, especially when browsing Twitter.

Comment 1

[Andrew McCreight [:mccr8]]

Judging by the number of people complaining about a very similar crash like this in #developers, I'm guessing this is going to be a Nightly topcrash.

Comment 2

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: frequent crash

Comment 3

[Dragana Damjanovic [:dragana]]

Probably it is bug 1211694 somehow triggered this. I will ask someone to back it out. And I will take a look tomorrow. (I am not sure why the patch triggers this)

Comment 4

[Ryan VanderMeulen [:RyanVM]]

I backed out bug 1211694 at Nick & Dragana's request to see if it fixes these crashes in tomorrow's nightly.
https://hg.mozilla.org/mozilla-central/rev/11ff0ccb7d59

Comment 5

[Patrick McManus [:mcmanus]]

I think this is a long standing h2 bug that dragana managed to bring to the surface but its not strictly dependent on 1211694 to trigger. (that patch makes it easier to trigger, because without it there is some extra space in the typical outgoing packet allocation - honestly, there probably is enough room 100% of the time, but I'm not certain of that analysis).

Nick, Dragana, I'll upload a patch.. see if it makes sense to you and go ahead with getting it tested/fixed-up or merged if so (I haven't even tried to compile it on this laptop).. my availability before friday is quite limited.

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: h2 packet formats

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Code in question goes back to Fx29, so marking all supported branches as affected.

Comment 8

[u408661]

Comment on attachment 8672908 [details] [diff] [review]
h2 packet formats

Review of attachment 8672908 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh, nice catch. LGTM.

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 8672908 [details] [diff] [review]
h2 packet formats


[Security approval request comment]
How easily could an exploit be constructed based on the patch? It gives a concrete area to target - but I'm not actually sure you could trigger it due to sufficient slack allocation in the default buffer. But the code clearly identifies the potential problem.

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 older supported branches are affected by this flaw? release, beta, aurora, esr

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? patch applies universally

How likely is this patch to cause regressions; how much testing does it need? very unlikely.

Comment 10

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

Comment on attachment 8672908 [details] [diff] [review]
h2 packet formats

[Triage Comment]

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9af9f4f39b28

Comment 12

[Carsten Book [:Tomcat]]

Failed to apply to 2.2r 

Tomcats-MacBook-Pro-2:mozilla-b2g37_v2_2r Tomcat$ hg qimport /sheriffs/patch/1213979.patch && hg qpush && hg qrefresh -e 
adding 1213979.patch to series file
applying 1213979.patch
patching file netwerk/protocol/http/Http2Stream.cpp
Hunk #1 FAILED at 721
1 out of 2 hunks FAILED -- saving rejects to file netwerk/protocol/http/Http2Stream.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory


Patrick could you take a look, thanks!

Comment 13

[Carsten Book [:Tomcat]]

also failed for beta and aurora

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9af9f4f39b28

Comment 15

[Patrick McManus [:mcmanus]]

Attachment: patch for aurora, beta, esr38, b2g37_v2_2r

this patch will apply to the older branches - a trivial logging line was added nearby in 44 that caused the conflict.

Comment 16

[Carsten Book [:Tomcat]]

thanks Patrick ! 

https://hg.mozilla.org/releases/mozilla-aurora/rev/551a28778624

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ed67ac61d1c0

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/de0f0f31955b
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/543a4e53966b

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/543a4e53966b

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This seems to have caused xpcshell failures on 2.2: https://treeherder.mozilla.org/logviewer.html#?job_id=171890&repo=mozilla-b2g37_v2_2

Comment 21

[Patrick McManus [:mcmanus]]

(In reply to Wes Kocher (:KWierso) from comment #20)
> This seems to have caused xpcshell failures on 2.2:
> https://treeherder.mozilla.org/logviewer.html#?job_id=171890&repo=mozilla-
> b2g37_v2_2

doesn't pass the sniff test (the changed code isn't even run in that test) - but I've been wrong about stuff like that before.

can you make new runs both with and without the patch?

If the problem is linked to this patch we might need keeler's help

Comment 22

[Ryan VanderMeulen [:RyanVM]]

That failure reeks of not running automated HSTS/HPKP updates on b2g37 to me.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8672908 [details] [diff] [review]
h2 packet formats

Not going take this patch in 41.