From b63ce1be6cb90135fd01c41b802a88cf778208c3 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 07:46:59 +0100 Subject: [PATCH 1/6] 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/src/interpreter/instructions/argument_get.rs b/src/interpreter/instructions/argument_get.rs index dd161e1..3b95405 100644 --- a/src/interpreter/instructions/argument_get.rs +++ b/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 2074ce8bce44789229d561aa693dec16c69fdb47 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 07:53:44 +0100 Subject: [PATCH 2/6] fix(interface-types) `Instruction::CallCore.function_index` is a `u32`. --- src/decoders/binary.rs | 2 +- src/decoders/wat.rs | 2 +- src/interpreter/instruction.rs | 2 +- src/interpreter/instructions/call_core.rs | 10 +++++----- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/decoders/binary.rs b/src/decoders/binary.rs index f1f7f77..d8699a6 100644 --- a/src/decoders/binary.rs +++ b/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/src/decoders/wat.rs b/src/decoders/wat.rs index 3e41d51..4a77857 100644 --- a/src/decoders/wat.rs +++ b/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/src/interpreter/instruction.rs b/src/interpreter/instruction.rs index 712a6ea..37f8162 100644 --- a/src/interpreter/instruction.rs +++ b/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/src/interpreter/instructions/call_core.rs b/src/interpreter/instructions/call_core.rs index 5a3da21..7a8e498 100644 --- a/src/interpreter/instructions/call_core.rs +++ b/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 bf7f7cf927d4163faa037ea4c96be84e507217c2 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 08:27:51 +0100 Subject: [PATCH 3/6] 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/src/interpreter/instructions/strings.rs b/src/interpreter/instructions/strings.rs index 011cac9..d84da47 100644 --- a/src/interpreter/instructions/strings.rs +++ b/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 c35996c2334c39153d65915c0305257214ca9d59 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 08:30:32 +0100 Subject: [PATCH 4/6] 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/src/interpreter/instructions/strings.rs b/src/interpreter/instructions/strings.rs index d84da47..8a40ed2 100644 --- a/src/interpreter/instructions/strings.rs +++ b/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 5275f3f3063c12e685ffa2f49ba1a3df5d43c69f Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 08:31:06 +0100 Subject: [PATCH 5/6] feat(interface-types) Add the `NegativeValue` instruction error. --- src/errors.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/errors.rs b/src/errors.rs index 5df5c49..db6d7bd 100644 --- a/src/errors.rs +++ b/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 8705de9cc0956d42797cf1bcaa6de421bac5b3a8 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Mar 2020 10:49:49 +0100 Subject: [PATCH 6/6] feat(interface-types) Simplify code by implementing `From`. --- src/errors.rs | 11 +++++++++-- src/interpreter/instructions/strings.rs | 24 ++++++++++-------------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index db6d7bd..bf9612a 100644 --- a/src/errors.rs +++ b/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/src/interpreter/instructions/strings.rs b/src/interpreter/instructions/strings.rs index 8a40ed2..66afa8d 100644 --- a/src/interpreter/instructions/strings.rs +++ b/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!(