New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 859809 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment

DCHECK failure in !object->IsFiller() in mark-compact.cc

Project Member Reported by ClusterFuzz, Jul 3

Issue description

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

Fuzzer: ochang_js_fuzzer
Job Type: linux_d8_dbg
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  !object->IsFiller() in mark-compact.cc
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_d8_dbg&range=53510:53511

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Jul 3

Labels: Test-Predator-Auto-Owner
Owner: szuend@google.com
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/v8/v8/+/aff803454745935ba7843257fcf10dce41dc33b1 (Reland "[array] Implement Array.p.sort in Torque").

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Cc: marja@chromium.org
So what happens is:

1) We do LeftTrimFixedArray. Incremental marking is not in progress, so we don't do NotifyLeftTrimming. But we create the filler object.

2) The filler object is put into the marking worklist.

Backtrace: 

    libv8_libbase.so(v8::base::debug::StackTrace::StackTrace()+0x1e) [0x7f8931ca554e]
    libv8.so(v8::internal::MarkCompactCollector::MarkingWorklist::Push(v8::internal::HeapObject*)+0x71) [0x7f8930f25131]
    libv8.so(v8::internal::MarkCompactCollector::MarkRootObject(v8::internal::Root, v8::internal::HeapObject*)+0x51) [0x7f8930f64831]
    libv8.so(v8::internal::MarkCompactCollector::RootMarkingVisitor::MarkObjectByPointer(v8::internal::Root, v8::internal::Object**)+0x64) [0x7f8930f647d4]
    libv8.so(v8::internal::MarkCompactCollector::RootMarkingVisitor::VisitRootPointer(v8::internal::Root, char const*, v8::internal::Object**)+0x27) [0x7f8930f64767]
    libv8.so(v8::internal::StandardFrame::IterateCompiledFrame(v8::internal::RootVisitor*) const+0x81a) [0x7f8930e7fe4a]
    libv8.so(v8::internal::StubFrame::Iterate(v8::internal::RootVisitor*) const+0x28) [0x7f8930e80098]
    libv8.so(v8::internal::Isolate::Iterate(v8::internal::RootVisitor*, v8::internal::ThreadLocalTop*)+0x2b1) [0x7f893109e7a1]
    libv8.so(v8::internal::Isolate::Iterate(v8::internal::RootVisitor*)+0x35) [0x7f893109e855]
    libv8.so(v8::internal::Heap::IterateStrongRoots(v8::internal::RootVisitor*, v8::internal::VisitMode)+0xd2) [0x7f8930f060c2]

3) The filler object is retrieved from the marking worklist -> DCHECK fires.
Project Member

Comment 4 by ClusterFuzz, Jul 3

Components: Blink>JavaScript>GC
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.
More info: the pointer to the filler object is inside the pointer spill slots & locals of the Frame:

void StandardFrame::IterateCompiledFrame(RootVisitor* v) const {
  ...
  // Visit pointer spill slots and locals.
  for (unsigned index = 0; index < stack_slots; index++) {
    int byte_index = index >> kBitsPerByteLog2;
    int bit_index = index & (kBitsPerByte - 1);
    if ((safepoint_bits[byte_index] & (1U << bit_index)) != 0) {
      v->VisitRootPointer(Root::kTop, nullptr, parameters_limit + index);
    }
  }
  ...

}

So now, one of the spill slots / locals is directly a pointer to the filler object (i.e., what used to be the FixedArray* but is now a filler object).

Umm, now I'm reaching the end of my knowledge; how is this supposed to work? Should the spill slots / locals not contain the old FixedArray* or should we filter out filler objects on this code path?
Cc: hpayer@chromium.org u...@chromium.org
ulan@, hpayer@: What's the gc contract, is it OK that the spill slots / locals contain a pointer to what used to be a FixedArray (but was then left-trimmed)?

I.e., e.g., for spill slots, they might be dead values, do we need to carefully nullify any registers which contain the original FixedArray (which is now the filler)?
Update based on discussions w/ szuend@, cbruni@, jgruber@. Located the FixedArray*, it's actually a local, and live, so the above speculation is not relevant.

Going to implement a better heap / stack check which verifies we don't have pointers to the orig. FixedArray just after left trimming.
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/f7bad08397d922d7fe0bc10624f517c6f5412595

commit f7bad08397d922d7fe0bc10624f517c6f5412595
Author: Simon Zünd <szuend@google.com>
Date: Tue Jul 03 12:42:20 2018

[array] Revert "Implement Array.p.sort in Torque"

This CL is a manual revert of the Array.p.sort Torque QuickSort
implementation.

The plan is to ship TimSort in either Chromium 69 or 70 and not ship
Torque-QuickSort at all (to keep disruption to a minimum). For this
reason we revert back to the implementation in array.js.

