New issue
Advanced search Search tips

Issue 843069 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug

Blocking:
issue 776656



Sign in to add a comment

Null-dereference READ in blink::Node::LazyReattachIfAttached

Project Member Reported by ClusterFuzz, May 15 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5678839037165568

Fuzzer: marty_html_twiddler
Job Type: linux_ubsan_vptr_content_shell_drt
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  blink::Node::LazyReattachIfAttached
  blink::HTMLSlotElement::DetachLayoutTree
  blink::ContainerNode::DetachLayoutTree
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_vptr_content_shell_drt&range=558606:558609

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5678839037165568

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, May 15 2018

Components: Blink>DOM Blink>HTML
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

Comment 2 by tkent@chromium.org, May 15 2018

Owner: hayato@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, May 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6836320ba930e1d75a86359f5bc4cf789d84500f

commit 6836320ba930e1d75a86359f5bc4cf789d84500f
Author: Hayato Ito <hayato@chromium.org>
Date: Wed May 16 15:04:25 2018

Fix a crash caused by touching outdated assigned_nodes

This is a tentative fix for several crashes cluster fuzzer reported.
It looks HTMLSlotElement::DetachLayoutTree is touching dirty
assigned_nodes.

It's still hard to have a minimized test case, so I'll work on that later
to investigate further. I've added TODO comment there.

Bug:  776656 , 843069 , 843261 
Change-Id: I9093e170e3c851b89612f40e764bfdbdc1532d4f
Reviewed-on: https://chromium-review.googlesource.com/1061274
Commit-Queue: Hayato Ito <hayato@chromium.org>
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559096}
[modify] https://crrev.com/6836320ba930e1d75a86359f5bc4cf789d84500f/third_party/blink/renderer/core/html/html_slot_element.cc

Comment 4 by hayato@chromium.org, May 17 2018

Status: Fixed (was: Assigned)
This should be fixed https://chromium-review.googlesource.com/1061274.

I'll add a test case to Blink later.

Comment 5 by hayato@chromium.org, May 17 2018

Cc: hayato@chromium.org
 Issue 843261  has been merged into this issue.

Comment 6 by hayato@chromium.org, May 17 2018

 Issue 843071  has been merged into this issue.

Comment 7 by hayato@chromium.org, May 17 2018

 Issue 843067  has been merged into this issue.

Comment 8 by hayato@chromium.org, May 17 2018

Update:

I've figure out the root cause.

Regarding blink::HTMLSlotElement::DetachLayoutTree(), there is a case where RecalcAssignment() would be called whine Node::DetachLayoutTree is being executed. See the trace below.

That means if we don't use AssignedNodes() in HTMLSlotElement::DetachLayoutTree(),
assigned_nodes will be modified while in iterating assigned_nodes_.


 ├─ Document::UpdateStyleAndLayout [document.cc:2355]
 │  ├─ Document::UpdateStyleAndLayoutTree [document.cc:2074]
