1742738 - Assertion failure: mItems.Length() == 0 || mItems.Length() == InternalList().Length() (DOM wrapper's list length is out of sync), at /builds/worker/checkouts/gecko/dom/svg/DOMSVGPointList.h:142

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20211122-bf638341288e (--enable-debug --enable-fuzzing)

Assertion failure: mItems.Length() == 0 || mItems.Length() == InternalList().Length() (DOM wrapper's list length is out of sync), at /builds/worker/checkouts/gecko/dom/svg/DOMSVGPointList.h:142

#0 0x7f45b1063cb3 in mozilla::dom::DOMSVGPointList::LengthNoFlush() const src/dom/svg/DOMSVGPointList.h:140:5
#1 0x7f45b10641e4 in mozilla::dom::DOMSVGPointList::InsertItemBefore(mozilla::dom::DOMSVGPoint&, unsigned int, mozilla::ErrorResult&) src/dom/svg/DOMSVGPointList.cpp:251:29
#2 0x7f45af561482 in mozilla::dom::SVGPointList_Binding::insertItemBefore(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/SVGPointListBinding.cpp:244:78
#3 0x7f45b00108a8 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3306:13
#4 0x7f45b392cf8f in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) src/js/src/vm/Interpreter.cpp:388:13
#5 0x7f45b392c69b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:475:12
#6 0x7f45b392e16e in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) src/js/src/vm/Interpreter.cpp:535:10
#7 0x7f45b39239f6 in CallFromStack src/js/src/vm/Interpreter.cpp:539:10
#8 0x7f45b39239f6 in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3243:16
#9 0x7f45b391a903 in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:357:13
#10 0x7f45b392c596 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:507:13
#11 0x7f45b392e16e in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) src/js/src/vm/Interpreter.cpp:535:10
#12 0x7f45b392e371 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) src/js/src/vm/Interpreter.cpp:552:8
#13 0x7f45b3ae6761 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/vm/CallAndConstruct.cpp:117:10
#14 0x7f45afd21b5c in mozilla::dom::EventHandlerNonNull::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/EventHandlerBinding.cpp:283:37
#15 0x7f45b043afb9 in void mozilla::dom::EventHandlerNonNull::Call<nsCOMPtr<mozilla::dom::EventTarget> >(nsCOMPtr<mozilla::dom::EventTarget> const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/obj-build/dist/include/mozilla/dom/EventHandlerBinding.h:365:12
#16 0x7f45b043a230 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:201:12
#17 0x7f45b041b25b in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1123:22
#18 0x7f45b041bf19 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1314:17
#19 0x7f45b0411984 in HandleEvent src/dom/events/EventListenerManager.h:394:5
#20 0x7f45b0411984 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:348:17
#21 0x7f45b0410ea7 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:550:16
#22 0x7f45b0413708 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1085:11
#23 0x7f45b0415ed6 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) src/dom/events/EventDispatcher.cpp
#24 0x7f45aea334ee in nsContentUtils::DispatchEvent(mozilla::dom::Document*, nsISupports*, mozilla::WidgetEvent&, mozilla::EventMessage, mozilla::CanBubble, mozilla::Cancelable, mozilla::Trusted, bool*, mozilla::ChromeOnlyDispatch) src/dom/base/nsContentUtils.cpp:4323:17
#25 0x7f45b03d636f in nsresult nsContentUtils::DispatchTrustedEvent<mozilla::WidgetEvent>(mozilla::dom::Document*, nsISupports*, mozilla::EventMessage, mozilla::CanBubble, mozilla::Cancelable, bool*, mozilla::ChromeOnlyDispatch) /builds/worker/workspace/obj-build/dist/include/nsContentUtils.h:1462:12
#26 0x7f45b03d605d in mozilla::AsyncEventDispatcher::Run() src/dom/events/AsyncEventDispatcher.cpp:52:12
#27 0x7f45ace73f12 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:144:20
#28 0x7f45acea3cfe in mozilla::RunnableTask::Run() src/xpcom/threads/TaskController.cpp:468:16
#29 0x7f45ace7d5b6 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) src/xpcom/threads/TaskController.cpp:771:26
#30 0x7f45ace7c278 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) src/xpcom/threads/TaskController.cpp:607:15
#31 0x7f45ace7c4f3 in mozilla::TaskController::ProcessPendingMTTask(bool) src/xpcom/threads/TaskController.cpp:391:36
#32 0x7f45acea72f6 in operator() src/xpcom/threads/TaskController.cpp:124:37
#33 0x7f45acea72f6 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:531:5
#34 0x7f45ace91fe3 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1183:16
#35 0x7f45ace991ca in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:467:10
#36 0x7f45ad92f3d6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:85:21
#37 0x7f45ad84e9d7 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:331:10
#38 0x7f45ad84e8e2 in RunHandler src/ipc/chromium/src/base/message_loop.cc:324:3
#39 0x7f45ad84e8e2 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:306:3
#40 0x7f45b17f5a78 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
#41 0x7f45b37b0843 in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:918:20
#42 0x7f45ad9302ca in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:235:9
#43 0x7f45ad84e9d7 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:331:10
#44 0x7f45ad84e8e2 in RunHandler src/ipc/chromium/src/base/message_loop.cc:324:3
#45 0x7f45ad84e8e2 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:306:3
#46 0x7f45b37afe7b in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:750:34
#47 0x563efcfdbe49 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
#48 0x563efcfdbe49 in main src/browser/app/nsBrowserApp.cpp:327:18
#49 0x7f45c40910b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
#50 0x563efcfb75dc in _start (/home/worker/builds/m-c-20211122210514-fuzzing-debug/firefox-bin+0x155dc)

Comment 1

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/fvFZiTzA6y9byx09FIaRfA/index.html

Comment 2

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20211124035313-77e540295ce5.
Failed to bisect testcase (Unable to launch the start build!):

Start: 85b08b0e3773d694395e31e40cc7d6b4e901bf0c (20201125094348)
End: bf638341288eacbfd43ac6d8d3080e011ff1e8fb (20211122210514)
BuildFlags: BuildFlags(asan=False, tsan=False, debug=True, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False)

Comment 3

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

The severity field is not set for this bug.
:dholbert, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Bugmon [:jkratzer for issues]]

Bugmon was unable reproduce this issue.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 5

[Jason Kratzer [:jkratzer]]

A change to the Taskcluster build definitions over the weekend caused Bugmon to fail when reproducing issues. This issue has been corrected. Re-enabling bugmon.

Comment 6

[Bugmon [:jkratzer for issues]]

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

Comment 7

[Bugmon [:jkratzer for issues]]

A pernosco session for this bug can be found here.

Comment 8

[Daniel Holbert [:dholbert]]

In the pernosco session, mItems.Length() is 2 whereas InternalList().Length() is 3. (Previously they're both 2, but InternalList().Length() grows to length 3 in a call to InsertBefore(), while operating on a different tearoff that shares the same underlying internal-list.)

This happens because there are two different tearoff objects, so one of them ends up out of date. The testcase has:

  let x = b.points
  try { FuzzingFunctions.garbageCollect() } catch (err) {}
  try { FuzzingFunctions.cycleCollect() } catch (err) {}
  let y = b.points

Here x and y end up with different DOMSVGPointList objects having been returned from b.points.

It looks like this is because the tearoff table might not be sticking around for the second b.points call for some reason, which is why we end up generating a new DOMSVGPointList tearoff.

Comment 9

[Daniel Holbert [:dholbert]]

I'm marking this as security-sensitive since I think (?) having these tearoffs out-of-sync might have some security implications (though it's been a while since I thought about them).

I looked at this a bit more in pernosco and have a rough idea of what's going on here:

• We run through the code that I quoted in comment 8 twice (since the main function go() schedules another call to itself with setTimeout(go, 0))
• The first time we run through the code, everything is fine. let x = b.points and let y = b.points both get the same tearoff object, as you would expect.
• The second time we run through the code, things go wrong as described below -- the short version is, that original tearoff object (from the first time through the loop) ends up getting cleaned up and trying to remove itself from the tearoff-table twice, separated in time which is the problem here. Here's what happens:
  (1) During the cycle-collection in line 5 (of the testcase), the original tearoff object is part of a cycle and we call Unlink on it, which removes it from its tearoff table and causes its tearoff table to be empty and hence destroyed:

NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DOMSVGPointList)
  // No unlinking of mElement, we'd need to null out the value pointer (the
  // object it points to is held by the element) and null-check it everywhere.
  tmp->RemoveFromTearoffTable();

