diff --git a/src/wasm/decoder.h b/src/wasm/decoder.h index f6c88d0416..3c0c0493b0 100644 --- a/src/wasm/decoder.h +++ b/src/wasm/decoder.h @@ -266,6 +266,7 @@ class Decoder { return offset - buffer_offset_; } const byte* end() const { return end_; } + void set_end(const byte* end) { end_ = end; } // Check if the byte at {offset} from the current pc equals {expected}. bool lookahead(int offset, byte expected) { diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc index a849ba1f0d..79e37a1148 100644 --- a/src/wasm/module-compiler.cc +++ b/src/wasm/module-compiler.cc @@ -2191,16 +2191,19 @@ bool AsyncStreamingProcessor::ProcessSection(SectionCode section_code, NativeModuleCache::WireBytesHash(bytes)); } if (section_code == SectionCode::kUnknownSectionCode) { - Decoder decoder(bytes, offset); - section_code = ModuleDecoder::IdentifyUnknownSection( - &decoder, bytes.begin() + bytes.length()); + size_t bytes_consumed = ModuleDecoder::IdentifyUnknownSection( + &decoder_, bytes, offset, §ion_code); + if (!decoder_.ok()) { + FinishAsyncCompileJobWithError(decoder_.FinishDecoding(false).error()); + return false; + } if (section_code == SectionCode::kUnknownSectionCode) { // Skip unknown sections that we do not know how to handle. return true; } // Remove the unknown section tag from the payload bytes. - offset += decoder.position(); - bytes = bytes.SubVector(decoder.position(), bytes.size()); + offset += bytes_consumed; + bytes = bytes.SubVector(bytes_consumed, bytes.size()); } constexpr bool verify_functions = false; decoder_.DecodeSection(section_code, bytes, offset, verify_functions); diff --git a/src/wasm/module-decoder.cc b/src/wasm/module-decoder.cc index 82fafa205b..709f808230 100644 --- a/src/wasm/module-decoder.cc +++ b/src/wasm/module-decoder.cc @@ -149,6 +149,42 @@ WireBytesRef consume_string(Decoder* decoder, bool validate_utf8, return {offset, decoder->failed() ? 0 : length}; } +namespace { +SectionCode IdentifyUnknownSectionInternal(Decoder* decoder) { + WireBytesRef string = consume_string(decoder, true, "section name"); + if (decoder->failed()) { + return kUnknownSectionCode; + } + const byte* section_name_start = + decoder->start() + decoder->GetBufferRelativeOffset(string.offset()); + + TRACE(" +%d section name : \"%.*s\"\n", + static_cast(section_name_start - decoder->start()), + string.length() < 20 ? string.length() : 20, section_name_start); + + if (string.length() == num_chars(kNameString) && + strncmp(reinterpret_cast(section_name_start), kNameString, + num_chars(kNameString)) == 0) { + return kNameSectionCode; + } else if (string.length() == num_chars(kSourceMappingURLString) && + strncmp(reinterpret_cast(section_name_start), + kSourceMappingURLString, + num_chars(kSourceMappingURLString)) == 0) { + return kSourceMappingURLSectionCode; + } else if (string.length() == num_chars(kCompilationHintsString) && + strncmp(reinterpret_cast(section_name_start), + kCompilationHintsString, + num_chars(kCompilationHintsString)) == 0) { + return kCompilationHintsSectionCode; + } else if (string.length() == num_chars(kDebugInfoString) && + strncmp(reinterpret_cast(section_name_start), + kDebugInfoString, num_chars(kDebugInfoString)) == 0) { + return kDebugInfoSectionCode; + } + return kUnknownSectionCode; +} +} // namespace + // An iterator over the sections in a wasm binary module. // Automatically skips all unknown sections. class WasmSectionIterator { @@ -232,8 +268,13 @@ class WasmSectionIterator { if (section_code == kUnknownSectionCode) { // Check for the known "name", "sourceMappingURL", or "compilationHints" // section. - section_code = - ModuleDecoder::IdentifyUnknownSection(decoder_, section_end_); + // To identify the unknown section we set the end of the decoder bytes to + // the end of the custom section, so that we do not read the section name + // beyond the end of the section. + const byte* module_end = decoder_->end(); + decoder_->set_end(section_end_); + section_code = IdentifyUnknownSectionInternal(decoder_); + if (decoder_->ok()) decoder_->set_end(module_end); // As a side effect, the above function will forward the decoder to after // the identifier string. payload_start_ = decoder_->pc(); @@ -2030,39 +2071,14 @@ void ModuleDecoder::set_code_section(uint32_t offset, uint32_t size) { return impl_->set_code_section(offset, size); } -SectionCode ModuleDecoder::IdentifyUnknownSection(Decoder* decoder, - const byte* end) { - WireBytesRef string = consume_string(decoder, true, "section name"); - if (decoder->failed() || decoder->pc() > end) { - return kUnknownSectionCode; - } - const byte* section_name_start = - decoder->start() + decoder->GetBufferRelativeOffset(string.offset()); - - TRACE(" +%d section name : \"%.*s\"\n", - static_cast(section_name_start - decoder->start()), - string.length() < 20 ? string.length() : 20, section_name_start); - - if (string.length() == num_chars(kNameString) && - strncmp(reinterpret_cast(section_name_start), kNameString, - num_chars(kNameString)) == 0) { - return kNameSectionCode; - } else if (string.length() == num_chars(kSourceMappingURLString) && - strncmp(reinterpret_cast(section_name_start), - kSourceMappingURLString, - num_chars(kSourceMappingURLString)) == 0) { - return kSourceMappingURLSectionCode; - } else if (string.length() == num_chars(kCompilationHintsString) && - strncmp(reinterpret_cast(section_name_start), - kCompilationHintsString, - num_chars(kCompilationHintsString)) == 0) { - return kCompilationHintsSectionCode; - } else if (string.length() == num_chars(kDebugInfoString) && - strncmp(reinterpret_cast(section_name_start), - kDebugInfoString, num_chars(kDebugInfoString)) == 0) { - return kDebugInfoSectionCode; - } - return kUnknownSectionCode; +size_t ModuleDecoder::IdentifyUnknownSection(ModuleDecoder* decoder, + Vector bytes, + uint32_t offset, + SectionCode* result) { + if (!decoder->ok()) return 0; + decoder->impl_->Reset(bytes, offset); + *result = IdentifyUnknownSectionInternal(decoder->impl_.get()); + return decoder->impl_->pc() - bytes.begin(); } bool ModuleDecoder::ok() { return impl_->ok(); } diff --git a/src/wasm/module-decoder.h b/src/wasm/module-decoder.h index 2ad2b4646c..49652db1fd 100644 --- a/src/wasm/module-decoder.h +++ b/src/wasm/module-decoder.h @@ -205,10 +205,10 @@ class ModuleDecoder { // SectionCode if the unknown section is known to decoder. // The decoder is expected to point after the section length and just before // the identifier string of the unknown section. - // If a SectionCode other than kUnknownSectionCode is returned, the decoder - // will point right after the identifier string. Otherwise, the position is - // undefined. - static SectionCode IdentifyUnknownSection(Decoder* decoder, const byte* end); + // The return value is the number of bytes that were consumed. + static size_t IdentifyUnknownSection(ModuleDecoder* decoder, + Vector bytes, + uint32_t offset, SectionCode* result); private: const WasmFeatures enabled_features_; diff --git a/src/wasm/wasm-engine.cc b/src/wasm/wasm-engine.cc index 29f0856d7a..7a20363bb1 100644 --- a/src/wasm/wasm-engine.cc +++ b/src/wasm/wasm-engine.cc @@ -254,10 +254,6 @@ size_t NativeModuleCache::PrefixHash(Vector wire_bytes) { break; } const uint8_t* payload_start = decoder.pc(); - // TODO(v8:10126): Remove this check, bytes have been validated already. - if (decoder.position() + section_size > wire_bytes.size()) { - return hash; - } decoder.consume_bytes(section_size, "section payload"); size_t section_hash = NativeModuleCache::WireBytesHash( Vector(payload_start, section_size)); diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index 894326d8a6..df5fea0d66 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -160,9 +160,6 @@ # OOM with too many isolates/memory objects (https://crbug.com/1010272) # Predictable tests fail due to race between postMessage and GrowMemory 'regress/wasm/regress-1010272': [PASS, NO_VARIANTS, ['system == android', SKIP], ['predictable', SKIP]], - - # https://crbug.com/v8/10126 - 'regress/wasm/regress-789952': [SKIP] }], # ALWAYS ############################################################################## diff --git a/test/mjsunit/regress/wasm/regress-10126-streaming.js b/test/mjsunit/regress/wasm/regress-10126-streaming.js new file mode 100644 index 0000000000..bb6df82f55 --- /dev/null +++ b/test/mjsunit/regress/wasm/regress-10126-streaming.js @@ -0,0 +1,7 @@ +// Copyright 2020 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. + +// Flags: --wasm-test-streaming + +load('test/mjsunit/regress/wasm/regress-10126.js') diff --git a/test/mjsunit/regress/wasm/regress-10126.js b/test/mjsunit/regress/wasm/regress-10126.js new file mode 100644 index 0000000000..4985dfedb7 --- /dev/null +++ b/test/mjsunit/regress/wasm/regress-10126.js @@ -0,0 +1,32 @@ +// Copyright 2020 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. + +load('test/mjsunit/wasm/wasm-module-builder.js') + +let binary = new Binary; +binary.emit_bytes([ + kWasmH0, // 0 header + kWasmH1, // 1 - + kWasmH2, // 2 - + kWasmH3, // 3 - + kWasmV0, // 4 version + kWasmV1, // 5 - + kWasmV2, // 6 - + kWasmV3, // 7 - + kUnknownSectionCode, // 8 custom section + 0x5, // 9 length + 0x6, // 10 invalid name length + 'a', // 11 payload + 'b', // 12 - + 'c', // 13 - + 'd', // 14 - + kCodeSectionCode, // 15 code section start + 0x1, // 16 code section length + 19, // 17 invalid number of functions +]); + +const buffer = binary.trunc_buffer(); +assertThrowsAsync( + WebAssembly.compile(buffer), WebAssembly.CompileError, + 'WebAssembly.compile(): expected 6 bytes, fell off end @+11'); diff --git a/test/mjsunit/regress/wasm/regress-789952.js b/test/mjsunit/regress/wasm/regress-789952.js deleted file mode 100644 index f73d8dc471..0000000000 --- a/test/mjsunit/regress/wasm/regress-789952.js +++ /dev/null @@ -1,40 +0,0 @@ -// Copyright 2017 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. - -let module_size = 19; -let string_len = 0x00fffff0 - module_size; - -print("Allocating backing store: " + (string_len + module_size)); -let backing = new ArrayBuffer(string_len + module_size); - -print("Allocating typed array buffer"); -let buffer = new Uint8Array(backing); - -print("Filling..."); -buffer.fill(0x41); - -print("Setting up array buffer"); -// Magic -buffer.set([0x00, 0x61, 0x73, 0x6D], 0); -// Version -buffer.set([0x01, 0x00, 0x00, 0x00], 4); -// kUnknownSection (0) -buffer.set([0], 8); -// Section length -buffer.set([0x80, 0x80, 0x80, 0x80, 0x00], 9); -// Name length -let x = string_len + 1; -let b1 = ((x >> 0) & 0x7F) | 0x80; -let b2 = ((x >> 7) & 0x7F) | 0x80; -let b3 = ((x >> 14) & 0x7F) | 0x80; -let b4 = ((x >> 21) & 0x7F); -//buffer.set([0xDE, 0xFF, 0xFF, 0x7F], 14); - buffer.set([b1, b2, b3, b4], 14); - -print("Parsing module..."); -let m = new WebAssembly.Module(buffer); - -print("Triggering!"); -let c = WebAssembly.Module.customSections(m, "A".repeat(string_len + 1)); -assertEquals(0, c.length); diff --git a/test/unittests/wasm/module-decoder-unittest.cc b/test/unittests/wasm/module-decoder-unittest.cc index 9cd8d9b347..e1eb59138b 100644 --- a/test/unittests/wasm/module-decoder-unittest.cc +++ b/test/unittests/wasm/module-decoder-unittest.cc @@ -1753,12 +1753,22 @@ TEST_F(WasmModuleVerifyTest, SectionWithoutNameLength) { EXPECT_FAILURE(data); } +TEST_F(WasmModuleVerifyTest, EmptyCustomSectionIsInvalid) { + // An empty custom section is invalid, because at least one byte for the + // length of the custom section name is required. + const byte data[] = { + 0, // unknown section code. + 0 // section length. + }; + EXPECT_FAILURE(data); +} + TEST_F(WasmModuleVerifyTest, TheLoneliestOfValidModulesTheTrulyEmptyOne) { const byte data[] = { 0, // unknown section code. - 0, // Empty section name. - // No section name, no content, nothing but sadness. - 0, // No section content. + 1, // section length, only one byte for the name length. + 0, // string length of 0. + // Empty section name, no content, nothing but sadness. }; EXPECT_VERIFIES(data); }