From ff6941966e1eb9258453766da2f436373774cbef Mon Sep 17 00:00:00 2001 From: titzer Date: Tue, 11 Oct 2016 05:40:24 -0700 Subject: [PATCH] [wasm] Canonicalize function signature indices for matching in indirect calls. R=bradnelson@chromium.org, ahaas@chromium.org, clemensh@chromium.org BUG=chromium:575167 Review-Url: https://codereview.chromium.org/2403093002 Cr-Commit-Position: refs/heads/master@{#40169} --- BUILD.gn | 2 ++ src/api.cc | 1 + src/compiler/wasm-compiler.cc | 4 ++- src/v8.gyp | 2 ++ src/wasm/module-decoder.cc | 27 ++++++++++++--- src/wasm/module-decoder.h | 7 ++++ src/wasm/signature-map.cc | 51 +++++++++++++++++++++++++++ src/wasm/signature-map.h | 41 ++++++++++++++++++++++ src/wasm/wasm-interpreter.cc | 11 +++++- src/wasm/wasm-module-builder.h | 2 ++ src/wasm/wasm-module.cc | 4 ++- src/wasm/wasm-module.h | 10 +++--- test/cctest/wasm/test-run-wasm.cc | 43 +++++++++++++++++++++++ test/cctest/wasm/wasm-run-utils.h | 9 +++-- test/mjsunit/wasm/indirect-calls.js | 53 ++++++++++++++++++++++++++--- 15 files changed, 245 insertions(+), 22 deletions(-) create mode 100644 src/wasm/signature-map.cc create mode 100644 src/wasm/signature-map.h diff --git a/BUILD.gn b/BUILD.gn index 37d45d650a..9f2c2e72d4 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1696,6 +1696,8 @@ v8_source_set("v8_base") { "src/wasm/leb-helper.h", "src/wasm/module-decoder.cc", "src/wasm/module-decoder.h", + "src/wasm/signature-map.cc", + "src/wasm/signature-map.h", "src/wasm/wasm-debug.cc", "src/wasm/wasm-debug.h", "src/wasm/wasm-external-refs.cc", diff --git a/src/api.cc b/src/api.cc index adb62f54fc..9b31aed774 100644 --- a/src/api.cc +++ b/src/api.cc @@ -73,6 +73,7 @@ #include "src/version.h" #include "src/vm-state-inl.h" #include "src/wasm/wasm-module.h" +#include "src/wasm/wasm-result.h" namespace v8 { diff --git a/src/compiler/wasm-compiler.cc b/src/compiler/wasm-compiler.cc index 9ff174d3a7..c52b452692 100644 --- a/src/compiler/wasm-compiler.cc +++ b/src/compiler/wasm-compiler.cc @@ -2148,9 +2148,11 @@ Node* WasmGraphBuilder::CallIndirect(uint32_t index, Node** args, Node*** rets, Int32Constant(kPointerSizeLog2)), Int32Constant(fixed_offset)), *effect_, *control_); + int32_t key = module_->module->function_tables[0].map.Find(sig); + DCHECK_GE(key, 0); Node* sig_match = graph()->NewNode(machine->Word32Equal(), - BuildChangeSmiToInt32(load_sig), Int32Constant(index)); + BuildChangeSmiToInt32(load_sig), Int32Constant(key)); trap_->AddTrapIfFalse(wasm::kTrapFuncSigMismatch, sig_match, position); } diff --git a/src/v8.gyp b/src/v8.gyp index 89fbaf3d8f..a0ab2337c9 100644 --- a/src/v8.gyp +++ b/src/v8.gyp @@ -1247,6 +1247,8 @@ 'wasm/leb-helper.h', 'wasm/module-decoder.cc', 'wasm/module-decoder.h', + 'wasm/signature-map.cc', + 'wasm/signature-map.h', 'wasm/wasm-debug.cc', 'wasm/wasm-debug.h', 'wasm/wasm-external-refs.cc', diff --git a/src/wasm/module-decoder.cc b/src/wasm/module-decoder.cc index 6b4861e8ee..0f8637f32e 100644 --- a/src/wasm/module-decoder.cc +++ b/src/wasm/module-decoder.cc @@ -301,7 +301,7 @@ class ModuleDecoder : public Decoder { import->index = static_cast(module->function_tables.size()); module->function_tables.push_back( - {0, 0, std::vector(), true, false}); + {0, 0, std::vector(), true, false, SignatureMap()}); expect_u8("element type", 0x20); WasmIndirectFunctionTable* table = &module->function_tables.back(); consume_resizable_limits("element count", "elements", kMaxUInt32, @@ -363,10 +363,12 @@ class ModuleDecoder : public Decoder { if (table_count > 1) { error(pos, pos, "invalid table count %d, maximum 1", table_count); } + if (module->function_tables.size() < 1) { + module->function_tables.push_back( + {0, 0, std::vector(), false, false, SignatureMap()}); + } for (uint32_t i = 0; ok() && i < table_count; i++) { - module->function_tables.push_back( - {0, 0, std::vector(), false, false}); WasmIndirectFunctionTable* table = &module->function_tables.back(); expect_u8("table type", kWasmAnyFunctionTypeForm); consume_resizable_limits("table elements", "elements", kMaxUInt32, @@ -503,8 +505,17 @@ class ModuleDecoder : public Decoder { if (section_iter.section_code() == kElementSectionCode) { uint32_t element_count = consume_u32v("element count"); for (uint32_t i = 0; ok() && i < element_count; ++i) { + const byte* pos = pc(); uint32_t table_index = consume_u32v("table index"); - if (table_index != 0) error("illegal table index != 0"); + if (table_index != 0) { + error(pos, pos, "illegal table index %u != 0", table_index); + } + WasmIndirectFunctionTable* table = nullptr; + if (table_index >= module->function_tables.size()) { + error(pos, pos, "out of bounds table index %u", table_index); + } else { + table = &module->function_tables[table_index]; + } WasmInitExpr offset = consume_init_expr(module, kAstI32); uint32_t num_elem = consume_u32v("number of elements"); std::vector vector; @@ -513,7 +524,13 @@ class ModuleDecoder : public Decoder { init->entries.reserve(SafeReserve(num_elem)); for (uint32_t j = 0; ok() && j < num_elem; j++) { WasmFunction* func = nullptr; - init->entries.push_back(consume_func_index(module, &func)); + uint32_t index = consume_func_index(module, &func); + init->entries.push_back(index); + if (table && index < module->functions.size()) { + // Canonicalize signature indices during decoding. + // TODO(titzer): suboptimal, redundant when verifying only. + table->map.FindOrInsert(module->functions[index].sig); + } } } diff --git a/src/wasm/module-decoder.h b/src/wasm/module-decoder.h index 22a313cec3..1e2b32a23c 100644 --- a/src/wasm/module-decoder.h +++ b/src/wasm/module-decoder.h @@ -7,10 +7,17 @@ #include "src/wasm/ast-decoder.h" #include "src/wasm/wasm-module.h" +#include "src/wasm/wasm-result.h" namespace v8 { namespace internal { namespace wasm { + +typedef Result ModuleResult; +typedef Result FunctionResult; +typedef std::vector> FunctionOffsets; +typedef Result FunctionOffsetsResult; + // Decodes the bytes of a WASM module between {module_start} and {module_end}. V8_EXPORT_PRIVATE ModuleResult DecodeWasmModule(Isolate* isolate, Zone* zone, const byte* module_start, diff --git a/src/wasm/signature-map.cc b/src/wasm/signature-map.cc new file mode 100644 index 0000000000..e7f8b2fc94 --- /dev/null +++ b/src/wasm/signature-map.cc @@ -0,0 +1,51 @@ +// Copyright 2016 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "src/wasm/signature-map.h" + +namespace v8 { +namespace internal { +namespace wasm { + +uint32_t SignatureMap::FindOrInsert(FunctionSig* sig) { + auto pos = map_.find(sig); + if (pos != map_.end()) { + return pos->second; + } else { + uint32_t index = next_++; + map_[sig] = index; + return index; + } +} + +int32_t SignatureMap::Find(FunctionSig* sig) const { + auto pos = map_.find(sig); + if (pos != map_.end()) { + return static_cast(pos->second); + } else { + return -1; + } +} + +bool SignatureMap::CompareFunctionSigs::operator()(FunctionSig* a, + FunctionSig* b) const { + if (a == b) return false; + if (a->return_count() < b->return_count()) return true; + if (a->return_count() > b->return_count()) return false; + if (a->parameter_count() < b->parameter_count()) return true; + if (a->parameter_count() > b->parameter_count()) return false; + for (size_t r = 0; r < a->return_count(); r++) { + if (a->GetReturn(r) < b->GetReturn(r)) return true; + if (a->GetReturn(r) > b->GetReturn(r)) return false; + } + for (size_t p = 0; p < a->parameter_count(); p++) { + if (a->GetParam(p) < b->GetParam(p)) return true; + if (a->GetParam(p) > b->GetParam(p)) return false; + } + return false; +} + +} // namespace wasm +} // namespace internal +} // namespace v8 diff --git a/src/wasm/signature-map.h b/src/wasm/signature-map.h new file mode 100644 index 0000000000..28c9fd63ed --- /dev/null +++ b/src/wasm/signature-map.h @@ -0,0 +1,41 @@ +// Copyright 2016 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef V8_WASM_SIGNATURE_MAP_H_ +#define V8_WASM_SIGNATURE_MAP_H_ + +#include + +#include "src/signature.h" +#include "src/wasm/wasm-opcodes.h" + +namespace v8 { +namespace internal { +namespace wasm { + +// A signature map canonicalizes signatures into a range of indices so that +// two different {FunctionSig} instances with the same contents map to the +// same index. +class SignatureMap { + public: + // Gets the index for a signature, assigning a new index if necessary. + uint32_t FindOrInsert(FunctionSig* sig); + + // Gets the index for a signature, returning {-1} if not found. + int32_t Find(FunctionSig* sig) const; + + private: + // TODO(wasm): use a hashmap instead of an ordered map? + struct CompareFunctionSigs { + bool operator()(FunctionSig* a, FunctionSig* b) const; + }; + uint32_t next_ = 0; + std::map map_; +}; + +} // namespace wasm +} // namespace internal +} // namespace v8 + +#endif // V8_WASM_SIGNATURE_MAP_H_ diff --git a/src/wasm/wasm-interpreter.cc b/src/wasm/wasm-interpreter.cc index 2ac681eff2..8064bf2765 100644 --- a/src/wasm/wasm-interpreter.cc +++ b/src/wasm/wasm-interpreter.cc @@ -1419,7 +1419,16 @@ class ThreadImpl : public WasmInterpreter::Thread { if (target == nullptr) { return DoTrap(kTrapFuncInvalid, pc); } else if (target->function->sig_index != operand.index) { - return DoTrap(kTrapFuncSigMismatch, pc); + // If not an exact match, we have to do a canonical check. + // TODO(titzer): make this faster with some kind of caching? + const WasmIndirectFunctionTable* table = + &module()->function_tables[0]; + int function_key = table->map.Find(target->function->sig); + if (function_key < 0 || + (function_key != + table->map.Find(module()->signatures[operand.index]))) { + return DoTrap(kTrapFuncSigMismatch, pc); + } } DoCall(target, &pc, pc + 1 + operand.length, &limit); diff --git a/src/wasm/wasm-module-builder.h b/src/wasm/wasm-module-builder.h index ed08923d35..4f463b01df 100644 --- a/src/wasm/wasm-module-builder.h +++ b/src/wasm/wasm-module-builder.h @@ -229,6 +229,8 @@ class V8_EXPORT_PRIVATE WasmModuleBuilder : public ZoneObject { // Writing methods. void WriteTo(ZoneBuffer& buffer) const; + // TODO(titzer): use SignatureMap from signature-map.h here. + // This signature map is zone-allocated, but the other is heap allocated. struct CompareFunctionSigs { bool operator()(FunctionSig* a, FunctionSig* b) const; }; diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc index f215f85176..3a4ac2dbef 100644 --- a/src/wasm/wasm-module.cc +++ b/src/wasm/wasm-module.cc @@ -2059,7 +2059,9 @@ Handle BuildFunctionTable(Isolate* isolate, uint32_t index, isolate->factory()->NewFixedArray(2 * table->max_size, TENURED); for (uint32_t i = 0; i < table->size; ++i) { const WasmFunction* function = &module->functions[table->values[i]]; - values->set(i, Smi::FromInt(function->sig_index)); + int32_t index = table->map.Find(function->sig); + DCHECK_GE(index, 0); + values->set(i, Smi::FromInt(index)); values->set(i + table->max_size, Smi::FromInt(table->values[i])); } // Set the remaining elements to -1 (instead of "undefined"). These diff --git a/src/wasm/wasm-module.h b/src/wasm/wasm-module.h index 65ddce3575..466b15ba67 100644 --- a/src/wasm/wasm-module.h +++ b/src/wasm/wasm-module.h @@ -11,8 +11,8 @@ #include "src/handles.h" #include "src/parsing/preparse-data.h" +#include "src/wasm/signature-map.h" #include "src/wasm/wasm-opcodes.h" -#include "src/wasm/wasm-result.h" namespace v8 { namespace internal { @@ -23,6 +23,8 @@ class WasmCompilationUnit; } namespace wasm { +class ErrorThrower; + const size_t kMaxModuleSize = 1024 * 1024 * 1024; const size_t kMaxFunctionSize = 128 * 1024; const size_t kMaxStringSize = 256; @@ -134,6 +136,7 @@ struct WasmIndirectFunctionTable { std::vector values; // function table, -1 indicating invalid. bool imported; // true if imported. bool exported; // true if exported. + SignatureMap map; // canonicalizing map for sig indexes. }; // Static representation of how to initialize a table. @@ -339,11 +342,6 @@ std::ostream& operator<<(std::ostream& os, const WasmModule& module); std::ostream& operator<<(std::ostream& os, const WasmFunction& function); std::ostream& operator<<(std::ostream& os, const WasmFunctionName& name); -typedef Result ModuleResult; -typedef Result FunctionResult; -typedef std::vector> FunctionOffsets; -typedef Result FunctionOffsetsResult; - class WasmCompiledModule : public FixedArray { public: static WasmCompiledModule* cast(Object* fixed_array) { diff --git a/test/cctest/wasm/test-run-wasm.cc b/test/cctest/wasm/test-run-wasm.cc index d9d9db80e1..d5b5c82cae 100644 --- a/test/cctest/wasm/test-run-wasm.cc +++ b/test/cctest/wasm/test-run-wasm.cc @@ -2632,6 +2632,49 @@ WASM_EXEC_TEST(CallIndirect_NoTable) { CHECK_TRAP(r.Call(2)); } +WASM_EXEC_TEST(CallIndirect_canonical) { + TestSignatures sigs; + TestingModule module(execution_mode); + + WasmFunctionCompiler t1(sigs.i_ii(), &module); + BUILD(t1, WASM_I32_ADD(WASM_GET_LOCAL(0), WASM_GET_LOCAL(1))); + t1.CompileAndAdd(/*sig_index*/ 0); + + WasmFunctionCompiler t2(sigs.i_ii(), &module); + BUILD(t2, WASM_I32_SUB(WASM_GET_LOCAL(0), WASM_GET_LOCAL(1))); + t2.CompileAndAdd(/*sig_index*/ 1); + + WasmFunctionCompiler t3(sigs.f_ff(), &module); + BUILD(t3, WASM_F32_SUB(WASM_GET_LOCAL(0), WASM_GET_LOCAL(1))); + t3.CompileAndAdd(/*sig_index*/ 2); + + // Signature table. + module.AddSignature(sigs.i_ii()); + module.AddSignature(sigs.i_ii()); + module.AddSignature(sigs.f_ff()); + + // Function table. + uint16_t i1 = static_cast(t1.function_index()); + uint16_t i2 = static_cast(t2.function_index()); + uint16_t i3 = static_cast(t3.function_index()); + uint16_t indirect_function_table[] = {i1, i2, i3, i1, i2}; + + module.AddIndirectFunctionTable(indirect_function_table, + arraysize(indirect_function_table)); + module.PopulateIndirectFunctionTable(); + + // Builder the caller function. + WasmRunner r(&module, MachineType::Int32()); + BUILD(r, WASM_CALL_INDIRECT2(1, WASM_GET_LOCAL(0), WASM_I8(77), WASM_I8(11))); + + CHECK_EQ(88, r.Call(0)); + CHECK_EQ(66, r.Call(1)); + CHECK_TRAP(r.Call(2)); + CHECK_EQ(88, r.Call(3)); + CHECK_EQ(66, r.Call(4)); + CHECK_TRAP(r.Call(5)); +} + WASM_EXEC_TEST(F32Floor) { WasmRunner r(execution_mode, MachineType::Float32()); BUILD(r, WASM_F32_FLOOR(WASM_GET_LOCAL(0))); diff --git a/test/cctest/wasm/wasm-run-utils.h b/test/cctest/wasm/wasm-run-utils.h index dc70b909e5..8bf1d3e5c7 100644 --- a/test/cctest/wasm/wasm-run-utils.h +++ b/test/cctest/wasm/wasm-run-utils.h @@ -227,10 +227,13 @@ class TestingModule : public ModuleEnv { } void AddIndirectFunctionTable(uint16_t* functions, uint32_t table_size) { - module_.function_tables.push_back( - {table_size, table_size, std::vector(), false, false}); + module_.function_tables.push_back({table_size, table_size, + std::vector(), false, false, + SignatureMap()}); + WasmIndirectFunctionTable& table = module_.function_tables.back(); for (uint32_t i = 0; i < table_size; ++i) { - module_.function_tables.back().values.push_back(functions[i]); + table.values.push_back(functions[i]); + table.map.FindOrInsert(module_.functions[functions[i]].sig); } Handle values = BuildFunctionTable( diff --git a/test/mjsunit/wasm/indirect-calls.js b/test/mjsunit/wasm/indirect-calls.js index 26021bb74d..7d7f135ae4 100644 --- a/test/mjsunit/wasm/indirect-calls.js +++ b/test/mjsunit/wasm/indirect-calls.js @@ -54,19 +54,19 @@ module = (function () { var sig_i_ii = builder.addType(kSig_i_ii); var sig_i_i = builder.addType(kSig_i_i); - builder.addImport("mul", sig_i_ii); - builder.addFunction("add", sig_i_ii) + var mul = builder.addImport("mul", sig_i_ii); + var add = builder.addFunction("add", sig_i_ii) .addBody([ kExprGetLocal, 0, // -- kExprGetLocal, 1, // -- kExprI32Add // -- ]); - builder.addFunction("popcnt", sig_i_i) + var popcnt = builder.addFunction("popcnt", sig_i_i) .addBody([ kExprGetLocal, 0, // -- kExprI32Popcnt // -- ]); - builder.addFunction("main", kSig_i_iii) + var main = builder.addFunction("main", kSig_i_iii) .addBody([ kExprGetLocal, 1, kExprGetLocal, 2, @@ -74,7 +74,7 @@ module = (function () { kExprCallIndirect, sig_i_ii ]) .exportFunc() - builder.appendToTable([0, 1, 2, 3]); + builder.appendToTable([mul.index, add.index, popcnt.index, main.index]); return builder.instantiate({mul: function(a, b) { return a * b | 0; }}); })(); @@ -84,3 +84,46 @@ assertEquals(99, module.exports.main(1, 22, 77)); assertTraps(kTrapFuncSigMismatch, "module.exports.main(2, 12, 33)"); assertTraps(kTrapFuncSigMismatch, "module.exports.main(3, 12, 33)"); assertTraps(kTrapFuncInvalid, "module.exports.main(4, 12, 33)"); + + +module = (function () { + var builder = new WasmModuleBuilder(); + + var mul = builder.addFunction("mul", kSig_i_ii) + .addBody([ + kExprGetLocal, 0, // -- + kExprGetLocal, 1, // -- + kExprI32Mul // -- + ]); + var add = builder.addFunction("add", kSig_i_ii) + .addBody([ + kExprGetLocal, 0, // -- + kExprGetLocal, 1, // -- + kExprI32Add // -- + ]); + var sub = builder.addFunction("sub", kSig_i_ii) + .addBody([ + kExprGetLocal, 0, // -- + kExprGetLocal, 1, // -- + kExprI32Sub // -- + ]); + builder.addFunction("main", kSig_i_ii) + .addBody([ + kExprI32Const, 33, // -- + kExprGetLocal, 0, // -- + kExprGetLocal, 1, // -- + kExprCallIndirect, 0]) // -- + .exportAs("main"); + + builder.appendToTable([mul.index, add.index, sub.index]); + + return builder.instantiate(); +})(); + +assertEquals(33, module.exports.main(1, 0)); +assertEquals(66, module.exports.main(2, 0)); +assertEquals(34, module.exports.main(1, 1)); +assertEquals(35, module.exports.main(2, 1)); +assertEquals(32, module.exports.main(1, 2)); +assertEquals(31, module.exports.main(2, 2)); +assertTraps(kTrapFuncInvalid, "module.exports.main(12, 3)");