HOWEVER, importantly, the original tearoff object itself does not yet get destroyed! Something else must be holding onto it which has not yet been destroyed.

(2) At line 6 of the testcase, we make a new tearoff for let x = b.points, which causes a new tearoff table to get constructed.
(3) At line 8 of the testcase, we cycle-collect again and this cycle-collection causes our original tearoff object to get destroyed. And in its destructor, it calls RemoveFromTearoffTable() again -- and that removal happens abstractly using our "key", so we end up inadvertently removing the tearoff that we only just created in (2) from the tearoff table and deleting the tearoff-table again altogether. (Meanwhile JS still has a handle to that tearoff, in the variable x)
(4) In line 9 of the testcase, let y = b.points, we find that there's no tearoff table so we make a new tearoff, even though one already exists and we've got a handle on it.
(5) We proceed to modify one of our two tearoffs and then trip over an assertion while working with the other one since it's out of sync.

Hopefully that makes sense.

Two thoughts here:

• I'm not entirely clear on why we unlink the object in step (1) of my list above and then don't delete it until later. Maybe that's normal? I'm not sure.
• Maybe we can mitigate this by adding a new bool to the tearoff object to track whether it expects to be in a tearoff table? (e.g. bool mIsInTearoffTable) And RemoveFromTearoffTable() would set that bool to false, and a redundant RemoveFromTearoffTable() call would simply be a no-op if the bool was already false.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)

