New issue
Advanced search Search tips

Issue 801785 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Unreachable code in objects.cc

Project Member Reported by ClusterFuzz, Jan 13 2018

Issue description

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

Fuzzer: ochang_js_fuzzer
Job Type: linux_d8_dbg
Platform Id: linux

Crash Type: Unreachable code
Crash Address: 
Crash State:
  objects.cc
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_d8_dbg&range=50552:50553

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Jan 13 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/+/8cf7223fb15d41bb40de3acdb6fb358ca469cb41 ([Liftoff] Also disassemble code on the native heap).

If this is incorrect, please remove the owner and apply the Test-Predator-Wrong-CLs label.
Status: Started (was: Assigned)
Cc: titzer@chromium.org mstarzinger@chromium.org
Components: -Blink>JavaScript Blink>JavaScript>WebAssembly
Oh, this is a nasty one. When copying code over to the native heap for wasm, we copy the relocation information as is, then patch all calls to call WasmCode objects instead. The relocation information still contains CODE_TARGET entries for those calls, hence the address of the WasmCode will be interpreted as instruction start of a Code*, and boom.
I think we would have to update the relocation info to make these entries WASM_CALL entries. Unfortunately, this cannot be done in place, since the encoding for different reloc kinds takes different space. Hence we would have to regenerate the relocation info by emitting it again using the RelocInfoWriter.


Alternatively, we could generate the reloc info directly as WASM_CALL, but the called WasmCode object is only available after moving the code to the NativeModule, so there is another time span where it would be invalid (tagged WASM_CALL, but still pointing to a Code object).

@Michi: What do you think is the right approach?
Cc: clemensh@chromium.org
 Issue 801784  has been merged into this issue.
Project Member

Comment 5 by ClusterFuzz, Jan 15 2018

Labels: OS-Mac
After offline discussion with Michi, we agreed that the second approach (emitting WASM_CALL reloc info directly) is the better approach, even though there is a small time span where that is technically incorrect. During that time span, noone should inspect the reloc info anyway.
It turns out that this approach does not work that easily, since when materializing code on the native wasm heap, we have to distinguish code targets from wasm targets.
Hence we have to either patch reloc info after materializing code on the native heap, or add special handling for CODE_TARGET reloc info attached to such code (i.e. make the RelocIterator aware that it iterates wasm code).
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 22 2018

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

commit d414d80d25248acbb7ab442a7cd6927925d65543
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Mon Jan 22 13:49:21 2018

[wasm] Fix printing of reloc info on the native heap

Tag RelocInfo which belongs to native wasm code, and fix printing to
not try to access the Code object for CODE_TARGET, but rather just
print "(wasm trampoline)".

Bug:  chromium:801785 

R=mstarzinger@chromium.org

Change-Id: I84a37f0c48ed7397cccf677b4d0f0352e5aceb9d
Reviewed-on: https://chromium-review.googlesource.com/875271
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50758}
[modify] https://crrev.com/d414d80d25248acbb7ab442a7cd6927925d65543/src/assembler.cc
[modify] https://crrev.com/d414d80d25248acbb7ab442a7cd6927925d65543/src/assembler.h
[add] https://crrev.com/d414d80d25248acbb7ab442a7cd6927925d65543/test/mjsunit/regress/wasm/regress-801785.js

Status: Fixed (was: Started)
Project Member

Comment 10 by ClusterFuzz, Jan 23 2018

ClusterFuzz has detected this issue as fixed in range 50757:50758.

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

Fuzzer: ochang_js_fuzzer
Job Type: linux_d8_dbg
Platform Id: linux

Crash Type: Unreachable code
Crash Address: 
Crash State:
  objects.cc
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_d8_dbg&range=50552:50553
Fixed: https://clusterfuzz.com/revisions?job=linux_d8_dbg&range=50757:50758

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

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 11 by ClusterFuzz, Jan 23 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 5910714631061504 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