Merge pull request #449 from filecoin-project/fix/syscall-abi

Use repr(packed, C) for syscall types

Author: Stebalien

fvm/CHANGELOG.md

@@ -2,7 +2,12 @@
 
 Changes to the reference FVM implementation.
 
-## [Unreleased]
+## 0.7.0 [UNRELEASED]
+
+This release contains exactly one (breaking) change.
+
+BREAKING: Updates the FVM to the latest syscall struct alignment
+(https://github.com/filecoin-project/fvm-specs/issues/63).
 
 ## 0.6.0 [2022-04-13]
 

fvm/Cargo.toml

@@ -20,7 +20,7 @@ ahash = "0.7"
 num-derive = "0.3.3"
 cid = { version = "0.8.2", default-features = false, features = ["serde-codec"] }
 multihash = { version = "0.16.1", default-features = false }
-fvm_shared = { version = "0.5.1", path = "../shared", features = ["crypto"] }
+fvm_shared = { version = "0.6.0", path = "../shared", features = ["crypto"] }
 fvm_ipld_hamt = { version = "0.5.0", path = "../ipld/hamt"}
 fvm_ipld_amt = { version = "0.4.0", path = "../ipld/amt"}
 fvm_ipld_blockstore = { version = "0.1.0", path = "../ipld/blockstore" }

fvm/src/syscalls/bind.rs

@@ -1,6 +1,7 @@
 use std::mem;
 
 use fvm_shared::error::ErrorNumber;
+use fvm_shared::sys::SyscallSafe;
 use wasmtime::{Caller, Linker, Trap, WasmTy};
 
 use super::context::Memory;
@@ -50,14 +51,14 @@ pub(super) trait BindSyscall<Args, Ret, Func> {
 /// results that can be handled by wasmtime. See the documentation on `BindSyscall` for details.
 #[doc(hidden)]
 pub trait IntoSyscallResult: Sized {
-    type Value: Copy + Sized + 'static;
+    type Value: SyscallSafe;
     fn into(self) -> Result<Result<Self::Value, SyscallError>, Abort>;
 }
 
 // Implementations for syscalls that abort on error.
 impl<T> IntoSyscallResult for Result<T, Abort>
 where
-    T: Copy + Sized + 'static,
+    T: SyscallSafe,
 {
     type Value = T;
     fn into(self) -> Result<Result<Self::Value, SyscallError>, Abort> {
@@ -68,7 +69,7 @@ where
 // Implementations for normal syscalls.
 impl<T> IntoSyscallResult for kernel::Result<T>
 where
-    T: Copy + Sized + 'static,
+    T: SyscallSafe,
 {
     type Value = T;
     fn into(self) -> Result<Result<Self::Value, SyscallError>, Abort> {
@@ -135,7 +136,7 @@ macro_rules! impl_bind_syscalls {
             K: Kernel,
             Func: Fn(Context<'_, K> $(, $t)*) -> Ret + Send + Sync + 'static,
             Ret: IntoSyscallResult,
-           $($t: WasmTy,)*
+           $($t: WasmTy+SyscallSafe,)*
         {
             fn bind(
                 &mut self,

fvm/src/syscalls/vm.rs

@@ -1,4 +1,5 @@
 use fvm_shared::error::ExitCode;
+use fvm_shared::sys::SyscallSafe;
 use fvm_shared::version::NetworkVersion;
 
 use super::error::Abort;
@@ -11,6 +12,8 @@ use crate::Kernel;
 #[derive(Copy, Clone)]
 pub enum Never {}
 
+unsafe impl SyscallSafe for Never {}
+
 // NOTE: this won't clobber the last syscall error because it directly returns a "trap".
 pub fn abort(
     context: Context<'_, impl Kernel>,

sdk/CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## [Unreleased]
 
+## 0.6.0 [2022-04-14]
+
+BREAKING: Upgrades to fvm_shared 0.6.0, and the new syscall struct alignment.
+https://github.com/filecoin-project/fvm-specs/issues/63
+
 ## 0.5.0 [2022-04-11]
 
 Upgrades the SDK to fvm_shared 0.5.0. This release includes a significant breaking change to exit codes.

sdk/Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "fvm_sdk"
 description = "Filecoin Virtual Machine actor development SDK"
-version = "0.5.0"
+version = "0.6.0"
 license = "MIT OR Apache-2.0"
 authors = ["Protocol Labs", "Filecoin Core Devs"]
 edition = "2018"
@@ -12,7 +12,7 @@ crate-type = ["lib"]
 
 [dependencies]
 cid = { version = "0.8.2", default-features = false }
-fvm_shared = { version = "0.5.1", path = "../shared" }
+fvm_shared = { version = "0.6.0", path = "../shared" }
 ## num-traits; disabling default features makes it play nice with no_std.
 num-traits = { version = "0.2.14", default-features = false }
 lazy_static = { version = "1.4.0", optional = true }

shared/CHANGELOG.md

@@ -1,6 +1,11 @@
 # Changelog
 
-## [Unreleased]
+## 0.6.0 [2022-04-14]
+
+BREAKING: Switch syscall struct alignment: https://github.com/filecoin-project/fvm-specs/issues/63
+
+Actors built against this new version of fvm_shared will be incompatible with prior FVM versions,
+and vice-versa.
 
 ## 0.5.1  [2022-04-11]
 

shared/Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "fvm_shared"
 description = "Filecoin Virtual Machine shared types and functions"
-version = "0.5.1"
+version = "0.6.0"
 edition = "2021"
 license = "MIT OR Apache-2.0"
 authors = ["ChainSafe Systems <[email protected]>", "Protocol Labs", "Filecoin Core Devs"]

shared/src/sys/mod.rs

@@ -11,8 +11,8 @@ pub type Codec = u64;
 ///
 /// Internally, this type is a tuple of `u64`s storing the "low" and "high" bits of a little-endian
 /// u128.
-#[repr(C)]
 #[derive(Debug, Copy, Clone)]
+#[repr(packed, C)]
 pub struct TokenAmount {
     pub lo: u64,
     pub hi: u64,
@@ -43,3 +43,38 @@ impl<'a> TryFrom<&'a crate::econ::TokenAmount> for TokenAmount {
         })
     }
 }
+
+/// An unsafe trait to mark "syscall safe" types. These types must be safe to memcpy to and from
+/// WASM. This means:
+///
+/// 1. Repr C & packed alignment (no reordering, no padding).
+/// 2. Copy, Sized, and no pointers.
+/// 3. No floats (non-determinism).
+///
+/// # Safety
+///
+/// Incorrectly implementing this could lead to undefined behavior in types passed between wasm and
+/// rust.
+pub unsafe trait SyscallSafe: Copy + Sized + 'static {}
+
+macro_rules! assert_syscall_safe {
+    ($($t:ty,)*) => {
+        $(unsafe impl SyscallSafe for $t {})*
+    }
+}
+
+assert_syscall_safe! {
+    (),
+
+    u8, u16, u32, u64,
+    i8, i16, i32, i64,
+
+    TokenAmount,
+    out::actor::ResolveAddress,
+    out::ipld::IpldOpen,
+    out::ipld::IpldStat,
+    out::send::Send,
+    out::crypto::VerifyConsensusFault,
+}
+
+unsafe impl<T, const N: usize> SyscallSafe for [T; N] where T: SyscallSafe {}

shared/src/sys/out.rs

@@ -7,27 +7,33 @@
 //!
 //! Read more at https://github.com/rust-lang/rust/issues/73755.
 
+// NOTE: When possible, pack fields such that loads will be power-of-two aligned. Un-aligned loads
+// _can_ be done (LLVM will generate the appropriate code) but are slower.
+//
+// Read up on https://doc.rust-lang.org/reference/type-layout.html for more information.
+//
+// Also, please also read the docs on super::SyscallSafe before modifying any of these types.
+
 pub mod actor {
-    #[repr(C)]
     #[derive(Debug, Copy, Clone)]
+    #[repr(packed, C)]
     pub struct ResolveAddress {
-        pub resolved: i32,
         pub value: u64,
+        pub resolved: i32,
     }
 }
 
 pub mod ipld {
-    #[repr(C)]
     #[derive(Debug, Copy, Clone)]
+    #[repr(packed, C)]
     pub struct IpldOpen {
-        /// TODO could be more efficient to align id, size, codec, depending on padding.
-        pub id: u32,
         pub codec: u64,
+        pub id: u32,
         pub size: u32,
     }
 
-    #[repr(C)]
     #[derive(Debug, Copy, Clone)]
+    #[repr(packed, C)]
     pub struct IpldStat {
         pub codec: u64,
         pub size: u32,
@@ -37,8 +43,8 @@ pub mod ipld {
 pub mod send {
     use crate::sys::BlockId;
 
-    #[repr(C)]
     #[derive(Debug, Copy, Clone)]
+    #[repr(packed, C)]
     pub struct Send {
         pub exit_code: u32,
         pub return_id: BlockId,
@@ -48,11 +54,11 @@ pub mod send {
 pub mod crypto {
     use crate::{ActorID, ChainEpoch};
 
-    #[repr(C)]
     #[derive(Debug, Copy, Clone)]
+    #[repr(packed, C)]
     pub struct VerifyConsensusFault {
-        pub fault: u32,
         pub epoch: ChainEpoch,
         pub target: ActorID,
+        pub fault: u32,
     }
 }