">>> summary.lastChild.remove()"
 ├─ ContainerNode::DetachLayoutTree [container_node.cc:957] this: 0xa26f74a3cc0 DIV id=second () [path: <#document> / <HTML> / <BODY> / <SUMMARY> / (1)<DIV id=second>]
 │  ├─ Node::DetachLayoutTree [node.cc:1166] this: 0xa26f74a3cc0 DIV id=second (.) [path: <#document> / <HTML> / <BODY> / <SUMMARY> / (1)<DIV id=second>] context:
">>> summary.remove()"
 ├─ ContainerNode::DetachLayoutTree [container_node.cc:957] this: 0xa26f74a3900 #shadowroot(UserAgent)(NeedsSlotAssignmentRecalc) () [path: <#document> / <HTML> / <BODY> / <SUMMARY> / <::  UA-shadowroot>]
 │  ├─ ContainerNode::DetachLayoutTree [container_node.cc:957] this: 0xa26f74a3a80 DIV pseudo=-webkit-details-marker id=details-marker () [path: <#document> / <HTML> / <BODY> / <SUMMARY> / <::  UA-shadowroot> / <DIV pseudo=-webkit-details-marker id=details-marker>]
 │  │  ├─ Node::DetachLayoutTree [node.cc:1166] this: 0xa26f74a3a80 DIV pseudo=-webkit-details-marker id=details-marker (.) [path: <#document> / <HTML> / <BODY> / <SUMMARY> / <::  UA-shadowroot> / <DIV pseudo=-webkit-details-marker id=details-marker>] context:
 │  ├─ HTMLSlotElement::DetachLayoutTree [html_slot_element.cc:367] this: 0xa26f74a3b20 SLOT name=user-agent-default-slot () [path: <#document> / <HTML> / <BODY> / <SUMMARY> / <::  UA-shadowroot> / <SLOT name=user-agent-default-slot>]
 │  │  ├─ [html_slot_element.cc:383]  > node->LazyReattachIfAttached()
 │  │  ├─ [html_slot_element.cc:384]  > node: 0xa26f74a3c20 (DIV id=first style=display: list-item () [path: <#document> / <HTML> / <BODY> / <SUMMARY> / <DIV id=first style=display: list-item>])
 │  │  ├─ ContainerNode::DetachLayoutTree [container_node.cc:957] this: 0xa26f74a3c20 DIV id=first style=display: list-item () [path: <#document> / <HTML> / <BODY> / <SUMMARY> / <DIV id=first style=display: list-item>]
 │  │  │  ├─ Node::DetachLayoutTree [node.cc:1166] this: 0xa26f74a3c20 DIV id=first style=display: list-item (.) [path: <#document> / <HTML> / <BODY> / <SUMMARY> / <DIV id=first style=display: list-item>] context:
 │  │  │  │  ├─ SlotAssignment::RecalcAssignment [slot_assignment.cc:218]
#0 0x7efdfd19b47c base::debug::StackTrace::StackTrace()
#1 0x7efdf6e87a88 blink::SlotAssignment::RecalcAssignment()
#2 0x7efdf728b0ad blink::HTMLSlotElement::AssignedNodeNextTo()
#3 0x7efdf6e2b160 blink::LayoutTreeBuilderTraversal::NextSibling()
#4 0x7efdf6e2ba02 blink::PseudoAwareNextSibling()
#5 0x7efdf6e2b891 blink::LayoutTreeBuilderTraversal::NextSkippingChildren()
#6 0x7efdf6e2bc81 blink::LayoutTreeBuilderTraversal::Next()
#7 0x7efdf72af181 blink::ListItemOrdinal::NextListItem()
#8 0x7efdf72afa8f blink::ListItemOrdinal::InvalidateOrdinalsAfter()
#9 0x7efdf72b0041 blink::ListItemOrdinal::ItemInsertedOrRemoved()
#10 0x7efdf7511582 blink::LayoutObjectChildList::RemoveChildNode()
#11 0x7efdf74ff47f blink::LayoutObject::RemoveChild()
#12 0x7efdf746e41f blink::LayoutBlockFlow::RemoveChild()
#13 0x7efdf750c584 blink::LayoutObject::WillBeDestroyed()
#14 0x7efdf74a2604 blink::LayoutBoxModelObject::WillBeDestroyed()
#15 0x7efdf74843cc blink::LayoutBox::WillBeDestroyed()
#16 0x7efdf74e9f1d blink::LayoutListItem::WillBeDestroyed()
#17 0x7efdf750d702 blink::LayoutObject::Destroy()
#18 0x7efdf750d6c2 blink::LayoutObject::DestroyAndCleanupAnonymousWrappers()
#19 0x7efdf6e485e5 blink::Node::DetachLayoutTree()
#20 0x7efdf6d7eef8 blink::ContainerNode::DetachLayoutTree()
#21 0x7efdf6de813f blink::Element::DetachLayoutTree()
#22 0x7efdf728bdd1 blink::HTMLSlotElement::DetachLayoutTree()
#23 0x7efdf6d7eedf blink::ContainerNode::DetachLayoutTree()
#24 0x7efdf6e7fdb5 blink::ShadowRoot::DetachLayoutTree()
#25 0x7efdf6de8134 blink::Element::DetachLayoutTree()
#26 0x7efdf6d7ddb9 blink::ContainerNode::RemoveBetween()
#27 0x7efdf6d7cb8c blink::ContainerNode::RemoveChild()
#28 0x7efdf7a785d9 blink::V8Element::removeMethodCallback()
#29 0x7efdf8339403 v8::internal::FunctionCallbackArguments::Call()
#30 0x7efdf8337762 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#31 0x7efdf8335b28 v8::internal::Builtin_Impl_HandleApiCall()
#32 0x7efdf833556d v8::internal::Builtin_HandleApiCall()
#33 0x00bf5cabfb84 <unknown>

Project Member

Comment 9 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75c643af6262516ed5f805e3199e8434c5777f5c

commit 75c643af6262516ed5f805e3199e8434c5777f5c
Author: Hayato Ito <hayato@chromium.org>
Date: Thu May 17 07:56:24 2018

Add a repro test case for  crbug.com/843069 

This is a follow-up CL for https://crrev.com/c1061274.

Add a test case, and update the comment to explain why we need to make sure assignment
is recalculated before iterating assigned_nodes.

TBR=kochi,rakina

Bug:  776656 , 843069 
Change-Id: I3707f6fd93b6f1709b34773a63f8ca91be67e824
Reviewed-on: https://chromium-review.googlesource.com/1062990
Reviewed-by: Hayato Ito <hayato@chromium.org>
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Hayato Ito <hayato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559448}
[add] https://crrev.com/75c643af6262516ed5f805e3199e8434c5777f5c/third_party/WebKit/LayoutTests/shadow-dom/crashes/detach-node-call-recalc-assignment-crash.html
[modify] https://crrev.com/75c643af6262516ed5f805e3199e8434c5777f5c/third_party/blink/renderer/core/html/html_slot_element.cc

Project Member

Comment 10 by ClusterFuzz, May 17 2018

Labels: -Reproducible Unreproducible
ClusterFuzz testcase 5678839037165568 appears to be flaky, updating reproducibility label.
Blocking: 776656

Sign in to add a comment