1233259 - Heap-use-after-free [@ nsDocument::NotifyMediaFeatureValuesChanged] with srcset

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: manual testcase

1. Load the testcase
2. Trigger cycle collection (press "CC" button in about:memory)
3. Zoom in on the testcase (press ⌘+ back in its tab)

Heap-use-after-free [@ nsDocument::NotifyMediaFeatureValuesChanged]

Comment 1

[Jesse Ruderman]

Attachment: ASan stacks

Comment 2

[Josh Matthews [:jdm]]

We're missing mResponsiveContent from nsDocument's traverse etc. methods.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Josh Matthews [:jdm] from comment #2)
> We're missing mResponsiveContent from nsDocument's traverse etc. methods.

Nope. mResponsiveContent keeps raw pointers to nsIContent objects.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8699287 [details] [diff] [review]
patch

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

Thanks for figuring this out :)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8699287 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not sure about exploit, but the issue is pretty clear.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Commit message could be

Which older supported branches are affected by this flaw?
nightly, aurora

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch seems to apply to Aurora just fine

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #6) 
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> Commit message could be
"Bug 1233259, only in-document images should respond to viewport changes, r=mystor"

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Wait, am I reading the spec wrong.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8699287 [details] [diff] [review]
patch

(I need to figure out what the spec tries to say here)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8699287 [details] [diff] [review]
patch

Ok, the spec is about to be clarified and the patch gives that behavior 
https://github.com/whatwg/html/issues/414

Comment 11

[Andrew McCreight [:mccr8]]

(I marked this sec-high rather than critical because it seems like it may require user interaction. That said, resizing the window or whatever isn't too much of a burden so it could also be critical I guess.)

Comment 12

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

Comment on attachment 8699287 [details] [diff] [review]
patch

Approvals given.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d406297b87

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1d406297b87

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/92d89aad273c

Comment 17

[Josh Matthews [:jdm]]

This says FF 44 is unaffected but I believe bug 1240763 is the same symptom of a crash caused by responsive content in an FF 43 build.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Don't have access to bug 1240763