New issue
Advanced search Search tips

Issue 856938 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

[wasm] Protected instructions not registered after deserialization

Project Member Reported by clemensh@chromium.org, Jun 27 2018

Issue description

We seem to fail to register protected instructions (for the trap handler) after deserialization of wasm modules.

Reproducer:
============================================================
function GenerateSerializedModule() {
  const builder = new WasmModuleBuilder();
  builder.addMemory(1, 1);
  builder.addFunction('main', kSig_i_i)
      .addBody([kExprGetLocal, 0, kExprI32LoadMem, 0, 0])
      .exportFunc();
  const wire_bytes = builder.toBuffer();
  const module = new WebAssembly.Module(wire_bytes);
  const buffer = %SerializeWasmModule(module);
  return [wire_bytes, buffer];
}
const [wire_bytes, buffer] = GenerateSerializedModule();
module = %DeserializeWasmModule(buffer, wire_bytes);
const instance = new WebAssembly.Instance(module);

instance.exports.main(kPageSize - 3);
============================================================

This results in a segfault. It's not a security bug, since we reliably get a segfault instead of the wasm trap.

Bisects to fffa33179d4105c5661f9d47d66bbb54fa9e4ad1 ([wasm] Register and release protected instructions only once). In that CL, we skipped registering protected instructions at instantiation, and forgot to add it in the deserialization path.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 27 2018

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

commit ce2d01bca3aad086ff7cea0353e7f90f95910dc0
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Wed Jun 27 12:22:10 2018

[wasm] Store protected instructions in an OwnedVector

We currently store the protected instructions per code object in a
{std::unique_ptr<std::vector<ProtectedInstructionData>>}. This wastes
memory, because it requires two heap allocations, plus the vector might
over-allocate (and it currently does, because it is filled dynamically
during compilation).
This CL changes that to store the protected instructions in an
{OwnedVector}. This requires one copy after generating the list of
{ProtectedInstructionData} in an {std::vector} during compilation, but
saves memory afterwards.

R=mstarzinger@chromium.org

Bug:  chromium:856938 
Change-Id: Ie290a17dc32f27fbbfe0c000a52297181c954550
Reviewed-on: https://chromium-review.googlesource.com/1116701
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54052}
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/compiler/pipeline.cc
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/compiler/wasm-compiler.cc
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/compiler/wasm-compiler.h
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/source-position-table.cc
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/vector.h
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/wasm/baseline/liftoff-compiler.cc
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/wasm/baseline/liftoff-compiler.h
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/wasm/wasm-code-manager.cc
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/wasm/wasm-code-manager.h
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/src/wasm/wasm-serialization.cc
[modify] https://crrev.com/ce2d01bca3aad086ff7cea0353e7f90f95910dc0/test/cctest/wasm/test-run-wasm.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 28 2018

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

commit 37ca8c3d2d7884d959a34b07249d7b5227f5670f
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Thu Jun 28 13:02:34 2018

[wasm] Remove friendship between NativeModule and (de)serializer

This CL removes the friendship between {NativeModule} and
{NativeModuleSerializer}/{NativeModuleDeserializer}.
Instead, it adds a new public method ({AddDeserializedCode}) which is
being called from the deserializer.

Drive-by: Unify the argument order to {AddCode}, {AddOwnedCode} and
{WasmCode}.

R=mstarzinger@chromium.org

Bug:  chromium:856938 
Change-Id: I88943c90c45650e21ae6bc17395a17f86319c046
Reviewed-on: https://chromium-review.googlesource.com/1117075
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54084}
[modify] https://crrev.com/37ca8c3d2d7884d959a34b07249d7b5227f5670f/src/compiler/pipeline.cc
[modify] https://crrev.com/37ca8c3d2d7884d959a34b07249d7b5227f5670f/src/wasm/baseline/liftoff-compiler.cc
[modify] https://crrev.com/37ca8c3d2d7884d959a34b07249d7b5227f5670f/src/wasm/wasm-code-manager.cc
[modify] https://crrev.com/37ca8c3d2d7884d959a34b07249d7b5227f5670f/src/wasm/wasm-code-manager.h
[modify] https://crrev.com/37ca8c3d2d7884d959a34b07249d7b5227f5670f/src/wasm/wasm-serialization.cc
[modify] https://crrev.com/37ca8c3d2d7884d959a34b07249d7b5227f5670f/test/cctest/wasm/test-run-wasm.cc
[modify] https://crrev.com/37ca8c3d2d7884d959a34b07249d7b5227f5670f/test/unittests/wasm/wasm-code-manager-unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 28 2018

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

commit 15428f19f2dc0af9631db93dd96787017d86a850
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Thu Jun 28 13:06:45 2018

[wasm] Register protected instructions after deserialization

R=mstarzinger@chromium.org

Bug:  chromium:856938 
Change-Id: I57699de23b5c35a531c7601fd14a91f075abb0da
Reviewed-on: https://chromium-review.googlesource.com/1117182
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54085}
[modify] https://crrev.com/15428f19f2dc0af9631db93dd96787017d86a850/src/wasm/wasm-code-manager.cc
[modify] https://crrev.com/15428f19f2dc0af9631db93dd96787017d86a850/test/mjsunit/wasm/compiled-module-serialization.js

Status: Fixed (was: Started)

Sign in to add a comment