1751679 - Crash in [@ mozilla::EditorDOMPointBase<T>::Offset]

Status: RESOLVED FIXED

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

Description

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

19 crashes from 10+ installations, Windows and macOS. Firefox 97+ affected.

Crash report: https://crash-stats.mozilla.org/report/index/009364dd-6a68-4614-8fbd-41efd0220112

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(isSome())

Top 10 frames of crashing thread:

0 xul.dll mozilla::EditorDOMPointBase<nsCOMPtr<nsINode>, nsCOMPtr<nsIContent> >::Offset const editor/libeditor/EditorDOMPoint.h:500
1 xul.dll mozilla::HTMLEditor::MoveOneHardLineContents editor/libeditor/HTMLEditorDeleteHandler.cpp:4714
2 xul.dll static mozilla::WhiteSpaceVisibilityKeeper::MergeFirstLineOfRightBlockElementIntoAncestorLeftBlockElement editor/libeditor/WSRunObject.cpp:515
3 xul.dll mozilla::HTMLEditor::AutoDeleteRangesHandler::AutoBlockElementsJoiner::AutoInclusiveAncestorBlockElementsJoiner::Run editor/libeditor/HTMLEditorDeleteHandler.cpp:4581
4 xul.dll mozilla::HTMLEditor::AutoDeleteRangesHandler::AutoBlockElementsJoiner::HandleDeleteAtCurrentBlockBoundary editor/libeditor/HTMLEditorDeleteHandler.cpp:2848
5 xul.dll mozilla::HTMLEditor::AutoDeleteRangesHandler::AutoBlockElementsJoiner::Run editor/libeditor/HTMLEditorDeleteHandler.cpp:522
6 xul.dll mozilla::HTMLEditor::AutoDeleteRangesHandler::HandleDeleteAroundCollapsedRanges editor/libeditor/HTMLEditorDeleteHandler.cpp:1832
7 xul.dll mozilla::HTMLEditor::AutoDeleteRangesHandler::Run editor/libeditor/HTMLEditorDeleteHandler.cpp:1631
8 xul.dll mozilla::HTMLEditor::HandleDeleteSelection editor/libeditor/HTMLEditorDeleteHandler.cpp:1126
9 xul.dll mozilla::EditorBase::DeleteSelectionAsSubAction editor/libeditor/EditorBase.cpp:4214

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, it detects a traditional bug.

Comment 2

[Christian Holler (:decoder)]

The isSome() diagnostic assert can indicate uninitialized memory. Marking s-s.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1751679 - Make `WhiteSpaceVisibilityKeeper::MergeFirstLineOfRightBlockElementIntoAncestorLeftBlockElement` track DOM points while the DOM tree is being changed r=m_kato!

The method caches a DOM point, but it is not tracked correctly.

This patch adds some validations into it and one of its callee,
HTMLEditor::MoveOneHardLineContents. Additionally, making
EditorDOMPointBase::Offset avoid accessing Maybe's storage when it's
Nothing in release builds as the last resort.

Comment 4

[Daniel Veditz [:dveditz]]

sec-moderate might be under-selling this depending on what that offset is later used for

Is this a regression? Or are the asserts new?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Daniel Veditz [:dveditz] from comment #4)

sec-moderate might be under-selling this depending on what that offset is later used for

Is this a regression? Or are the asserts new?

The assertion is new. The offset value is used to consider where the content nodes in an array will be inserted in a parent node. Therefore, even if the value is wrong, the result is just to get odd DOM tree result of editing (Backspace or Delete from start/end of a nested child block element). So I don't think that this is a security issue.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9260792 [details]
Bug 1751679 - Make WhiteSpaceVisibilityKeeper::MergeFirstLineOfRightBlockElementIntoAncestorLeftBlockElement track DOM points while the DOM tree is being changed r=m_kato!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't think that this can attack users except getting odd editing result because if the storage of Maybe<uint32_t> is random value, it just points in the parent left block element where the moving nodes which are placed in the first line of the right block element are inserted.

Anyway, this patch may not unclear what's going on because this does not touch some places around the buggy point.

• 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?: 97
• If not all supported branches, which bug introduced the flaw?: Bug 1741148
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Not risky because this makes it and its callers check whether the inserting point becomes invalid or not immediately after modifying the DOM tree and stop handling the edit operation in such case. This patch is cleanly grafted in beta channel.
• How likely is this patch to cause regressions; how much testing does it need?: I'm not sure how to reproduce this bug. With this patch, it'll start to return 0u as offset when the point is invalid in release builds. I.e., when users meet a bug, the content will be inserted to start of parent block. It's clearly odd behavior so that I'd like users to report the bug as a web-compat issue.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Calling this wontfix for 97 given where we are in the cycle (we're building our final beta tomorrow). Let me know if you feel strongly otherwise and we can reconsider.

Comment 9

[Andrew McCreight [:mccr8]]

Bug 1753342 has a test case.

Comment 10

[Andrew McCreight [:mccr8]]

This is sec-moderate, so it doesn't need sec approval.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9260792 [details]
Bug 1751679 - Make WhiteSpaceVisibilityKeeper::MergeFirstLineOfRightBlockElementIntoAncestorLeftBlockElement track DOM points while the DOM tree is being changed r=m_kato!

Oh, I didn't know that. I'll land it ASAP.

Comment 12

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

Make WhiteSpaceVisibilityKeeper::MergeFirstLineOfRightBlockElementIntoAncestorLeftBlockElement track DOM points while the DOM tree is being changed r=m_kato
https://hg.mozilla.org/integration/autoland/rev/216db1a8defd33d9efe0623261dcd5f211b90a9b
https://hg.mozilla.org/mozilla-central/rev/216db1a8defd

Comment 13

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

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The amount of the crash reports is not so many, and the patch changes some logic. So I think that it should just ride the train (although it's fine to uplift the patch if release managers want this fix).