From 1885af398fde704af479cbb6ae5583da82852efa Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 31 Aug 2020 17:13:06 +0200 Subject: [PATCH] fix: `UnwindRegistry` owns the `eh_frame`s. `UnwindRegistry` registers `eh_frame` from a slice. This slice is a pointer owned by `SerializableModule`, which is held by `Artifact`. If the artifact is dropped _before_ the `UnwindRegistry`, then we have a dangling pointer, and it can crash. Actually, it crashes, see https://github.com/wasmerio/wasmer/pull/1581 as our first attempt to fix this problem. So instead of `UnwindRegistry` to register a pointer to a data it doesn't own, this patch updates `UnwindRegistry` to register a pointer to data that it owns. `UnwindRegistry` now receives an `eh_frame` of kind `Vec` instead of `&[u8]`. The bytes are copied. In addition, this patch creates an `UnwindRegistryExt` trait, so that it brings consistency amongst all `UnwindRegistry` implementation (`systemv`, `windows_x64` and `dummy`). This is for an internal usage only. --- lib/engine-jit/src/artifact.rs | 15 ++--- lib/engine-jit/src/code_memory.rs | 2 +- lib/engine-jit/src/unwind/dummy.rs | 5 +- lib/engine-jit/src/unwind/mod.rs | 17 +++++ lib/engine-jit/src/unwind/systemv.rs | 82 +++++++++++++----------- lib/engine-jit/src/unwind/windows_x64.rs | 5 +- 6 files changed, 78 insertions(+), 48 deletions(-) diff --git a/lib/engine-jit/src/artifact.rs b/lib/engine-jit/src/artifact.rs index 767b7f55a96..fe34557a34b 100644 --- a/lib/engine-jit/src/artifact.rs +++ b/lib/engine-jit/src/artifact.rs @@ -6,7 +6,7 @@ use crate::link::link_module; #[cfg(feature = "compiler")] use crate::serialize::SerializableCompilation; use crate::serialize::SerializableModule; -use crate::unwind::UnwindRegistry; +use crate::unwind::{UnwindRegistry, UnwindRegistryExt}; use std::sync::{Arc, Mutex}; use wasmer_compiler::{CompileError, Features, Triple}; #[cfg(feature = "compiler")] @@ -135,10 +135,6 @@ impl JITArtifact { } let inner_bytes = &bytes[Self::MAGIC_HEADER.len()..]; - - // let r = flexbuffers::Reader::get_root(bytes).map_err(|e| DeserializeError::CorruptedBinary(format!("{:?}", e)))?; - // let serializable = SerializableModule::deserialize(r).map_err(|e| DeserializeError::CorruptedBinary(format!("{:?}", e)))?; - let serializable: SerializableModule = bincode::deserialize(inner_bytes) .map_err(|e| DeserializeError::CorruptedBinary(format!("{:?}", e)))?; @@ -193,9 +189,12 @@ impl JITArtifact { .bytes .len(); let eh_frame_section_pointer = custom_sections[debug.eh_frame]; - Some(unsafe { - std::slice::from_raw_parts(eh_frame_section_pointer, eh_frame_section_size) - }) + Some( + unsafe { + std::slice::from_raw_parts(eh_frame_section_pointer, eh_frame_section_size) + } + .to_vec(), + ) } None => None, }; diff --git a/lib/engine-jit/src/code_memory.rs b/lib/engine-jit/src/code_memory.rs index ac92aeb626d..ab0deabbe2c 100644 --- a/lib/engine-jit/src/code_memory.rs +++ b/lib/engine-jit/src/code_memory.rs @@ -2,7 +2,7 @@ // Attributions: https://github.com/wasmerio/wasmer/blob/master/ATTRIBUTIONS.md //! Memory management for executable code. -use crate::unwind::UnwindRegistry; +use crate::unwind::{UnwindRegistry, UnwindRegistryExt}; use std::mem::ManuallyDrop; use std::sync::Arc; use std::{cmp, mem}; diff --git a/lib/engine-jit/src/unwind/dummy.rs b/lib/engine-jit/src/unwind/dummy.rs index 3d029ecd981..3322d1e8830 100644 --- a/lib/engine-jit/src/unwind/dummy.rs +++ b/lib/engine-jit/src/unwind/dummy.rs @@ -1,5 +1,6 @@ //! Module for Dummy unwind registry. +use crate::unwind::UnwindRegistryExt; use wasmer_compiler::CompiledFunctionUnwindInfo; /// Represents a registry of function unwind information when the host system @@ -11,7 +12,9 @@ impl DummyUnwindRegistry { pub fn new() -> Self { DummyUnwindRegistry {} } +} +impl UnwindRegistryExt for DummyUnwindRegistry { /// Registers a function given the start offset, length, and unwind information. pub fn register( &mut self, @@ -25,7 +28,7 @@ impl DummyUnwindRegistry { } /// Publishes all registered functions. - pub fn publish(&mut self, eh_frame: Option<&[u8]>) -> Result<(), String> { + pub fn publish(&mut self, eh_frame: Option>) -> Result<(), String> { // Do nothing Ok(()) } diff --git a/lib/engine-jit/src/unwind/mod.rs b/lib/engine-jit/src/unwind/mod.rs index 7d61694a210..21b4d92e924 100644 --- a/lib/engine-jit/src/unwind/mod.rs +++ b/lib/engine-jit/src/unwind/mod.rs @@ -1,3 +1,20 @@ +use wasmer_compiler::CompiledFunctionUnwindInfo; + +pub trait UnwindRegistryExt { + /// Registers a function given the start offset, length, and + /// unwind information. + fn register( + &mut self, + base_address: usize, + func_start: u32, + func_len: u32, + info: &CompiledFunctionUnwindInfo, + ) -> Result<(), String>; + + /// Publishes all registered functions. + fn publish(&mut self, eh_frame: Option>) -> Result<(), String>; +} + cfg_if::cfg_if! { if #[cfg(all(windows, target_arch = "x86_64"))] { mod windows_x64; diff --git a/lib/engine-jit/src/unwind/systemv.rs b/lib/engine-jit/src/unwind/systemv.rs index 86d33c25048..33c46ad250e 100644 --- a/lib/engine-jit/src/unwind/systemv.rs +++ b/lib/engine-jit/src/unwind/systemv.rs @@ -2,10 +2,13 @@ // Attributions: https://github.com/wasmerio/wasmer/blob/master/ATTRIBUTIONS.md //! Module for System V ABI unwind registry. + +use crate::unwind::UnwindRegistryExt; use wasmer_compiler::CompiledFunctionUnwindInfo; /// Represents a registry of function unwind information for System V ABI. pub struct UnwindRegistry { + eh_frame: Vec, registrations: Vec, published: bool, } @@ -20,50 +23,21 @@ impl UnwindRegistry { /// Creates a new unwind registry with the given base address. pub fn new() -> Self { Self { + eh_frame: Vec::new(), registrations: Vec::new(), published: false, } } - /// Registers a function given the start offset, length, and unwind information. - pub fn register( - &mut self, - _base_address: usize, - _func_start: u32, - _func_len: u32, - info: &CompiledFunctionUnwindInfo, - ) -> Result<(), String> { - match info { - CompiledFunctionUnwindInfo::Dwarf => {} - _ => return Err("unsupported unwind information".to_string()), - }; - Ok(()) - } - - /// Publishes all registered functions. - pub fn publish(&mut self, eh_frame: Option<&[u8]>) -> Result<(), String> { - if self.published { - return Err("unwind registry has already been published".to_string()); - } - - if let Some(eh_frame) = eh_frame { - unsafe { - self.register_frames(eh_frame); - } - } - - self.published = true; - - Ok(()) - } - #[allow(clippy::cast_ptr_alignment)] - unsafe fn register_frames(&mut self, eh_frame: &[u8]) { + unsafe fn register_frames(&mut self, eh_frame: Vec) { + self.eh_frame = eh_frame; + cfg_if::cfg_if! { if #[cfg(target_os = "macos")] { // On macOS, `__register_frame` takes a pointer to a single FDE - let start = eh_frame.as_ptr(); - let end = start.add(eh_frame.len()); + let start = self.eh_frame.as_ptr(); + let end = start.add(self.eh_frame.len()); let mut current = start; // Walk all of the entries in the frame table and register them @@ -87,9 +61,9 @@ impl UnwindRegistry { // deregistering it. We must avoid this // scenario. Usually, this is handled upstream by the // compilers. - debug_assert_ne!(eh_frame, &[0, 0, 0, 0], "`eh_frame` seems to contain empty FDEs"); + debug_assert_ne!(self.eh_frame.as_slice(), &[0, 0, 0, 0], "`eh_frame` seems to contain empty FDEs"); - let ptr = eh_frame.as_ptr(); + let ptr = self.eh_frame.as_ptr(); __register_frame(ptr); self.registrations.push(ptr as usize); } @@ -97,6 +71,40 @@ impl UnwindRegistry { } } +impl UnwindRegistryExt for UnwindRegistry { + /// Registers a function given the start offset, length, and unwind information. + fn register( + &mut self, + _base_address: usize, + _func_start: u32, + _func_len: u32, + info: &CompiledFunctionUnwindInfo, + ) -> Result<(), String> { + match info { + CompiledFunctionUnwindInfo::Dwarf => {} + _ => return Err("unsupported unwind information".to_string()), + }; + Ok(()) + } + + /// Publishes all registered functions. + fn publish(&mut self, eh_frame: Option>) -> Result<(), String> { + if self.published { + return Err("unwind registry has already been published".to_string()); + } + + if let Some(eh_frame) = eh_frame { + unsafe { + self.register_frames(eh_frame); + } + } + + self.published = true; + + Ok(()) + } +} + impl Drop for UnwindRegistry { fn drop(&mut self) { if self.published { diff --git a/lib/engine-jit/src/unwind/windows_x64.rs b/lib/engine-jit/src/unwind/windows_x64.rs index 96cac39239c..3a214d65610 100644 --- a/lib/engine-jit/src/unwind/windows_x64.rs +++ b/lib/engine-jit/src/unwind/windows_x64.rs @@ -2,6 +2,7 @@ // Attributions: https://github.com/wasmerio/wasmer/blob/master/ATTRIBUTIONS.md //! Module for Windows x64 ABI unwind registry. +use crate::unwind::UnwindRegistryExt; use std::collections::HashMap; use wasmer_compiler::CompiledFunctionUnwindInfo; use winapi::um::winnt; @@ -21,7 +22,9 @@ impl UnwindRegistry { published: false, } } +} +impl UnwindRegistryExt for UnwindRegistry { /// Registers a function given the start offset, length, and unwind information. pub fn register( &mut self, @@ -60,7 +63,7 @@ impl UnwindRegistry { } /// Publishes all registered functions. - pub fn publish(&mut self, _eh_frame: Option<&[u8]>) -> Result<(), String> { + pub fn publish(&mut self, _eh_frame: Option>) -> Result<(), String> { if self.published { return Err("unwind registry has already been published".to_string()); }