• I'm not entirely clear on why we unlink the object in step (1) of my list above and then don't delete it until later. Maybe that's normal? I'm not sure.

Chatting with mccr8, it sounds like this^ is actually a normal thing that can happen, for C++ objects that are owned by JS. (They can get unlinked in one CC and then deleted in the next GC or CC, with JS having an opportunity to run in between.)

Given that, I think we do need some sort of bool mIsInTearoffTable guard here to nerf redundant calls to RemoveFromTearoffTable() - maybe for all of our SVG tearoffs, even (it's probably not just this one that's susceptible to this).

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Bug 1742738 part 1: Tighten up tearoff-table removal for DOMSVGPointList and DOMSVGStringList. r?#svg

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: Bug 1742738 part 2: Tighten up tearoff-table removal for DOMSVGLength. r?#svg

I'm doing this one in its own patch since it's slightly more subtle than the
others, due to the existence of multiple instance-creation codepaths, some of
which generate instances that never end up in the tearoff table.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Bug 1742738 part 3: Tighten up tearoff-table removal for DOMSVGPoint. r?#svg

I'm doing this one in its own patch since it's slightly more subtle than the
others, due to the existence of multiple instance-creation codepaths, some of
which generate instances that never end up in the tearoff table.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)

I think we do need some sort of bool mIsInTearoffTable guard here to nerf redundant calls to RemoveFromTearoffTable() - maybe for all of our SVG tearoffs, even (it's probably not just this one that's susceptible to this).

More specifically: I think we need this fixup for all of our DOMSVG{...} wrappers that have a call to RemoveTearoff outside of their destructor.

I audited for these classes by doing grep -B4 RemoveTearoff * in dom/svg, and I found the four classes that my patches are fixing up (all of which do table-removal in their cycle-collection "unlink" function as well as in their destructor).

The rest of the files only have a call to RemoveTearoff inside of a destructor, so they don't have the same opportunity for double-removals coming from the same tearoff-object.

Comment 15

[Daniel Holbert [:dholbert]]

The main idea with all 3 patches is:

• Add a new mIsInTearoffTable flag, which we set to true as soon as we've been added to the tearoff table.[1]
• Check that flag before trying to remove from the tearoff-table (and consider this new flag to be the authoritative signal for whether we need to remove).
• Immediately set the flag to false after removal, so that we won't attempt to remove again.

