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

Issue 853468 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Null-dereference READ in v8::internal::MarkCompactCollector::RootMarkingVisitor::VisitRootPointers

Project Member Reported by ClusterFuzz, Jun 16 2018

Issue description

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

Fuzzer: mbarbella_js_mutation
Job Type: linux_msan_d8
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  v8::internal::MarkCompactCollector::RootMarkingVisitor::VisitRootPointers
  v8::internal::WasmCompileLazyFrame::Iterate
  v8::internal::Isolate::Iterate
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_msan_d8&range=53697:53698

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Jun 16 2018

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.
Project Member

Comment 2 by ClusterFuzz, Jun 16 2018

Labels: Test-Predator-Auto-Owner
Owner: clemensh@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/v8/v8/+/b9b4b8798fba40237f53455ef37f6a94f41e16c6 ([wasm] Merge {WasmSharedModuleData} with {WasmModuleObject}).

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: mstarzinger@chromium.org jgruber@chromium.org
Just debugged a bit with mstarzinger, seems that frame constants on arm64 are off. My CL is not really the culprit, it just changes the GC timing such that the bug is observable.
I will try to add explicit GC calls and create a minimized repo tomorrow.
I recently changed something there, see:

    [masm] Don't push CodeObject when entering INTERNAL frames
    Reviewed-on: https://chromium-review.googlesource.com/1075047

and the follow-up fix:

    [mips,mips64] Fix sp and fp offsets for INTERNAL frames
    Reviewed-on: https://chromium-review.googlesource.com/1078109
I had another look over the changes in #4, they still look correct to me and shouldn't affect WasmCompileLazyFrame constants.
After adding an explicit GC in the WasmCompileLazy runtime function, it reproduces totally reliable (on arm64 simulator).

Patch:
=================================================================
diff --git a/src/runtime/runtime-wasm.cc b/src/runtime/runtime-wasm.cc
index 63ecaa53ef..d0955eb681 100644
--- a/src/runtime/runtime-wasm.cc
+++ b/src/runtime/runtime-wasm.cc
@@ -297,6 +297,9 @@ RUNTIME_FUNCTION(Runtime_WasmCompileLazy) {
 
   ClearThreadInWasmScope wasm_flag(true);
 
+  isolate->heap()->CollectAllAvailableGarbage(
+      i::GarbageCollectionReason::kTesting);
+
 #ifdef DEBUG
   StackFrameIterator it(isolate, isolate->thread_local_top());
   // On top: C entry stub.
=================================================================


Repro:
=================================================================
function asm() {
  'use asm';
  function f() { }
  return f;
}
asm();
=================================================================

Bisecting now.
Still bisecting.
Just figured out that this is the same bug that lead to the revert of the jump table for wasm: https://ci.chromium.org/buildbot/client.v8.ports/V8%20Linux%20-%20arm64%20-%20sim%20-%20gc%20stress/11659
Status: Started (was: Assigned)
Ok, bisects all the way back to b2abe2cf971a382e973a9e99124452a9bfe2005a ([wasm] Introduce specialized WasmCompileLazy frame type.).

A fix is to replace (in WasmCompileLazyFrameConstants)
  TypedFrameConstants::kFixedFrameSizeFromFp
by either
  RoundUp<16>(TypedFrameConstants::kFixedFrameSizeFromFp)
or
  TypedFrameConstants::kFixedFrameSizeFromFp + kPointerSize

I prefer the RoundUp version, will upload a fix.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 19 2018

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

commit 49946f02bbfbd294f53aaa2641184cbf049f9f3d
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Tue Jun 19 08:41:58 2018

[arm64] Fix WasmCompileLazyFrameConstants

{WasmCompileLazyFrameConstants::kFixedFrameSizeFromFp} did not
incorporate padding. This CL fixes that.

R=mstarzinger@chromium.org

No-Try: true
Bug:  chromium:853468 
Change-Id: I042e68623bdfd81c96180a39c29ecd70271ba1be
Reviewed-on: https://chromium-review.googlesource.com/1105051
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53817}
[modify] https://crrev.com/49946f02bbfbd294f53aaa2641184cbf049f9f3d/src/arm64/frame-constants-arm64.h

Labels: Merge-Request-68
Status: Fixed (was: Started)
Fixed. No backmerge required, as b2abe2cf971a382e973a9e99124452a9bfe2005a landed after the 68 branch.
Let's ensure that it's well tested in canary first, before merging to m68. 
Project Member

Comment 12 by ClusterFuzz, Jun 20 2018

ClusterFuzz has detected this issue as fixed in range 53816:53817.

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

Fuzzer: mbarbella_js_mutation
Job Type: linux_msan_d8
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  v8::internal::MarkCompactCollector::RootMarkingVisitor::VisitRootPointers
  v8::internal::WasmCompileLazyFrame::Iterate
  v8::internal::Isolate::Iterate
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_msan_d8&range=53697:53698
Fixed: https://clusterfuzz.com/revisions?job=linux_msan_d8&range=53816:53817

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

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.
Labels: -Merge-Request-68 Merge-Rejected-68
Sorry, wrong label. I wanted to add the reject label directly, since this does not require backmerge.
Project Member

Comment 14 by ClusterFuzz, Jun 20 2018

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

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

Sign in to add a comment