R=jgruber@chromium.org

Bug:  chromium:859809 , v8:7382
Change-Id: I92eb70408883f51d98311e78642f554316bc1e76
Reviewed-on: https://chromium-review.googlesource.com/1124334
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Simon Zünd <szuend@google.com>
Cr-Commit-Position: refs/heads/master@{#54166}
[modify] https://crrev.com/f7bad08397d922d7fe0bc10624f517c6f5412595/src/bootstrapper.cc
[modify] https://crrev.com/f7bad08397d922d7fe0bc10624f517c6f5412595/src/builtins/array-sort.tq
[modify] https://crrev.com/f7bad08397d922d7fe0bc10624f517c6f5412595/src/debug/debug-evaluate.cc
[modify] https://crrev.com/f7bad08397d922d7fe0bc10624f517c6f5412595/src/js/array.js
[modify] https://crrev.com/f7bad08397d922d7fe0bc10624f517c6f5412595/test/message/fail/non-alphanum.out

Status: Fixed (was: Assigned)
Update: The bug is caused by the QuickSort impl. of Torque. We keep a pointer to the Fixed{Double}Array of the JSArray that will be sorted. This pointer is used to determine if execution can continue on a fast-path after a JavaScript call to the comparison function or to ToString.

The clusterfuzz case calls Array.p.shift and gc() inside the comparison function, causing a left trimming of the FixedArray, resulting in our elements pointer pointing to the filler object.

The solution will be to properly reload the elements pointer after returning
from JavaScript. For now we reverted back to JavaScript sorting.
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/26ac0729905dc406d96478c71ce82caef31dc2f8

commit 26ac0729905dc406d96478c71ce82caef31dc2f8
Author: Simon Zünd <szuend@google.com>
Date: Tue Jul 03 14:16:14 2018

[array] Add regression test that causes left trimming while sorting

This CL adds a regression test that will check that the elements
pointer is properly reloaded after the JavaScript comparison
function is called during Array.p.sort.

R=jgruber@chromium.org

Bug:  chromium:859809 
Change-Id: I15f55fcc1906bd8d0751596e5457367a643b92da
Reviewed-on: https://chromium-review.googlesource.com/1124475
Commit-Queue: Simon Zünd <szuend@google.com>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54174}
[add] https://crrev.com/26ac0729905dc406d96478c71ce82caef31dc2f8/test/mjsunit/regress/regress-crbug-859809.js

Project Member

Comment 11 by sheriffbot@chromium.org, Jul 3

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 12 by ClusterFuzz, Jul 4

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 5569461449654272 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 13 by ClusterFuzz, Jul 4

ClusterFuzz has detected this issue as fixed in range 54165:54166.

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

Fuzzer: ochang_js_fuzzer
Job Type: linux_d8_dbg
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  !object->IsFiller() in mark-compact.cc
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_d8_dbg&range=53510:53511
Fixed: https://clusterfuzz.com/revisions?job=linux_d8_dbg&range=54165:54166

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

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

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/67c1079663c3e218fb72d490c5b187fc9f6eab1c

commit 67c1079663c3e218fb72d490c5b187fc9f6eab1c
Author: Marja Hölttä <marja@chromium.org>
Date: Wed Jul 04 16:00:56 2018

[heap] After left trimming, verify that we don't have pointers to the filler object.

BUG= chromium:859809 

Change-Id: I9ac81585c7f141cb1839ff7de237e0930f44e634
Reviewed-on: https://chromium-review.googlesource.com/1124450
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54219}
[modify] https://crrev.com/67c1079663c3e218fb72d490c5b187fc9f6eab1c/src/elements.cc
[modify] https://crrev.com/67c1079663c3e218fb72d490c5b187fc9f6eab1c/src/heap/heap.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/eb98d5c5c6caf7684b87841b0c56e34e17f60780

commit eb98d5c5c6caf7684b87841b0c56e34e17f60780
Author: Marja Hölttä <marja@chromium.org>
Date: Thu Jul 12 09:46:27 2018

[heap] Remove obsolete left trimming code + comments.

Follow-up to r54219 ( https://chromium-review.googlesource.com/1124450 )

They're relevant if we also iterate the heap and check there are no pointers to
the original FixedArrayBase, but in the landed version of that CL we don't do
that.

BUG= chromium:859809 

Change-Id: Iffd8b76e74b6690cde961d4c542cb16ddd934f33
Reviewed-on: https://chromium-review.googlesource.com/1131123
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54396}
[modify] https://crrev.com/eb98d5c5c6caf7684b87841b0c56e34e17f60780/src/elements.cc

Project Member

Comment 16 by sheriffbot@chromium.org, Jul 28

Labels: Pri-1
Project Member

Comment 17 by sheriffbot@chromium.org, Oct 9

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment