1856737 - Poison crash in [@ nsCOMPtr<T>::assign_with_AddRef | nsCOMPtr<T>::operator= | mozilla::EventTargetChainItem::LegacyPreActivationBehavior]

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/516ad414-279a-44d3-8ca8-616240231003

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  nsCOMPtr<nsISupports>::assign_with_AddRef  xpcom/base/nsCOMPtr.h:807
0  xul.dll  nsCOMPtr<nsISupports>::operator=  xpcom/base/nsCOMPtr.h:560
0  xul.dll  mozilla::EventTargetChainItem::LegacyPreActivationBehavior  dom/events/EventDispatcher.cpp:448
0  xul.dll  mozilla::EventDispatcher::Dispatch  dom/events/EventDispatcher.cpp:1096
1  xul.dll  mozilla::PresShell::HandleDOMEventWithTarget  layout/base/PresShell.cpp:8811
2  xul.dll  mozilla::dom::Element::DispatchEvent  dom/base/Element.cpp:2263
3  xul.dll  mozilla::dom::Element::DispatchClickEvent  dom/base/Element.cpp:2303
4  xul.dll  mozilla::dom::HTMLLabelElement::PostHandleEvent  dom/html/HTMLLabelElement.cpp:165
5  xul.dll  mozilla::EventTargetChainItem::PostHandleEvent  dom/events/EventDispatcher.cpp:485
5  xul.dll  mozilla::EventTargetChainItem::HandleEventTargetChain  dom/events/EventDispatcher.cpp:654

This method was added in bug 1658996, and the crash first showed up in the 20230918094102 Nightly build, but maybe this is just a signature change? I feel like I've seen weird event crashes before, so maybe it isn't actually a regression.

Comment 1

[Andrew McCreight [:mccr8]]

Vincent, do you think your patch could have caused a regression here? Thanks.

Comment 2

[Andrew McCreight [:mccr8]]

I looked for crashes on poison values with EventStateManager::PostHandleMouseUp in the proto signature over the last 6 months. I found 10 with this signature. I didn't see any other signatures that looked like "generic event handling stuff" which is what I'd expect if this was merely signature change.

Doing the same thing with crashes with HTMLLabelElement::PostHandleEvent in the proto signature over the last 6 months, I found 3 with this signature and no other crashes.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1658996

Comment 4

[Vincent Hilla [:vhilla]]

These were my changes to the EventTargetChainItem and mItemData was used in a similar way already in PreHandleEvent and PostHandleEvent. In those two functions, more access violations occurred for mItemData: 11 here and 3 here.

Though given LegacyPreActivationBehavior runs earlier and under other conditions than Pre/PostHandleEvent, it might be that I created a new case where an access violation around mItemData happens. I just noticed these two places should check NS_SUCCEEDED(rv) and my patch didn't account for the case when aTargets in Dispatch is non-null. Still, I don't think there should ever be a case where accessing mItemData is illegal.

I'll add checking rv and handling aTargets to my backlock, but am out of office for the next few weeks.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Vincent, thanks for your comment.
Edgar, can you take this bug so hopefully we don't need to wait for a few weeks before making progress?

Comment 6

[Andrew McCreight [:mccr8]]

One approach to investigating here would be to look at the locations in the call stack and look for raw pointers that are live across call sites and method calls on object members.

Comment 7

[Edgar Chen [:edgar]]

My theory is that EventDispatcher uses a raw pointer to cache the activation target which pointers to a element of nsTArray<EventTargetChainItem> chain and initial capacity of chain is 128. In most of case, the number of chain items is fewer than 128, so it works fine. However, if the chain items exceed 128, the nsTArray allocates a new memory region and moves the data there, which causes the raw pointer points to a invalid memory.

Comment 8

[Edgar Chen [:edgar]]

Attachment: Bug 1856737 - Use index to track activation target item; r?smaug,masayuki

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 9357161 [details]
Bug 1856737 - Use index to track activation target item; r?smaug,masayuki

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it is not hard as the patch shows the problem is about raw pointer and nsTArray accessing.
• 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?: beta
• If not all supported branches, which bug introduced the flaw?: Bug 1658996
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Patch should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: It just use index instead of raw pointer to track event chain item, behavior doesn't change, it should be safe.
• Is Android affected?: Yes

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9357161 [details]
Bug 1856737 - Use index to track activation target item; r?smaug,masayuki

Approved to request uplift and land

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f94dbdc2a2a1
Use index to track activation target item; r=smaug

Comment 12

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

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

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:edgar, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox119 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Edgar Chen [:edgar]]

Comment on attachment 9357161 [details]
Bug 1856737 - Use index to track activation target item; r?smaug,masayuki

Beta/Release Uplift Approval Request

• User impact if declined: Tab crash when clicking on input radio/checkbox and the event target chain items exceed the default capacity.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: None
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It just use index instead of raw pointer to track event chain item, behavior doesn't change, it should be safe.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9357161 [details]
Bug 1856737 - Use index to track activation target item; r?smaug,masayuki

Approved for 119.0b9

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/76c4334b6bac

Comment 17

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter