From 9826ff38136dcbb3a881e9c2986e82ac24cc1b5c Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 07:30:34 +0100 Subject: [PATCH 01/10] fix(runtime-c-api) Change mutability of `memory` for `const` in `wasmer_memory_data_length`. --- lib/runtime-c-api/src/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/runtime-c-api/src/memory.rs b/lib/runtime-c-api/src/memory.rs index 6d3268768..40f5718bc 100644 --- a/lib/runtime-c-api/src/memory.rs +++ b/lib/runtime-c-api/src/memory.rs @@ -198,7 +198,7 @@ pub extern "C" fn wasmer_memory_data(memory: *const wasmer_memory_t) -> *mut u8 /// ``` #[allow(clippy::cast_ptr_alignment)] #[no_mangle] -pub extern "C" fn wasmer_memory_data_length(memory: *mut wasmer_memory_t) -> u32 { +pub extern "C" fn wasmer_memory_data_length(memory: *const wasmer_memory_t) -> u32 { if memory.is_null() { return 0; } From 6eaf87d651119d66b70f3b3494ab20f0cf88e2b2 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 07:32:57 +0100 Subject: [PATCH 02/10] chore(runtime-c-api) Update C/C++ headers. --- lib/runtime-c-api/wasmer.h | 2 +- lib/runtime-c-api/wasmer.hh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/runtime-c-api/wasmer.h b/lib/runtime-c-api/wasmer.h index 7a872f65c..1e7d9296d 100644 --- a/lib/runtime-c-api/wasmer.h +++ b/lib/runtime-c-api/wasmer.h @@ -1171,7 +1171,7 @@ uint8_t *wasmer_memory_data(const wasmer_memory_t *memory); * uint32_t memory_data_length = wasmer_memory_data_length(memory); * ``` */ -uint32_t wasmer_memory_data_length(wasmer_memory_t *memory); +uint32_t wasmer_memory_data_length(const wasmer_memory_t *memory); /** * Frees memory for the given `wasmer_memory_t`. diff --git a/lib/runtime-c-api/wasmer.hh b/lib/runtime-c-api/wasmer.hh index 6bd66d40b..0bbf819cc 100644 --- a/lib/runtime-c-api/wasmer.hh +++ b/lib/runtime-c-api/wasmer.hh @@ -964,7 +964,7 @@ uint8_t *wasmer_memory_data(const wasmer_memory_t *memory); /// ```c /// uint32_t memory_data_length = wasmer_memory_data_length(memory); /// ``` -uint32_t wasmer_memory_data_length(wasmer_memory_t *memory); +uint32_t wasmer_memory_data_length(const wasmer_memory_t *memory); /// Frees memory for the given `wasmer_memory_t`. /// From f71872c365a85a1540a2b4fab172768817565d8a Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 07:35:54 +0100 Subject: [PATCH 03/10] doc(changelog) Add #1335. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55f36466f..72ddfeb9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## **[Unreleased]** +- [#1335](https://github.com/wasmerio/wasmer/pull/1335) Change mutability of `memory` for `const` in `wasmer_memory_data_length` in the C API - [#1320](https://github.com/wasmerio/wasmer/pull/1320) Change `custom_sections` field in `ModuleInfo` to be more standards compliant by allowing multiple custom sections with the same name. To get the old behavior with the new API, you can add `.last().unwrap()` to accesses. For example, `module_info.custom_sections["custom_section_name"].last().unwrap()`. - [#1303](https://github.com/wasmerio/wasmer/pull/1303) NaN canonicalization for singlepass backend. - [#1292](https://github.com/wasmerio/wasmer/pull/1292) Experimental Support for Android (x86_64 and AArch64) From b0879d1d12d5e8fefeaed9b0fb0d9fb3407d6047 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 07:46:59 +0100 Subject: [PATCH 04/10] fix(interface-types) Cast index to `usize` to compare index to length. The index is bound to `u32::max_value()`. The invocation inputs' length is bound to `usize::max_value()`, which can be `u64::max_value`. Consequently, casting the invocation inputs' length to `u32` can lead to an integer overflow. It is better to cast `index` to `usize` when comparing with the invocation inputs' length. --- .../src/interpreter/instructions/argument_get.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/interface-types/src/interpreter/instructions/argument_get.rs b/lib/interface-types/src/interpreter/instructions/argument_get.rs index dd161e158..3b95405be 100644 --- a/lib/interface-types/src/interpreter/instructions/argument_get.rs +++ b/lib/interface-types/src/interpreter/instructions/argument_get.rs @@ -8,7 +8,7 @@ executable_instruction!( move |runtime| -> _ { let invocation_inputs = runtime.invocation_inputs; - if index >= (invocation_inputs.len() as u32) { + if (index as usize) >= invocation_inputs.len() { return Err(InstructionError::new( instruction, InstructionErrorKind::InvocationInputIsMissing { index }, From 0e70e538cc2dc482f6a3f86db1c60f870b8e648a Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 07:53:44 +0100 Subject: [PATCH 05/10] fix(interface-types) `Instruction::CallCore.function_index` is a `u32`. --- lib/interface-types/src/decoders/binary.rs | 2 +- lib/interface-types/src/decoders/wat.rs | 2 +- lib/interface-types/src/interpreter/instruction.rs | 2 +- .../src/interpreter/instructions/call_core.rs | 10 +++++----- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/interface-types/src/decoders/binary.rs b/lib/interface-types/src/decoders/binary.rs index f1f7f7712..d8699a61f 100644 --- a/lib/interface-types/src/decoders/binary.rs +++ b/lib/interface-types/src/decoders/binary.rs @@ -169,7 +169,7 @@ fn instruction<'input, E: ParseError<&'input [u8]>>( ( input, Instruction::CallCore { - function_index: argument_0 as usize, + function_index: argument_0 as u32, }, ) } diff --git a/lib/interface-types/src/decoders/wat.rs b/lib/interface-types/src/decoders/wat.rs index 3e41d513b..4a77857f7 100644 --- a/lib/interface-types/src/decoders/wat.rs +++ b/lib/interface-types/src/decoders/wat.rs @@ -146,7 +146,7 @@ impl<'a> Parse<'a> for Instruction { parser.parse::()?; Ok(Instruction::CallCore { - function_index: parser.parse::()? as usize, + function_index: parser.parse::()?, }) } else if lookahead.peek::() { parser.parse::()?; diff --git a/lib/interface-types/src/interpreter/instruction.rs b/lib/interface-types/src/interpreter/instruction.rs index 712a6ea58..37f81626b 100644 --- a/lib/interface-types/src/interpreter/instruction.rs +++ b/lib/interface-types/src/interpreter/instruction.rs @@ -12,7 +12,7 @@ pub enum Instruction { /// The `call-core` instruction. CallCore { /// The function index. - function_index: usize, + function_index: u32, }, /// The `s8.from_i32` instruction. diff --git a/lib/interface-types/src/interpreter/instructions/call_core.rs b/lib/interface-types/src/interpreter/instructions/call_core.rs index 5a3da2124..7a8e4985d 100644 --- a/lib/interface-types/src/interpreter/instructions/call_core.rs +++ b/lib/interface-types/src/interpreter/instructions/call_core.rs @@ -8,16 +8,16 @@ use crate::{ }; executable_instruction!( - call_core(function_index: usize, instruction: Instruction) -> _ { + call_core(function_index: u32, instruction: Instruction) -> _ { move |runtime| -> _ { let instance = &mut runtime.wasm_instance; - let index = FunctionIndex::new(function_index); + let index = FunctionIndex::new(function_index as usize); let local_or_import = instance.local_or_import(index).ok_or_else(|| { InstructionError::new( instruction, InstructionErrorKind::LocalOrImportIsMissing { - function_index: function_index as u32, + function_index: function_index, }, ) })?; @@ -40,7 +40,7 @@ executable_instruction!( return Err(InstructionError::new( instruction, InstructionErrorKind::LocalOrImportSignatureMismatch { - function_index: function_index as u32, + function_index: function_index, expected: (local_or_import.inputs().to_vec(), vec![]), received: (input_types, vec![]), }, @@ -51,7 +51,7 @@ executable_instruction!( InstructionError::new( instruction, InstructionErrorKind::LocalOrImportCall { - function_index: function_index as u32, + function_index: function_index, }, ) })?; From 86b545fd497a518f8c16ca539b0d6e563d303062 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 08:27:51 +0100 Subject: [PATCH 06/10] fix(interface-types) Avoid integer overflows in string instructions. --- .../src/interpreter/instructions/strings.rs | 33 +++++++++++++++---- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/lib/interface-types/src/interpreter/instructions/strings.rs b/lib/interface-types/src/interpreter/instructions/strings.rs index 011cac951..d84da4760 100644 --- a/lib/interface-types/src/interpreter/instructions/strings.rs +++ b/lib/interface-types/src/interpreter/instructions/strings.rs @@ -10,7 +10,7 @@ use crate::{ Instruction, }, }; -use std::cell::Cell; +use std::{cell::Cell, convert::TryInto}; executable_instruction!( string_lift_memory(instruction: Instruction) -> _ { @@ -23,7 +23,6 @@ executable_instruction!( })?; let memory_index: u32 = 0; - let memory = runtime .wasm_instance .memory(memory_index as usize) @@ -34,8 +33,18 @@ executable_instruction!( ) })?; - let pointer = to_native::(&inputs[0], instruction)? as usize; - let length = to_native::(&inputs[1], instruction)? as usize; + let pointer: usize = to_native::(&inputs[0], instruction)?.try_into().map_err(|_| { + InstructionError::new( + instruction, + InstructionErrorKind::NegativeValue { subject: "pointer" }, + ) + })?; + let length: usize = to_native::(&inputs[1], instruction)?.try_into().map_err(|_| { + InstructionError::new( + instruction, + InstructionErrorKind::NegativeValue { subject: "length" }, + ) + })?; let memory_view = memory.view(); if length == 0 { @@ -102,7 +111,12 @@ executable_instruction!( let string: String = to_native(&string, instruction)?; let string_bytes = string.as_bytes(); - let string_length = string_bytes.len() as i32; + let string_length: i32 = string_bytes.len().try_into().map_err(|_| { + InstructionError::new( + instruction, + InstructionErrorKind::NegativeValue { subject: "string_length" }, + ) + })?; let outputs = allocator.call(&[InterfaceValue::I32(string_length)]).map_err(|_| { InstructionError::new( @@ -110,7 +124,12 @@ executable_instruction!( InstructionErrorKind::LocalOrImportCall { function_index: allocator_index }, ) })?; - let string_pointer: i32 = to_native(&outputs[0], instruction)?; + let string_pointer: u32 = to_native::(&outputs[0], instruction)?.try_into().map_err(|_| { + InstructionError::new( + instruction, + InstructionErrorKind::NegativeValue { subject: "string_pointer" }, + ) + })?; let memory_index: u32 = 0; let memory_view = instance @@ -127,7 +146,7 @@ executable_instruction!( memory_view[string_pointer as usize + nth].set(*byte); } - runtime.stack.push(InterfaceValue::I32(string_pointer)); + runtime.stack.push(InterfaceValue::I32(string_pointer as i32)); runtime.stack.push(InterfaceValue::I32(string_length)); Ok(()) From f3be7981d211ff2f8cca3d973e37f23acf393638 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 08:30:32 +0100 Subject: [PATCH 07/10] test(interface-types) Test negative pointer or length in `string.lift_memory`. --- .../src/interpreter/instructions/strings.rs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/lib/interface-types/src/interpreter/instructions/strings.rs b/lib/interface-types/src/interpreter/instructions/strings.rs index d84da4760..8a40ed2a6 100644 --- a/lib/interface-types/src/interpreter/instructions/strings.rs +++ b/lib/interface-types/src/interpreter/instructions/strings.rs @@ -222,6 +222,42 @@ mod tests { stack: [InterfaceValue::String("".into())], ); + test_executable_instruction!( + test_string_lift_memory__negative_pointer = + instructions: [ + Instruction::ArgumentGet { index: 0 }, + Instruction::ArgumentGet { index: 1 }, + Instruction::StringLiftMemory, + ], + invocation_inputs: [ + InterfaceValue::I32(-42), + InterfaceValue::I32(13), + ], + instance: Instance { + memory: Memory::new("Hello!".as_bytes().iter().map(|u| Cell::new(*u)).collect()), + ..Default::default() + }, + error: r#"`string.lift_memory` read the value of `pointer` which must be positive"#, + ); + + test_executable_instruction!( + test_string_lift_memory__negative_length = + instructions: [ + Instruction::ArgumentGet { index: 0 }, + Instruction::ArgumentGet { index: 1 }, + Instruction::StringLiftMemory, + ], + invocation_inputs: [ + InterfaceValue::I32(0), + InterfaceValue::I32(-1), + ], + instance: Instance { + memory: Memory::new("Hello!".as_bytes().iter().map(|u| Cell::new(*u)).collect()), + ..Default::default() + }, + error: r#"`string.lift_memory` read the value of `length` which must be positive"#, + ); + test_executable_instruction!( test_string_lift_memory__read_out_of_memory = instructions: [ From 25cd6cd24a7dc69f2bd27259361e17a147ab5cf8 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 08:31:06 +0100 Subject: [PATCH 08/10] feat(interface-types) Add the `NegativeValue` instruction error. --- lib/interface-types/src/errors.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/interface-types/src/errors.rs b/lib/interface-types/src/errors.rs index 5df5c49e3..db6d7bd60 100644 --- a/lib/interface-types/src/errors.rs +++ b/lib/interface-types/src/errors.rs @@ -149,6 +149,12 @@ pub enum InstructionErrorKind { /// The string contains invalid UTF-8 encoding. String(string::FromUtf8Error), + + /// A negative value isn't allowed (like a negative pointer value). + NegativeValue { + /// The variable name that triggered the error. + subject: &'static str, + }, } impl Error for InstructionErrorKind {} @@ -227,6 +233,12 @@ impl Display for InstructionErrorKind { "{}", error ), + + Self::NegativeValue { subject } => write!( + formatter, + "read the value of `{}` which must be positive", + subject + ), } } } From 6e5d9624f1f6b685b1864f18ca2d956b4a3d847c Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 10:49:49 +0100 Subject: [PATCH 09/10] feat(interface-types) Simplify code by implementing `From`. --- lib/interface-types/src/errors.rs | 11 +++++++-- .../src/interpreter/instructions/strings.rs | 24 ++++++++----------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/lib/interface-types/src/errors.rs b/lib/interface-types/src/errors.rs index db6d7bd60..bf9612a8a 100644 --- a/lib/interface-types/src/errors.rs +++ b/lib/interface-types/src/errors.rs @@ -5,6 +5,7 @@ use crate::{ast::InterfaceType, interpreter::Instruction}; use std::{ error::Error, fmt::{self, Display, Formatter}, + num::TryFromIntError, result::Result, string::{self, ToString}, }; @@ -150,7 +151,7 @@ pub enum InstructionErrorKind { /// The string contains invalid UTF-8 encoding. String(string::FromUtf8Error), - /// A negative value isn't allowed (like a negative pointer value). + /// Out of range integral type conversion attempted. NegativeValue { /// The variable name that triggered the error. subject: &'static str, @@ -236,9 +237,15 @@ impl Display for InstructionErrorKind { Self::NegativeValue { subject } => write!( formatter, - "read the value of `{}` which must be positive", + "attempted to convert `{}` but it appears to be a negative value", subject ), } } } + +impl From<(TryFromIntError, &'static str)> for InstructionErrorKind { + fn from((_, subject): (TryFromIntError, &'static str)) -> Self { + InstructionErrorKind::NegativeValue { subject } + } +} diff --git a/lib/interface-types/src/interpreter/instructions/strings.rs b/lib/interface-types/src/interpreter/instructions/strings.rs index 8a40ed2a6..66afa8d72 100644 --- a/lib/interface-types/src/interpreter/instructions/strings.rs +++ b/lib/interface-types/src/interpreter/instructions/strings.rs @@ -33,18 +33,14 @@ executable_instruction!( ) })?; - let pointer: usize = to_native::(&inputs[0], instruction)?.try_into().map_err(|_| { - InstructionError::new( - instruction, - InstructionErrorKind::NegativeValue { subject: "pointer" }, - ) - })?; - let length: usize = to_native::(&inputs[1], instruction)?.try_into().map_err(|_| { - InstructionError::new( - instruction, - InstructionErrorKind::NegativeValue { subject: "length" }, - ) - })?; + let pointer: usize = to_native::(&inputs[0], instruction)? + .try_into() + .map_err(|e| (e, "pointer").into()) + .map_err(|k| InstructionError::new(instruction, k))?; + let length: usize = to_native::(&inputs[1], instruction)? + .try_into() + .map_err(|e| (e, "length").into()) + .map_err(|k| InstructionError::new(instruction, k))?; let memory_view = memory.view(); if length == 0 { @@ -237,7 +233,7 @@ mod tests { memory: Memory::new("Hello!".as_bytes().iter().map(|u| Cell::new(*u)).collect()), ..Default::default() }, - error: r#"`string.lift_memory` read the value of `pointer` which must be positive"#, + error: r#"`string.lift_memory` attempted to convert `pointer` but it appears to be a negative value"#, ); test_executable_instruction!( @@ -255,7 +251,7 @@ mod tests { memory: Memory::new("Hello!".as_bytes().iter().map(|u| Cell::new(*u)).collect()), ..Default::default() }, - error: r#"`string.lift_memory` read the value of `length` which must be positive"#, + error: r#"`string.lift_memory` attempted to convert `length` but it appears to be a negative value"#, ); test_executable_instruction!( From baeeea15351287a6bc1c9bf3a498e181ca70a084 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 27 Mar 2020 07:47:03 +0100 Subject: [PATCH 10/10] doc(changelog) Fix typo. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72ddfeb9d..5a18164d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## **[Unreleased]** -- [#1335](https://github.com/wasmerio/wasmer/pull/1335) Change mutability of `memory` for `const` in `wasmer_memory_data_length` in the C API +- [#1335](https://github.com/wasmerio/wasmer/pull/1335) Change mutability of `memory` to `const` in `wasmer_memory_data_length` in the C API - [#1320](https://github.com/wasmerio/wasmer/pull/1320) Change `custom_sections` field in `ModuleInfo` to be more standards compliant by allowing multiple custom sections with the same name. To get the old behavior with the new API, you can add `.last().unwrap()` to accesses. For example, `module_info.custom_sections["custom_section_name"].last().unwrap()`. - [#1303](https://github.com/wasmerio/wasmer/pull/1303) NaN canonicalization for singlepass backend. - [#1292](https://github.com/wasmerio/wasmer/pull/1292) Experimental Support for Android (x86_64 and AArch64)