This prevents double-removal which (combined with some delay-for-JS-to-run) was causing the problem.

[1] (I set the flag to true up-front in part 1 to simplify the flow slightly -- since in that patch, there's only one invocation of the constructor which immediately/unconditionally inserts into the tearoff table.)

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Tyson Smith [:tsmith] from comment #0)

Created attachment 9252243 [details]
testcase.html

Found while fuzzing m-c 20211122-bf638341288e (--enable-debug --enable-fuzzing)

Tyson: if you can still repro with this testcase, would you mind testing with the attached patch-stack and confirm that the patch stack (particularly part 1) fixes the issue?

I tried testing today's mozilla-central built with --enable-debug --enable-fuzzing but I can't reproduce the bug when viewing a locally saved copy of the testcase.

I suspect it should be reproducible, given that bugmon was able to get a pernosco recording 3 days ago, but I'm not sure if I'm missing some fuzzing component.

Thanks!

Comment 17

[Tyson Smith [:tsmith]]

I am able to reproduce the issue (did you miss the pref fuzzing.enabled=true?) and I am not able to reproduce the issue with the part 1 patch applied.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Tyson Smith [:tsmith] from comment #17)

I am able to reproduce the issue (did you miss the pref fuzzing.enabled=true?)

That was probably it -- I was just building with --enable-debug --enable-fuzzing.

I am not able to reproduce the issue with the part 1 patch applied.

Hooray! Thanks for testing.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)

I'm marking this as security-sensitive since I think (?) having these tearoffs out-of-sync might have some security implications (though it's been a while since I thought about them).

More notes on this - I think we can consider this sec-moderate since as far as I can tell, the worst that can happen here would be accessing beyond the end of a nsTArray, which nowadays triggers an intentional safe crash via e.g. here for accessing an array-entry and here for removing an array-entry.

So I don't think this is easily exploitable (due to the above, plus the fact that it requires precise control over cycle-collection), hence rating as sec-moderate. Given that, this doesn't need explicit security approval in order to land, per category (A) at https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html#on-requesting-sec-approval . --> Proceeding to trigger lando.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd2063aa0818
part 1: Tighten up tearoff-table removal for DOMSVGPointList and DOMSVGStringList. r=firefox-svg-reviewers,longsonr
https://hg.mozilla.org/integration/autoland/rev/45724ee22942
part 2: Tighten up tearoff-table removal for DOMSVGLength. r=firefox-svg-reviewers,longsonr
https://hg.mozilla.org/integration/autoland/rev/49455c6b902f
part 3: Tighten up tearoff-table removal for DOMSVGPoint. r=firefox-svg-reviewers,longsonr

Comment 21

[Cristina Horotan [:chorotan]]

Backed out for causing Crashtest failures at DOMSVGLength.cpp

Backout link

Push with failures

Failure log 1
Failure log 2
Failure log wpt

Comment 22

[Daniel Holbert [:dholbert]]

Thanks - those test failures highlighted an oversight in part 2, which I've now addressed. I can successfully run all of the tests in those directories in a debug build now.

Try run with updated patch ({mochi/crash/ref/web-platform}-tests on linux debug), just to be sure there aren't further issues before re-landing:
https://treeherder.mozilla.org/jobs?repo=try&revision=19b83338837c9994b2c642a0010686fa705033cc

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/170ea0bbeec4
part 1: Tighten up tearoff-table removal for DOMSVGPointList and DOMSVGStringList. r=firefox-svg-reviewers,longsonr
https://hg.mozilla.org/integration/autoland/rev/3ab3197c3ef6
part 2: Tighten up tearoff-table removal for DOMSVGLength. r=firefox-svg-reviewers,longsonr
https://hg.mozilla.org/integration/autoland/rev/2a12045ea900
part 3: Tighten up tearoff-table removal for DOMSVGPoint. r=firefox-svg-reviewers,longsonr

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/170ea0bbeec4
https://hg.mozilla.org/mozilla-central/rev/3ab3197c3ef6
https://hg.mozilla.org/mozilla-central/rev/2a12045ea900

Comment 25

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20250422155236-701cf6f8c792.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.