1827019 - Crash in [@ nsTArray_base<T>::Length | nsTArray_Impl<T>::IndexOf | nsTArray_Impl<T>::RemoveElement | nsTArray_Impl<T>::RemoveElement | mozilla::dom::HTMLFieldSetElement::RemoveElement]

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/d3d52eb0-c603-4af9-8821-18dc80230326

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_RelocateUsingMemutils>::Length const  xpcom/ds/nsTArray.h:410
0  xul.dll  nsTArray_Impl<nsGenericHTMLFormElement*, nsTArrayInfallibleAllocator>::IndexOf const  xpcom/ds/nsTArray.h:1343
0  xul.dll  nsTArray_Impl<nsGenericHTMLFormElement*, nsTArrayInfallibleAllocator>::RemoveElement  xpcom/ds/nsTArray.h:1953
0  xul.dll  nsTArray_Impl<nsGenericHTMLFormElement*, nsTArrayInfallibleAllocator>::RemoveElement  xpcom/ds/nsTArray.h:1966
0  xul.dll  mozilla::dom::HTMLFieldSetElement::RemoveElement  dom/html/HTMLFieldSetElement.cpp:227
1  xul.dll  mozilla::dom::ElementInternals::cycleCollection::Unlink  dom/html/ElementInternals.cpp:29
2  xul.dll  nsCycleCollector::CollectWhite  xpcom/base/nsCycleCollector.cpp:3099
2  xul.dll  nsCycleCollector::Collect  xpcom/base/nsCycleCollector.cpp:3465
3  xul.dll  nsCycleCollector::FinishAnyCurrentCollection  xpcom/base/nsCycleCollector.cpp:3524
4  xul.dll  mozilla::CycleCollectedJSRuntime::OnGC  xpcom/base/CycleCollectedJSRuntime.cpp:1841

About 117 crashes like this (on poison values) in the last 3 months.

The ones I looked at all had cycleCollection::Unlink calling into HTMLFieldSetElement::RemoveElement.

Comment 1

[Andrew McCreight [:mccr8]]

mFieldSet is a raw pointer. There's a comment that says "This is a pointer to the target element's closest fieldset parent if any. It's safe to use raw pointer because it will be reset via CustomElementData::Unlink when mTarget is released or unlinked."

I guess something went wrong with that logic. Edgar, could you take a look once you are back at work? Thanks.

Comment 2

[Andrew McCreight [:mccr8]]

This also shows up in recent ESR builds under a slightly different signature: bp-1e05f218-249b-45f4-9a3d-8d51e0230318

Comment 3

[Edgar Chen [:edgar]]

(In reply to Andrew McCreight [:mccr8] from comment #1)

I guess something went wrong with that logic. Edgar, could you take a look once you are back at work? Thanks.

One obvious potential issue can be found around https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/dom/html/ElementInternals.cpp#415-417, where ElementInternals removes FACE from HTMLFieldSetElement but doesn't set mFieldSet to null. It's necessary to review if there are other cases, and the comment for mFieldSet requires some revisions to be more accurate.

Comment 4

[Edgar Chen [:edgar]]

Attachment: Bug 1827019 - Reset mFieldset after unlink; r?smaug

Comment 5

[Edgar Chen [:edgar]]

Attachment: Bug 1827019 - Add diagnostic assertion to ensure mForm is reset after unlink; r?smaug

Depends on D176356

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 9330051 [details]
Bug 1827019 - Reset mFieldset after unlink; r?smaug

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't think it is easy to construct a exploit based on this patch.
• 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?: All
• If not all supported branches, which bug introduced the flaw?: None
• 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?: The risk should be low as the changes are straight forward.
• Is Android affected?: Yes

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 9330051 [details]
Bug 1827019 - Reset mFieldset after unlink; r?smaug

Beta/Release Uplift Approval Request

• User impact if declined: Content process might crash during unlinking.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The risk should be low as the changes are straight forward.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 9330051 [details]
Bug 1827019 - Reset mFieldset after unlink; r?smaug

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec high bug
• User impact if declined: Content process might crash during unlinking.
• Fix Landed on Version: Not yet landed on Nightly
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The risk should be low as the changes are straight forward.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Dan, we'd have to uplift this directly into the RC builds if we want to take it this cycle still. How strongly do you feel about that?

Comment 10

[Daniel Veditz [:dveditz]]

How easily could an exploit be constructed based on the patch?: I don't think it is easy to construct a exploit based on this patch.

I'm not confident that's true. The patch draws attention to the fact we have raw pointers mFieldSet and mForm, and the fact the patch explicitly nulls one of them out during Unlink strongly signals we're trying to fix a UAF of some kind. If there's a reliable way to trigger these conditions it could pop out quite quickly from targeted fuzzing that creates/modifies/deletes forms and fieldsets in combination with various events. A generic content fuzzer might not throw forms and fieldsets together often enough to find this bug, but with these clues people can focus their tools.

We should take it in 113. The patch is unlikely to cause new stability issues (should reduce crashes; at worst might convert some UAF to safe null-derefs), and if we don't take it now we should hold off landing the patch until late beta 114.

Comment 11

[Daniel Veditz [:dveditz]]

Although I'm happy that this patch fixes a raw pointer lifetime bug, I'm worried this might not be the only place these raw pointers cause us grief now or after future DOM code changes. Can we take another look at the problems that led to WONTFIXing bug 1162765 and see if we could move forward in that direction now?

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 9330051 [details]
Bug 1827019 - Reset mFieldset after unlink; r?smaug

a=dveditz for sec-approval, and for uplift to beta and ESR-102.

Please land ASAP so these get into the RC build on Monday and a bit of nightly testing over the weekend.

Comment 13

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

As said in bug 1162765, we'd need to add some flag if raw pointer wasn't used. I this we do have couple of spare bits in nsINode, so in theory those could be used. But we tend to need those bits for something more generic every now and then.

Comment 14

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

Reset mFieldset after unlink; r=smaug
https://hg.mozilla.org/integration/autoland/rev/13a45ae9137ac94c3f7d6665e37c5b8f65bd95a2
https://hg.mozilla.org/mozilla-central/rev/13a45ae9137a

Add diagnostic assertion to ensure mForm is reset after unlink; r=smaug
https://hg.mozilla.org/integration/autoland/rev/c4248a2d4832404b9c1edbc1b854f9ac952c634d
https://hg.mozilla.org/mozilla-central/rev/c4248a2d4832

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9330052 [details]
Bug 1827019 - Add diagnostic assertion to ensure mForm is reset after unlink; r?smaug

No point in uplifting a patch to Beta/ESR which only adds a diagnostic assert.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Landed for 113.0rc1 and 102.11esr.

https://hg.mozilla.org/releases/mozilla-beta/rev/527cf7dc1bb7
https://hg.mozilla.org/releases/mozilla-esr102/rev/d7603e3c388c