Closed Bug 1422931 Opened 2 years ago Closed 2 years ago

AddressSanitizer: heap-buffer-overflow [@ mozilla::dom::ExplicitChildIterator::GetNextChild] with READ of size 8

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: truber, Assigned: jessica)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-bounds, sec-high)

Crash Data

Attachments

(3 files, 5 obsolete files)

Attached file testcase.html
The attached testcase crashes in m-c rev 20171204-195bb467e6cb.

==2323==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60d00004c938 at pc 0x7fde9a152cb9 bp 0x7ffc1aab74b0 sp 0x7ffc1aab74a8
READ of size 8 at 0x60d00004c938 thread T0 (file:// Content)
    #0 0x7fde9a152cb8 in Length /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:400:37
    #1 0x7fde9a152cb8 in IsEmpty /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:403
    #2 0x7fde9a152cb8 in mozilla::dom::ExplicitChildIterator::GetNextChild() /builds/worker/workspace/build/src/dom/base/ChildIterator.cpp:112
    #3 0x7fde9ed73b68 in nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsAtom*, int, bool, nsStyleContext*, unsigned int, nsTArray<nsIAnonymousContentCreator::ContentInfo>*, nsCSSFrameConstructor::FrameConstructionItemList&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp
    #4 0x7fde9ed97bf2 in DoAddFrameConstructionItems /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5809:3
    #5 0x7fde9ed97bf2 in nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, nsCSSFrameConstructor::InsertionPoint const&, nsCSSFrameConstructor::FrameConstructionItemList&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5831
    #6 0x7fde9eda6c74 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, nsCSSFrameConstructor::InsertionKind, TreeMatchContext*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:7744:5
    #7 0x7fde9ecb750c in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:1414:27
    #8 0x7fde9ed37263 in mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1159:9
    #9 0x7fde9ecf0bf8 in ProcessPendingRestyles /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1235:3
    #10 0x7fde9ecf0bf8 in ProcessPendingRestyles /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RestyleManagerInlines.h:44
    #11 0x7fde9ecf0bf8 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4219
    #12 0x7fde9a408a1a in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:571:5
    #13 0x7fde9a408a1a in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:8294
    #14 0x7fde99115297 in nsDocLoader::DocLoaderIsEmpty(bool) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:704:14
    #15 0x7fde9911772c in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:633:5
    #16 0x7fde9911864c in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp
    #17 0x7fde973d82ea in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:629:28
    #18 0x7fde9a40ef47 in DoUnblockOnload /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:9116:18
    #19 0x7fde9a40ef47 in nsDocument::UnblockOnload(bool) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:9038
    #20 0x7fde9a3eadea in nsDocument::DispatchContentLoadedEvents() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:5678:3
    #21 0x7fde9a458ea4 in applyImpl<nsDocument, void (nsDocument::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1142:12
    #22 0x7fde9a458ea4 in apply<nsDocument, void (nsDocument::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1148
    #23 0x7fde9a458ea4 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1192
    #24 0x7fde971f0524 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:396:25
    #25 0x7fde97216dfe in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1033:14
    #26 0x7fde97232b80 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10
Flags: in-testsuite?
Would be nice to have a link to hg from about:buildconfig.
Flags: needinfo?(jjong)
Crash Signature: [@ mozilla::dom::ExplicitChildIterator::GetNextChild]
This happens when the pref "dom.webcomponents.enabled" is off.
slot element would become an HTMLUnknowElement [1] instead of HTMLSlotElement, but HTMLSlotElement::FromContent() would still returns non-null. This should be fixed for places that use HTMLSlotElement::FromContent().

[1] https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/dom/html/HTMLSlotElement.cpp#15-26
Assignee: nobody → jjong
Flags: needinfo?(jjong)
Blocks: 1409975
Has Regression Range: --- → yes
Priority: -- → P1
Attached patch patch, v1. (obsolete) — Splinter Review
This patch fixes the crash. However it hits an assertion while running try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f295d1201c34b816c17522b3a7ddc8ef8da89f37

I think it's because nsDocument::DeleteShell() [1] is called after the layout/reftests/webcomponents/ tests are finished, and "dom.webcomponents.enabled" is already set back to false, so ServoRestyleManager::ClearServoDataFromSubtree(), which uses StyleChildrenIterator, failed to clean up properly.

I wonder if it's possible to wait until nsDocument::DeleteShell ends to advance to the next test? Or is there a better way to fix ths?

[1] https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/dom/base/nsDocument.cpp#4243
(In reply to Jessica Jong [:jessica] from comment #4)
> Created attachment 8934437 [details] [diff] [review]
> patch, v1.
> 
> This patch fixes the crash. However it hits an assertion while running try:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f295d1201c34b816c17522b3a7ddc8ef8da89f37
> 
> I think it's because nsDocument::DeleteShell() [1] is called after the
> layout/reftests/webcomponents/ tests are finished, and
> "dom.webcomponents.enabled" is already set back to false, so
> ServoRestyleManager::ClearServoDataFromSubtree(), which uses
> StyleChildrenIterator, failed to clean up properly.
> 
> I wonder if it's possible to wait until nsDocument::DeleteShell ends to
> advance to the next test? Or is there a better way to fix ths?
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/dom/base/nsDocument.cpp#4243

No, that assertion is a bug. That means that something is not consistent in the flattened tree, and you're returning a wrong parent somewhere else.
Hmm, tricky.

Should we change dom.webcomponents.enabled handling so that once it is enabled for a document, it stays enabled and never gets disabled on that document.
Attached patch pref per-doc patch, v1. (obsolete) — Splinter Review
Attachment 8934863 [details] [diff] fixes the assertion in comment 4, but it fails to build on Windows:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8e8f45e27ac54580c4e3bc1be92baf156ae0039&selectedJob=150091402

I think it's because I included "nsDocument.h" in "nsTextNode.h". But I have to do this for "Func="nsDocument::IsWebComponentsEnabled"" to work in Text.webidl (Text.webidl has a explicit header file set in: https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/dom/bindings/Bindings.conf#1053)
Attached patch Part 2: pref per-doc, v2. (obsolete) — Splinter Review
Finally got this working with a very hacky-way: I added a IsWebComponentsEnabled() in nsTextNode.h so that Text.webidl can use.
Attachment #8934863 - Attachment is obsolete: true
Attachment #8934898 - Flags: review?(bugs)
Attachment #8934437 - Attachment is obsolete: true
Attachment #8934899 - Flags: review?(bugs)
See Also: → 1423250
See Also: 1423250
Duplicate of this bug: 1423250
Attachment #8934899 - Flags: review?(bugs) → review-
Comment on attachment 8934898 [details] [diff] [review]
Part 2: pref per-doc, v2.

># HG changeset patch
># User Jessica Jong <jjong@mozilla.com>
># Parent  03a7038f6d4f0808fc2621965acb2be55da0942f
>Bug 1422931 - Part 2: Make webcomponents preference per-doc. r=smaug
>
>This is to fix the case where preference is restore to false when a
>testcas
testcas
> nsContentUtils::HasDistributedChildren(nsIContent* aContent)
> {
>-  if (!IsWebComponentsEnabled() || !aContent) {
>+  if (!aContent || !nsDocument::IsWebComponentsEnabled(aContent->NodeInfo())) {
I don't understand all this NodeInfo() stuff, when you could just pass nsINode*


>+nsDocument::IsWebComponentsEnabled(JSContext* aCx, JSObject* aObject)
>+{
>+  JS::Rooted<JSObject*> obj(aCx, aObject);
>+
>+  JSAutoCompartment ac(aCx, obj);
>+  JS::Rooted<JSObject*> global(aCx, JS_GetGlobalForObject(aCx, obj));
>+  nsCOMPtr<nsPIDOMWindowInner> window =
>+    do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(global));
>+
>+  nsIDocument* doc = window ? window->GetExtantDoc() : nullptr;
>+  if (!doc) {
>+    return false;
>+  }
>+
>+  // Once it is enabled for a document, it stays enabled.
>+  if (doc->IsWebComponentsEnabled()) {
>+    return true;
>+  }
>+
>+  bool enabled = nsContentUtils::IsWebComponentsEnabled();
>+  doc->SetWebComponentsEnabled(enabled);
>+
>+  return enabled;
>+}
>+
>+bool
>+nsDocument::IsWebComponentsEnabled(mozilla::dom::NodeInfo* aNodeInfo)
So this could jsut take nsINode*, from which you get OwnerDoc() which never returns null

In NS_NewHTMLSlotElement you could then pass aNodeInfo->GetDocument() to IsWebComponentsEnabled



>+bool
>+nsTextNode::IsWebComponentsEnabled(JSContext* aCx, JSObject* aObject)
>+{
>+  JS::Rooted<JSObject*> obj(aCx, aObject);
>+
>+  JSAutoCompartment ac(aCx, obj);
>+  JS::Rooted<JSObject*> global(aCx, JS_GetGlobalForObject(aCx, obj));
>+  nsCOMPtr<nsPIDOMWindowInner> window =
>+    do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(global));
>+
>+  nsIDocument* doc = window ? window->GetExtantDoc() : nullptr;
>+  if (!doc) {
>+    return false;
>+  }
>+
>+  // Once it is enabled for a document, it stays enabled.
>+  if (doc->IsWebComponentsEnabled()) {
>+    return true;
>+  }
>+
>+  bool enabled = nsContentUtils::IsWebComponentsEnabled();
>+  doc->SetWebComponentsEnabled(enabled);
I don't understand all this. Why can you just call the nsDocument version here?
Including nsDocument.h in nsTextNode.cpp should be fine, I assume.
Or if not, move IsWebComponentsEnabled checks to nsContentUtils?
Attachment #8934898 - Flags: review?(bugs) → review-
Duplicate of this bug: 1424101
(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #13)
> Comment on attachment 8934898 [details] [diff] [review]
> Part 2: pref per-doc, v2.
> 
> ># HG changeset patch
> ># User Jessica Jong <jjong@mozilla.com>
> ># Parent  03a7038f6d4f0808fc2621965acb2be55da0942f
> >Bug 1422931 - Part 2: Make webcomponents preference per-doc. r=smaug
> >
> >This is to fix the case where preference is restore to false when a
> >testcas
> testcas
> > nsContentUtils::HasDistributedChildren(nsIContent* aContent)
> > {
> >-  if (!IsWebComponentsEnabled() || !aContent) {
> >+  if (!aContent || !nsDocument::IsWebComponentsEnabled(aContent->NodeInfo())) {
> I don't understand all this NodeInfo() stuff, when you could just pass
> nsINode*

This was for NS_NewHTMLSlotElement to pass aNodeInfo directly.

> 
> 
> >+nsDocument::IsWebComponentsEnabled(JSContext* aCx, JSObject* aObject)
> >+{
> >+  JS::Rooted<JSObject*> obj(aCx, aObject);
> >+
> >+  JSAutoCompartment ac(aCx, obj);
> >+  JS::Rooted<JSObject*> global(aCx, JS_GetGlobalForObject(aCx, obj));
> >+  nsCOMPtr<nsPIDOMWindowInner> window =
> >+    do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(global));
> >+
> >+  nsIDocument* doc = window ? window->GetExtantDoc() : nullptr;
> >+  if (!doc) {
> >+    return false;
> >+  }
> >+
> >+  // Once it is enabled for a document, it stays enabled.
> >+  if (doc->IsWebComponentsEnabled()) {
> >+    return true;
> >+  }
> >+
> >+  bool enabled = nsContentUtils::IsWebComponentsEnabled();
> >+  doc->SetWebComponentsEnabled(enabled);
> >+
> >+  return enabled;
> >+}
> >+
> >+bool
> >+nsDocument::IsWebComponentsEnabled(mozilla::dom::NodeInfo* aNodeInfo)
> So this could jsut take nsINode*, from which you get OwnerDoc() which never
> returns null
> 
> In NS_NewHTMLSlotElement you could then pass aNodeInfo->GetDocument() to
> IsWebComponentsEnabled

Ok, will change the function to take nsINode* instead.

> 
> 
> 
> >+bool
> >+nsTextNode::IsWebComponentsEnabled(JSContext* aCx, JSObject* aObject)
> >+{
> >+  JS::Rooted<JSObject*> obj(aCx, aObject);
> >+
> >+  JSAutoCompartment ac(aCx, obj);
> >+  JS::Rooted<JSObject*> global(aCx, JS_GetGlobalForObject(aCx, obj));
> >+  nsCOMPtr<nsPIDOMWindowInner> window =
> >+    do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(global));
> >+
> >+  nsIDocument* doc = window ? window->GetExtantDoc() : nullptr;
> >+  if (!doc) {
> >+    return false;
> >+  }
> >+
> >+  // Once it is enabled for a document, it stays enabled.
> >+  if (doc->IsWebComponentsEnabled()) {
> >+    return true;
> >+  }
> >+
> >+  bool enabled = nsContentUtils::IsWebComponentsEnabled();
> >+  doc->SetWebComponentsEnabled(enabled);
> I don't understand all this. Why can you just call the nsDocument version
> here?
> Including nsDocument.h in nsTextNode.cpp should be fine, I assume.
> Or if not, move IsWebComponentsEnabled checks to nsContentUtils?

Incluing nsDocument.h in nsTextNode.cpp should be fine, let me try it.
Attached patch Part 2: pref per-doc, v3. (obsolete) — Splinter Review
Addressed review comment 13:
- Take nsINode* instead of NodeInfo* in nsDocument::IsWebComponentsEnabled
- Just use nsDocument::IsWebComponentsEnabled version in nsTextNode::IsWebComponentsEnabled
Attachment #8934898 - Attachment is obsolete: true
Attachment #8935601 - Flags: review?(bugs)
Comment on attachment 8934899 [details] [diff] [review]
Part 1: fix crash with slot, v1.

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

I assume this got r- because of Part 2?
Attachment #8934899 - Flags: review- → review?(bugs)
Comment on attachment 8934899 [details] [diff] [review]
Part 1: fix crash with slot, v1.

Sorry, this should have got r+. Must have accidentally selected -.
Attachment #8934899 - Flags: review?(bugs) → review+
Comment on attachment 8935601 [details] [diff] [review]
Part 2: pref per-doc, v3.

>+  // Check whether web components are enabled for the global of aObject.
>+  static bool IsWebComponentsEnabled(JSContext* aCx, JSObject* aObject);
>+  // Check whether web components are enabled for the global of the document
>+  // this nodeinfo comes from.
"are enabled for the document this node belongs to."
Attachment #8935601 - Flags: review?(bugs) → review+
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=60d9744e3fc44f79eecfa02cfec5f1c144e962a4

Oh no, we fail on a crashtest on Android, and the reason is similar to comment 4.

Testcases order in crashtest.list:
pref(dom.webcomponents.enabled,false) load 1422931.html
pref(dom.webcomponents.enabled,true) load 1419799.html

When 1419799.html is running, `nsSHistory::EvictOutOfRangeContentViewers` is called, which leads to `nsIDocument::ClearStaleServoData` for the previous test. But it seems that it clears it with dom.webcomponents.enabled set to true. I thought we already made the pref per document... and I'm not sure why it only happens on Android...
Backed out for crashtest failures on dom/base/crashtests/1419799.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ece11256d13763d5d6d15e00797d9afabcc8935
Flags: needinfo?(jjong)
Attached patch Part 2: pref per-doc, v4. (obsolete) — Splinter Review
I did not consider the case where pref is false at the beginning then set to true somehow.
This patch reads the pref when creating the document and value does not change for the lifetime of the document.
Attachment #8935601 - Attachment is obsolete: true
Flags: needinfo?(jjong)
Attachment #8935834 - Flags: review?(bugs)
Comment on attachment 8935834 [details] [diff] [review]
Part 2: pref per-doc, v4.

+  // Check whether web components are enabled for the global of the document
+  // this nodeinfo comes from.
+  static bool IsWebComponentsEnabled(const nsINode* aNode);
I commented about this comment already.
Attachment #8935834 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #25)
> Comment on attachment 8935834 [details] [diff] [review]
> Part 2: pref per-doc, v4.
> 
> +  // Check whether web components are enabled for the global of the document
> +  // this nodeinfo comes from.
> +  static bool IsWebComponentsEnabled(const nsINode* aNode);
> I commented about this comment already.

Oh, sorry, I did not apply the patch that was actually landed (and backed-out).
Fixed comment, carrying r+.
Attachment #8935834 - Attachment is obsolete: true
Attachment #8935837 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/be58c2e8866b
https://hg.mozilla.org/mozilla-central/rev/13dd2a7aee33
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Group: dom-core-security → core-security-release
Can you check to see if 58 is also affected? There are some reports with this signature in crash-stats for 57 and 58. I'm not sure if they are from this issue or a different one. Thanks!
Flags: needinfo?(jjong)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #31)
> Can you check to see if 58 is also affected? There are some reports with
> this signature in crash-stats for 57 and 58. I'm not sure if they are from
> this issue or a different one. Thanks!

This was a regression caused by Bug 1409975, so 57 and 58 should not be affected.
Flags: needinfo?(jjong)
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.