From 206a8aeb7af95877527a61de2d9843a06ca35411 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 9 Feb 2024 06:37:05 -0800 Subject: [PATCH 1/4] Perform stronger typechecks of host-owned resources Right now the abstraction for host-owned resources in Wasmtime is quite simple, it's "just an index". This can be error prone because the host can suffer both from use-after-free and ABA-style problems. While there's not much that can be done about use-after-free the previous implementation would accidentally enable "AB" style problems where if a host-owned resource index was deallocated and then reallocated with another type the original handle would still work. While not a major bug this can be confusing and additionally violate's our guarantees as a component runtime to guests to ensure that resources always have valid `rep`s going into components. This commit adds a new layer of storage which keeps track of the `ResourceType` for all host-owned resources. This means that resources going into a host-owned table now record their type and coming out of a host-owned table a typecheck is always performed. Note that guests can continue to skip this logic as they already have per-type tables and so won't suffer the same issue. This change is inspired by my taking a look at #7883. The crux of the issue was a typo where a resource was reused by accident which led to confusing behavior due to the reuse. This change instead makes it return an error more quickly and doesn't allow the component to see any invalid state. Closes #7883 --- crates/wasmtime/src/runtime/component/func.rs | 2 +- .../src/runtime/component/func/options.rs | 86 +++--- .../src/runtime/component/resources.rs | 271 ++++++++++++++++-- crates/wasmtime/src/runtime/store.rs | 13 +- tests/all/component_model/resources.rs | 84 ++++-- 5 files changed, 357 insertions(+), 99 deletions(-) diff --git a/crates/wasmtime/src/runtime/component/func.rs b/crates/wasmtime/src/runtime/component/func.rs index 3088dec87221..337c6439e279 100644 --- a/crates/wasmtime/src/runtime/component/func.rs +++ b/crates/wasmtime/src/runtime/component/func.rs @@ -692,7 +692,7 @@ impl Func { // of the component. flags.set_may_enter(true); - let (calls, host_table) = store.0.component_calls_and_host_table(); + let (calls, host_table, _) = store.0.component_resource_state(); ResourceTables { calls, host_table: Some(host_table), diff --git a/crates/wasmtime/src/runtime/component/func/options.rs b/crates/wasmtime/src/runtime/component/func/options.rs index 6cb038f4f692..58c3c07c6c2e 100644 --- a/crates/wasmtime/src/runtime/component/func/options.rs +++ b/crates/wasmtime/src/runtime/component/func/options.rs @@ -1,4 +1,5 @@ use crate::component::matching::InstanceType; +use crate::component::resources::HostResourceTables; use crate::component::ResourceType; use crate::store::{StoreId, StoreOpaque}; use crate::StoreContextMut; @@ -278,7 +279,7 @@ impl<'a, T> LowerContext<'a, T> { /// /// The `ty` provided is which table to put this into. pub fn guest_resource_lower_own(&mut self, ty: TypeResourceTableIndex, rep: u32) -> u32 { - self.resource_tables().resource_lower_own(Some(ty), rep) + self.resource_tables().guest_resource_lower_own(rep, ty) } /// Lowers a `borrow` resource into the guest, converting the `rep` to a @@ -298,19 +299,20 @@ impl<'a, T> LowerContext<'a, T> { if unsafe { (*self.instance).resource_owned_by_own_instance(ty) } { return rep; } - self.resource_tables().resource_lower_borrow(Some(ty), rep) + self.resource_tables().guest_resource_lower_borrow(rep, ty) } /// Lifts a host-owned `own` resource at the `idx` specified into the /// representation of that resource. - pub fn host_resource_lift_own(&mut self, idx: u32) -> Result { - self.resource_tables().resource_lift_own(None, idx) + pub fn host_resource_lift_own(&mut self, idx: u32, expected: ResourceType) -> Result { + self.resource_tables().host_resource_lift_own(idx, expected) } /// Lifts a host-owned `borrow` resource at the `idx` specified into the /// representation of that resource. - pub fn host_resource_lift_borrow(&mut self, idx: u32) -> Result { - self.resource_tables().resource_lift_borrow(None, idx) + pub fn host_resource_lift_borrow(&mut self, idx: u32, expected: ResourceType) -> Result { + self.resource_tables() + .host_resource_lift_borrow(idx, expected) } /// Lowers a resource into the host-owned table, returning the index it was @@ -318,8 +320,8 @@ impl<'a, T> LowerContext<'a, T> { /// /// Note that this is a special case for `Resource`. Most of the time a /// host value shouldn't be lowered with a lowering context. - pub fn host_resource_lower_own(&mut self, rep: u32) -> u32 { - self.resource_tables().resource_lower_own(None, rep) + pub fn host_resource_lower_own(&mut self, rep: u32, ty: ResourceType) -> u32 { + self.resource_tables().host_resource_lower_own(rep, ty) } /// Returns the underlying resource type for the `ty` table specified. @@ -335,26 +337,27 @@ impl<'a, T> LowerContext<'a, T> { InstanceType::new(unsafe { &*self.instance }) } - fn resource_tables(&mut self) -> ResourceTables<'_> { - let (calls, host_table) = self.store.0.component_calls_and_host_table(); - ResourceTables { - host_table: Some(host_table), - calls, - // Note that the unsafety here should be valid given the contract of - // `LowerContext::new`. - tables: Some(unsafe { (*self.instance).component_resource_tables() }), - } + fn resource_tables(&mut self) -> HostResourceTables<'_> { + let (calls, host_table, host_resource_types) = self.store.0.component_resource_state(); + HostResourceTables::from_parts( + ResourceTables { + host_table: Some(host_table), + calls, + // Note that the unsafety here should be valid given the contract of + // `LowerContext::new`. + tables: Some(unsafe { (*self.instance).component_resource_tables() }), + }, + host_resource_types, + ) } - /// Begins a call into the component instance, starting recording of - /// metadata related to resource borrowing. + /// See [`HostResourceTables::enter_call`]. #[inline] pub fn enter_call(&mut self) { self.resource_tables().enter_call() } - /// Completes a call into the component instance, validating that it's ok to - /// complete by ensuring the are no remaining active borrows. + /// See [`HostResourceTables::exit_call`]. #[inline] pub fn exit_call(&mut self) -> Result<()> { self.resource_tables().exit_call() @@ -379,6 +382,7 @@ pub struct LiftContext<'a> { instance: *mut ComponentInstance, host_table: &'a mut ResourceTable, + host_resource_types: &'a mut Vec, calls: &'a mut CallContexts, } @@ -404,8 +408,8 @@ impl<'a> LiftContext<'a> { // so it's hacked around a bit. This unsafe pointer cast could be fixed // with more methods in more places, but it doesn't seem worth doing it // at this time. - let (calls, host_table) = - (&mut *(store as *mut StoreOpaque)).component_calls_and_host_table(); + let (calls, host_table, host_resource_types) = + (&mut *(store as *mut StoreOpaque)).component_resource_state(); let memory = options.memory.map(|_| options.memory(store)); LiftContext { @@ -415,6 +419,7 @@ impl<'a> LiftContext<'a> { instance, calls, host_table, + host_resource_types, } } @@ -450,7 +455,7 @@ impl<'a> LiftContext<'a> { ty: TypeResourceTableIndex, idx: u32, ) -> Result<(u32, Option>, Option)> { - let idx = self.resource_tables().resource_lift_own(Some(ty), idx)?; + let idx = self.resource_tables().guest_resource_lift_own(idx, ty)?; // Note that the unsafety here should be valid given the contract of // `LiftContext::new`. let (dtor, flags) = unsafe { (*self.instance).dtor_and_flags(ty) }; @@ -463,19 +468,19 @@ impl<'a> LiftContext<'a> { ty: TypeResourceTableIndex, idx: u32, ) -> Result { - self.resource_tables().resource_lift_borrow(Some(ty), idx) + self.resource_tables().guest_resource_lift_borrow(idx, ty) } /// Lowers a resource into the host-owned table, returning the index it was /// inserted at. - pub fn host_resource_lower_own(&mut self, rep: u32) -> u32 { - self.resource_tables().resource_lower_own(None, rep) + pub fn host_resource_lower_own(&mut self, rep: u32, ty: ResourceType) -> u32 { + self.resource_tables().host_resource_lower_own(rep, ty) } /// Lowers a resource into the host-owned table, returning the index it was /// inserted at. - pub fn host_resource_lower_borrow(&mut self, rep: u32) -> u32 { - self.resource_tables().resource_lower_borrow(None, rep) + pub fn host_resource_lower_borrow(&mut self, rep: u32, ty: ResourceType) -> u32 { + self.resource_tables().host_resource_lower_borrow(rep, ty) } /// Returns the underlying type of the resource table specified by `ty`. @@ -491,23 +496,26 @@ impl<'a> LiftContext<'a> { InstanceType::new(unsafe { &*self.instance }) } - fn resource_tables(&mut self) -> ResourceTables<'_> { - ResourceTables { - host_table: Some(self.host_table), - calls: self.calls, - // Note that the unsafety here should be valid given the contract of - // `LiftContext::new`. - tables: Some(unsafe { (*self.instance).component_resource_tables() }), - } + fn resource_tables(&mut self) -> HostResourceTables<'_> { + HostResourceTables::from_parts( + ResourceTables { + host_table: Some(self.host_table), + calls: self.calls, + // Note that the unsafety here should be valid given the contract of + // `LiftContext::new`. + tables: Some(unsafe { (*self.instance).component_resource_tables() }), + }, + self.host_resource_types, + ) } - /// Same as `LowerContext::enter_call` + /// See [`HostResourceTables::enter_call`]. #[inline] pub fn enter_call(&mut self) { self.resource_tables().enter_call() } - /// Same as `LiftContext::enter_call` + /// See [`HostResourceTables::exit_call`]. #[inline] pub fn exit_call(&mut self) -> Result<()> { self.resource_tables().exit_call() diff --git a/crates/wasmtime/src/runtime/component/resources.rs b/crates/wasmtime/src/runtime/component/resources.rs index 624514d1ffe7..2f9e3bb58eae 100644 --- a/crates/wasmtime/src/runtime/component/resources.rs +++ b/crates/wasmtime/src/runtime/component/resources.rs @@ -11,7 +11,9 @@ use std::fmt; use std::marker; use std::mem::MaybeUninit; use std::sync::atomic::{AtomicU32, Ordering::Relaxed}; -use wasmtime_environ::component::{CanonicalAbiInfo, DefinedResourceIndex, InterfaceType}; +use wasmtime_environ::component::{ + CanonicalAbiInfo, DefinedResourceIndex, InterfaceType, TypeResourceTableIndex, +}; use wasmtime_runtime::component::{ComponentInstance, InstanceFlags, ResourceTables}; use wasmtime_runtime::{SendSyncPtr, VMFuncRef, ValRaw}; @@ -238,15 +240,200 @@ const BORROW: u32 = u32::MAX; const NOT_IN_TABLE: u32 = u32::MAX - 1; const TAKEN: u32 = u32::MAX - 2; -fn host_resource_tables(store: &mut StoreOpaque) -> ResourceTables<'_> { - let (calls, host_table) = store.component_calls_and_host_table(); - ResourceTables { - calls, - host_table: Some(host_table), - tables: None, +/// TODO +pub struct HostResourceTables<'a> { + tables: ResourceTables<'a>, + host_resource_types: &'a mut Vec, +} + +struct UnusedHostTableSlot; + +impl<'a> HostResourceTables<'a> { + pub fn new_host(store: &'a mut StoreOpaque) -> HostResourceTables<'_> { + let (calls, host_table, host_resource_types) = store.component_resource_state(); + HostResourceTables::from_parts( + ResourceTables { + host_table: Some(host_table), + calls, + tables: None, + }, + host_resource_types, + ) + } + + pub fn from_parts( + tables: ResourceTables<'a>, + host_resource_types: &'a mut Vec, + ) -> Self { + HostResourceTables { + tables, + host_resource_types, + } + } + + /// Lifts an `own` resource that resides in the host's tables at the `idx` + /// specified into its `rep`. + /// + /// This method additionally takes an `expected` type which the resource is + /// expected to have. All host resources are stored into a single table so + /// this is used to perform a runtime check to ensure that the resource + /// still has the same type as when it was originally inserted. + /// + /// # Errors + /// + /// Returns an error if `idx` doesn't point to a valid owned resource, if + /// `idx` can't be lifted as an `own` (e.g. it has active borrows), or if + /// the resource at `idx` does not have the type `expected`. + pub fn host_resource_lift_own(&mut self, idx: u32, expected: ResourceType) -> Result { + self.validate_host_type(idx, expected)?; + self.tables.resource_lift_own(None, idx) + } + + /// See [`HostResourceTables::host_resource_lift_own`]. + pub fn host_resource_lift_borrow(&mut self, idx: u32, expected: ResourceType) -> Result { + self.validate_host_type(idx, expected)?; + self.tables.resource_lift_borrow(None, idx) + } + + /// Lowers an `own` resource to be owned by the host. + /// + /// This returns a new index into the host's set of resource tables which + /// will point to the `rep` specified as well as recording that it has the + /// `ty` specified. The returned index is suitable for conversion into + /// either [`Resource`] or [`ResourceAny`]. + pub fn host_resource_lower_own(&mut self, rep: u32, ty: ResourceType) -> u32 { + let idx = self.tables.resource_lower_own(None, rep); + self.register_host_type(idx, ty); + idx + } + + /// See [`HostResourceTables::host_resource_lower_own`]. + pub fn host_resource_lower_borrow(&mut self, rep: u32, ty: ResourceType) -> u32 { + let idx = self.tables.resource_lower_borrow(None, rep); + self.register_host_type(idx, ty); + idx + } + + /// Validates that the host resource at `idx` has the `expected` type. + /// + /// If `idx` is out-of-bounds or not actively being used then this method + /// does not return an error. That is deferred to retun an error via the + /// lift/drop operation corresponding to this method to return a more + /// precise error. + fn validate_host_type(&mut self, idx: u32, expected: ResourceType) -> Result<()> { + let actual = usize::try_from(idx) + .ok() + .and_then(|i| self.host_resource_types.get(i).copied()); + + // If `idx` is out-of-bounds, or if the slot is known as being + // not-in-use (e.g. dropped by the host) then skip returning an error. + // In such a situation the operation that this is guarding will return a + // more precise error, such as a lift operation. + if let Some(actual) = actual { + if actual != expected && actual != ResourceType::host::() { + bail!("host-owned resource is being used with the wrong type"); + } + } + Ok(()) + } + + fn register_host_type(&mut self, idx: u32, ty: ResourceType) { + let idx = idx as usize; + match self.host_resource_types.get_mut(idx) { + Some(slot) => *slot = ty, + None => { + assert_eq!(idx, self.host_resource_types.len()); + self.host_resource_types.push(ty); + } + } + } + + /// Drops a host-owned resource from host tables. + /// + /// This method will attempt to interpret `idx` as pointing to either a + /// `borrow` or `own` resource with the `expected` type specified. This + /// method will then return the underlying `rep` if it points to an `own` + /// resource which can then be further processed for destruction. + /// + /// # Errors + /// + /// Returns an error if `idx` doesn't point to a valid resource, points to + /// an `own` with active borrows, or if it doesn't have the type `expected` + /// in the host tables. + pub fn host_resource_drop(&mut self, idx: u32, expected: ResourceType) -> Result> { + self.validate_host_type(idx, expected)?; + let ret = self.tables.resource_drop(None, idx); + if ret.is_ok() { + self.host_resource_types[idx as usize] = ResourceType::host::(); + } + ret + } + + /// Lowers an `own` resource into the guest, converting the `rep` specified + /// into a guest-local index. + /// + /// The `ty` provided is which table to put this into. + pub fn guest_resource_lower_own(&mut self, rep: u32, ty: TypeResourceTableIndex) -> u32 { + self.tables.resource_lower_own(Some(ty), rep) + } + + /// Lowers a `borrow` resource into the guest, converting the `rep` + /// specified into a guest-local index. + /// + /// The `ty` provided is which table to put this into. + /// + /// Note that this cannot be used in isolation because lowering a borrow + /// into a guest has a special case where `rep` is returned directly if `ty` + /// belongs to the component being lowered into. That property must be + /// handled by the caller of this function. + pub fn guest_resource_lower_borrow(&mut self, rep: u32, ty: TypeResourceTableIndex) -> u32 { + self.tables.resource_lower_borrow(Some(ty), rep) + } + + /// Lifts an `own` resource from the `idx` specified from the table `ty`. + /// + /// This will lookup the appropriate table in the guest and return the `rep` + /// corresponding to `idx` if it's valid. + pub fn guest_resource_lift_own(&mut self, idx: u32, ty: TypeResourceTableIndex) -> Result { + self.tables.resource_lift_own(Some(ty), idx) + } + + /// Lifts a `borrow` resource from the `idx` specified from the table `ty`. + /// + /// This will lookup the appropriate table in the guest and return the `rep` + /// corresponding to `idx` if it's valid. + pub fn guest_resource_lift_borrow( + &mut self, + idx: u32, + ty: TypeResourceTableIndex, + ) -> Result { + self.tables.resource_lift_borrow(Some(ty), idx) + } + + /// Begins a call into the component instance, starting recording of + /// metadata related to resource borrowing. + #[inline] + pub fn enter_call(&mut self) { + self.tables.enter_call() + } + + /// Completes a call into the component instance, validating that it's ok to + /// complete by ensuring the are no remaining active borrows. + #[inline] + pub fn exit_call(&mut self) -> Result<()> { + self.tables.exit_call() } } +// fn host_resource_tables(store: &mut StoreOpaque) -> ResourceTables<'_> { +// let (calls, host_table, _) = store.component_resource_state(); +// ResourceTables { +// calls, +// host_table: Some(host_table), +// tables: None, +// } +// } + impl Resource where T: 'static, @@ -297,6 +484,7 @@ where } fn lower_to_index(&self, cx: &mut LowerContext<'_, U>, ty: InterfaceType) -> Result { + let rty = ResourceType::host::(); match ty { InterfaceType::Own(t) => { let rep = match self.state.load(Relaxed) { @@ -323,7 +511,7 @@ where // If this resource lives in a host table then try to take // it out of the table, which may fail, and on success we // can move the rep into the guest table. - idx => cx.host_resource_lift_own(idx)?, + idx => cx.host_resource_lift_own(idx, rty)?, }; Ok(cx.guest_resource_lower_own(t, rep)) } @@ -345,15 +533,15 @@ where // // Afterwards this is the same as the `idx` case below. NOT_IN_TABLE => { - let idx = cx.host_resource_lower_own(self.rep); + let idx = cx.host_resource_lower_own(self.rep, rty); let prev = self.state.swap(idx, Relaxed); assert_eq!(prev, NOT_IN_TABLE); - cx.host_resource_lift_borrow(idx)? + cx.host_resource_lift_borrow(idx, rty)? } // If this resource lives in a table then it needs to come // out of the table with borrow-tracking employed. - idx => cx.host_resource_lift_borrow(idx)?, + idx => cx.host_resource_lift_borrow(idx, ResourceType::host::())?, }; Ok(cx.guest_resource_lower_borrow(t, rep)) } @@ -398,22 +586,38 @@ where } /// Attempts to convert a [`ResourceAny`] into [`Resource`]. + /// + /// This method will check that `resource` has type + /// `ResourceType::host::()` and then convert it into a typed version of + /// the resource. + /// + /// # Errors + /// + /// This function will return an error if `resource` does not have type + /// `ResourceType::host::()`. This function may also return an error if + /// `resource` is no longer valid, for example it was previously converted. + /// + /// # Panics + /// + /// This function will panic if `resource` does not belong to the `store` + /// specified. pub fn try_from_resource_any( - ResourceAny { idx, ty, own_state }: ResourceAny, + resource: ResourceAny, mut store: impl AsContextMut, ) -> Result { let store = store.as_context_mut(); let store_id = store.0.id(); - let mut tables = host_resource_tables(store.0); - assert_eq!(ty, ResourceType::host::(), "resource type mismatch"); + let mut tables = HostResourceTables::new_host(store.0); + let ResourceAny { idx, ty, own_state } = resource; + ensure!(ty == ResourceType::host::(), "resource type mismatch"); let (state, rep) = if let Some(OwnState { store, dtor, flags }) = own_state { assert_eq!(store_id, store, "wrong store used to convert resource"); assert!(dtor.is_some(), "destructor must be set"); assert!(flags.is_none(), "flags must not be set"); - let rep = tables.resource_lift_own(None, idx)?; + let rep = tables.host_resource_lift_own(idx, ty)?; (AtomicU32::new(NOT_IN_TABLE), rep) } else { - let rep = tables.resource_lift_borrow(None, idx)?; + let rep = tables.host_resource_lift_borrow(idx, ty)?; (AtomicU32::new(BORROW), rep) }; Ok(Resource { @@ -542,15 +746,26 @@ struct OwnState { impl ResourceAny { /// Attempts to convert an imported [`Resource`] into [`ResourceAny`]. - /// `idx` is the [`ResourceImportIndex`] returned by [`Linker::resource`]. + /// + /// * `resource` is the resource to convert. + /// * `store` is the store to place the returned resource into. + /// * `instance_pre` is the instance from where `idx` below was derived. + /// * `idx` is the [`ResourceImportIndex`] returned by [`Linker::resource`]. /// /// [`Linker::resource`]: crate::component::LinkerInstance::resource + /// + /// # Errors + /// + /// This method will return an error if `idx` isn't valid for + /// `instance_pre` or if `resource` is not of the correct type for the + /// `idx` import. pub fn try_from_resource( - Resource { rep, state, .. }: Resource, + resource: Resource, mut store: impl AsContextMut, instance_pre: &InstancePre, idx: ResourceImportIndex, ) -> Result { + let Resource { rep, state, .. } = resource; let store = store.as_context_mut(); let import = instance_pre .resource_import(idx) @@ -563,11 +778,11 @@ impl ResourceAny { }; ensure!(*ty == ResourceType::host::(), "resource type mismatch"); - let mut tables = host_resource_tables(store.0); + let mut tables = HostResourceTables::new_host(store.0); let (idx, own_state) = match state.load(Relaxed) { - BORROW => (tables.resource_lower_borrow(None, rep), None), + BORROW => (tables.host_resource_lower_borrow(rep, *ty), None), NOT_IN_TABLE => { - let idx = tables.resource_lower_own(None, rep); + let idx = tables.host_resource_lower_own(rep, *ty); ( idx, Some(OwnState { @@ -656,7 +871,7 @@ impl ResourceAny { // // This could fail if the index is invalid or if this is removing an // `Own` entry which is currently being borrowed. - let rep = host_resource_tables(store.0).resource_drop(None, self.idx)?; + let rep = HostResourceTables::new_host(store.0).host_resource_drop(self.idx, self.ty)?; let (rep, state) = match (rep, &self.own_state) { (Some(rep), Some(state)) => (rep, state), @@ -708,16 +923,16 @@ impl ResourceAny { match ty { InterfaceType::Own(t) => { if cx.resource_type(t) != self.ty { - bail!("mismatched resource types") + bail!("mismatched resource types"); } - let rep = cx.host_resource_lift_own(self.idx)?; + let rep = cx.host_resource_lift_own(self.idx, self.ty)?; Ok(cx.guest_resource_lower_own(t, rep)) } InterfaceType::Borrow(t) => { if cx.resource_type(t) != self.ty { - bail!("mismatched resource types") + bail!("mismatched resource types"); } - let rep = cx.host_resource_lift_borrow(self.idx)?; + let rep = cx.host_resource_lift_borrow(self.idx, self.ty)?; Ok(cx.guest_resource_lower_borrow(t, rep)) } _ => bad_type_info(), @@ -729,7 +944,7 @@ impl ResourceAny { InterfaceType::Own(t) => { let ty = cx.resource_type(t); let (rep, dtor, flags) = cx.guest_resource_lift_own(t, index)?; - let idx = cx.host_resource_lower_own(rep); + let idx = cx.host_resource_lower_own(rep, ty); Ok(ResourceAny { idx, ty, @@ -743,7 +958,7 @@ impl ResourceAny { InterfaceType::Borrow(t) => { let ty = cx.resource_type(t); let rep = cx.guest_resource_lift_borrow(t, index)?; - let idx = cx.host_resource_lower_borrow(rep); + let idx = cx.host_resource_lower_borrow(rep, ty); Ok(ResourceAny { idx, ty, diff --git a/crates/wasmtime/src/runtime/store.rs b/crates/wasmtime/src/runtime/store.rs index 513474f8b012..c5e0ccc87c68 100644 --- a/crates/wasmtime/src/runtime/store.rs +++ b/crates/wasmtime/src/runtime/store.rs @@ -369,6 +369,8 @@ pub struct StoreOpaque { component_host_table: wasmtime_runtime::component::ResourceTable, #[cfg(feature = "component-model")] component_calls: wasmtime_runtime::component::CallContexts, + #[cfg(feature = "component-model")] + host_resource_types: Vec, } #[cfg(feature = "async")] @@ -520,6 +522,8 @@ impl Store { component_host_table: Default::default(), #[cfg(feature = "component-model")] component_calls: Default::default(), + #[cfg(feature = "component-model")] + host_resource_types: Vec::new(), }, limiter: None, call_hook: None, @@ -1627,13 +1631,18 @@ at https://bytecodealliance.org/security. #[inline] #[cfg(feature = "component-model")] - pub(crate) fn component_calls_and_host_table( + pub(crate) fn component_resource_state( &mut self, ) -> ( &mut wasmtime_runtime::component::CallContexts, &mut wasmtime_runtime::component::ResourceTable, + &mut Vec, ) { - (&mut self.component_calls, &mut self.component_host_table) + ( + &mut self.component_calls, + &mut self.component_host_table, + &mut self.host_resource_types, + ) } #[cfg(feature = "component-model")] diff --git a/tests/all/component_model/resources.rs b/tests/all/component_model/resources.rs index bb593894419b..9ac9461a16ee 100644 --- a/tests/all/component_model/resources.rs +++ b/tests/all/component_model/resources.rs @@ -137,44 +137,70 @@ fn resource_any() -> Result<()> { "#, )?; - let mut store = Store::new(&engine, ()); let linker = Linker::new(&engine); - let i = linker.instantiate(&mut store, &c)?; - let t = i.get_resource(&mut store, "t").unwrap(); - let u = i.get_resource(&mut store, "u").unwrap(); + { + let mut store = Store::new(&engine, ()); + let i = linker.instantiate(&mut store, &c)?; + let t = i.get_resource(&mut store, "t").unwrap(); + let u = i.get_resource(&mut store, "u").unwrap(); + + assert_ne!(t, u); + + let t_ctor = i.get_typed_func::<(u32,), (ResourceAny,)>(&mut store, "[constructor]t")?; + let u_ctor = i.get_typed_func::<(u32,), (ResourceAny,)>(&mut store, "[constructor]u")?; + let t_dtor = i.get_typed_func::<(ResourceAny,), ()>(&mut store, "drop-t")?; + let u_dtor = i.get_typed_func::<(ResourceAny,), ()>(&mut store, "drop-u")?; + + let (t1,) = t_ctor.call(&mut store, (100,))?; + t_ctor.post_return(&mut store)?; + let (t2,) = t_ctor.call(&mut store, (200,))?; + t_ctor.post_return(&mut store)?; + let (u1,) = u_ctor.call(&mut store, (300,))?; + u_ctor.post_return(&mut store)?; + let (u2,) = u_ctor.call(&mut store, (400,))?; + u_ctor.post_return(&mut store)?; - assert_ne!(t, u); + assert_eq!(t1.ty(), t); + assert_eq!(t2.ty(), t); + assert_eq!(u1.ty(), u); + assert_eq!(u2.ty(), u); - let t_ctor = i.get_typed_func::<(u32,), (ResourceAny,)>(&mut store, "[constructor]t")?; - let u_ctor = i.get_typed_func::<(u32,), (ResourceAny,)>(&mut store, "[constructor]u")?; - let t_dtor = i.get_typed_func::<(ResourceAny,), ()>(&mut store, "drop-t")?; - let u_dtor = i.get_typed_func::<(ResourceAny,), ()>(&mut store, "drop-u")?; + u_dtor.call(&mut store, (u2,))?; + u_dtor.post_return(&mut store)?; - let (t1,) = t_ctor.call(&mut store, (100,))?; - t_ctor.post_return(&mut store)?; - let (t2,) = t_ctor.call(&mut store, (200,))?; - t_ctor.post_return(&mut store)?; - let (u1,) = u_ctor.call(&mut store, (300,))?; - u_ctor.post_return(&mut store)?; - let (u2,) = u_ctor.call(&mut store, (400,))?; - u_ctor.post_return(&mut store)?; + u_dtor.call(&mut store, (u1,))?; + u_dtor.post_return(&mut store)?; - assert_eq!(t1.ty(), t); - assert_eq!(t2.ty(), t); - assert_eq!(u1.ty(), u); - assert_eq!(u2.ty(), u); + t_dtor.call(&mut store, (t1,))?; + t_dtor.post_return(&mut store)?; - u_dtor.call(&mut store, (u2,))?; - u_dtor.post_return(&mut store)?; + t_dtor.call(&mut store, (t2,))?; + t_dtor.post_return(&mut store)?; + } + + { + let mut store = Store::new(&engine, ()); + let i = linker.instantiate(&mut store, &c)?; + let t_ctor = i.get_typed_func::<(u32,), (ResourceAny,)>(&mut store, "[constructor]t")?; + let u_ctor = i.get_typed_func::<(u32,), (ResourceAny,)>(&mut store, "[constructor]u")?; + let t_dtor = i.get_typed_func::<(ResourceAny,), ()>(&mut store, "drop-t")?; - u_dtor.call(&mut store, (u1,))?; - u_dtor.post_return(&mut store)?; + // `t` is placed at host index 0 + let (t,) = t_ctor.call(&mut store, (100,))?; + t_ctor.post_return(&mut store)?; + t_dtor.call(&mut store, (t1,))?; + t_dtor.post_return(&mut store)?; - t_dtor.call(&mut store, (t1,))?; - t_dtor.post_return(&mut store)?; + // `u` is also placed at host index 0 since `t` was deallocated + let (u,) = u_ctor.call(&mut store, (100,))?; + u_ctor.post_return(&mut store)?; - t_dtor.call(&mut store, (t2,))?; - t_dtor.post_return(&mut store)?; + // reuse of `t` should fail, despite it pointing to a valid resource + assert_eq!( + t_dtor.call(&mut store, (t1,)).unwrap_err().to_string(), + "host-owned resource is being used with the wrong type" + ); + } Ok(()) } From b245988a7f3c952e1c3565fb5abdc4355bae0329 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 9 Feb 2024 07:09:46 -0800 Subject: [PATCH 2/4] Fix test --- tests/all/component_model/resources.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/all/component_model/resources.rs b/tests/all/component_model/resources.rs index 9ac9461a16ee..08059df4c218 100644 --- a/tests/all/component_model/resources.rs +++ b/tests/all/component_model/resources.rs @@ -188,16 +188,16 @@ fn resource_any() -> Result<()> { // `t` is placed at host index 0 let (t,) = t_ctor.call(&mut store, (100,))?; t_ctor.post_return(&mut store)?; - t_dtor.call(&mut store, (t1,))?; + t_dtor.call(&mut store, (t,))?; t_dtor.post_return(&mut store)?; // `u` is also placed at host index 0 since `t` was deallocated - let (u,) = u_ctor.call(&mut store, (100,))?; + let (_u,) = u_ctor.call(&mut store, (100,))?; u_ctor.post_return(&mut store)?; // reuse of `t` should fail, despite it pointing to a valid resource assert_eq!( - t_dtor.call(&mut store, (t1,)).unwrap_err().to_string(), + t_dtor.call(&mut store, (t,)).unwrap_err().to_string(), "host-owned resource is being used with the wrong type" ); } From 43e6350878288262a9c680f0d97a08f6194bf53e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 12 Feb 2024 12:01:33 -0800 Subject: [PATCH 3/4] Use generation counters, not types --- .../src/runtime/component/func/options.rs | 35 +- crates/wasmtime/src/runtime/component/mod.rs | 2 + .../src/runtime/component/resources.rs | 368 +++++++++++------- crates/wasmtime/src/runtime/store.rs | 8 +- 4 files changed, 256 insertions(+), 157 deletions(-) diff --git a/crates/wasmtime/src/runtime/component/func/options.rs b/crates/wasmtime/src/runtime/component/func/options.rs index 58c3c07c6c2e..dc5c574a7eda 100644 --- a/crates/wasmtime/src/runtime/component/func/options.rs +++ b/crates/wasmtime/src/runtime/component/func/options.rs @@ -1,5 +1,5 @@ use crate::component::matching::InstanceType; -use crate::component::resources::HostResourceTables; +use crate::component::resources::{HostResourceData, HostResourceIndex, HostResourceTables}; use crate::component::ResourceType; use crate::store::{StoreId, StoreOpaque}; use crate::StoreContextMut; @@ -304,15 +304,14 @@ impl<'a, T> LowerContext<'a, T> { /// Lifts a host-owned `own` resource at the `idx` specified into the /// representation of that resource. - pub fn host_resource_lift_own(&mut self, idx: u32, expected: ResourceType) -> Result { - self.resource_tables().host_resource_lift_own(idx, expected) + pub fn host_resource_lift_own(&mut self, idx: HostResourceIndex) -> Result { + self.resource_tables().host_resource_lift_own(idx) } /// Lifts a host-owned `borrow` resource at the `idx` specified into the /// representation of that resource. - pub fn host_resource_lift_borrow(&mut self, idx: u32, expected: ResourceType) -> Result { - self.resource_tables() - .host_resource_lift_borrow(idx, expected) + pub fn host_resource_lift_borrow(&mut self, idx: HostResourceIndex) -> Result { + self.resource_tables().host_resource_lift_borrow(idx) } /// Lowers a resource into the host-owned table, returning the index it was @@ -320,8 +319,8 @@ impl<'a, T> LowerContext<'a, T> { /// /// Note that this is a special case for `Resource`. Most of the time a /// host value shouldn't be lowered with a lowering context. - pub fn host_resource_lower_own(&mut self, rep: u32, ty: ResourceType) -> u32 { - self.resource_tables().host_resource_lower_own(rep, ty) + pub fn host_resource_lower_own(&mut self, rep: u32) -> HostResourceIndex { + self.resource_tables().host_resource_lower_own(rep) } /// Returns the underlying resource type for the `ty` table specified. @@ -338,7 +337,7 @@ impl<'a, T> LowerContext<'a, T> { } fn resource_tables(&mut self) -> HostResourceTables<'_> { - let (calls, host_table, host_resource_types) = self.store.0.component_resource_state(); + let (calls, host_table, host_resource_data) = self.store.0.component_resource_state(); HostResourceTables::from_parts( ResourceTables { host_table: Some(host_table), @@ -347,7 +346,7 @@ impl<'a, T> LowerContext<'a, T> { // `LowerContext::new`. tables: Some(unsafe { (*self.instance).component_resource_tables() }), }, - host_resource_types, + host_resource_data, ) } @@ -382,7 +381,7 @@ pub struct LiftContext<'a> { instance: *mut ComponentInstance, host_table: &'a mut ResourceTable, - host_resource_types: &'a mut Vec, + host_resource_data: &'a mut HostResourceData, calls: &'a mut CallContexts, } @@ -408,7 +407,7 @@ impl<'a> LiftContext<'a> { // so it's hacked around a bit. This unsafe pointer cast could be fixed // with more methods in more places, but it doesn't seem worth doing it // at this time. - let (calls, host_table, host_resource_types) = + let (calls, host_table, host_resource_data) = (&mut *(store as *mut StoreOpaque)).component_resource_state(); let memory = options.memory.map(|_| options.memory(store)); @@ -419,7 +418,7 @@ impl<'a> LiftContext<'a> { instance, calls, host_table, - host_resource_types, + host_resource_data, } } @@ -473,14 +472,14 @@ impl<'a> LiftContext<'a> { /// Lowers a resource into the host-owned table, returning the index it was /// inserted at. - pub fn host_resource_lower_own(&mut self, rep: u32, ty: ResourceType) -> u32 { - self.resource_tables().host_resource_lower_own(rep, ty) + pub fn host_resource_lower_own(&mut self, rep: u32) -> HostResourceIndex { + self.resource_tables().host_resource_lower_own(rep) } /// Lowers a resource into the host-owned table, returning the index it was /// inserted at. - pub fn host_resource_lower_borrow(&mut self, rep: u32, ty: ResourceType) -> u32 { - self.resource_tables().host_resource_lower_borrow(rep, ty) + pub fn host_resource_lower_borrow(&mut self, rep: u32) -> HostResourceIndex { + self.resource_tables().host_resource_lower_borrow(rep) } /// Returns the underlying type of the resource table specified by `ty`. @@ -505,7 +504,7 @@ impl<'a> LiftContext<'a> { // `LiftContext::new`. tables: Some(unsafe { (*self.instance).component_resource_tables() }), }, - self.host_resource_types, + self.host_resource_data, ) } diff --git a/crates/wasmtime/src/runtime/component/mod.rs b/crates/wasmtime/src/runtime/component/mod.rs index 821ef62590f9..d8d50bb54df3 100644 --- a/crates/wasmtime/src/runtime/component/mod.rs +++ b/crates/wasmtime/src/runtime/component/mod.rs @@ -63,6 +63,8 @@ pub use self::resources::{Resource, ResourceAny}; pub use self::types::{ResourceType, Type}; pub use self::values::{Enum, Flags, List, OptionVal, Record, ResultVal, Tuple, Val, Variant}; +pub(crate) use self::resources::HostResourceData; + // These items are expected to be used by an eventual // `#[derive(ComponentType)]`, they are not part of Wasmtime's API stability // guarantees diff --git a/crates/wasmtime/src/runtime/component/resources.rs b/crates/wasmtime/src/runtime/component/resources.rs index 2f9e3bb58eae..a8132a896a48 100644 --- a/crates/wasmtime/src/runtime/component/resources.rs +++ b/crates/wasmtime/src/runtime/component/resources.rs @@ -10,7 +10,7 @@ use std::any::TypeId; use std::fmt; use std::marker; use std::mem::MaybeUninit; -use std::sync::atomic::{AtomicU32, Ordering::Relaxed}; +use std::sync::atomic::{AtomicU64, Ordering::Relaxed}; use wasmtime_environ::component::{ CanonicalAbiInfo, DefinedResourceIndex, InterfaceType, TypeResourceTableIndex, }; @@ -203,71 +203,166 @@ pub struct Resource { /// Dear rust please consider `T` used even though it's not actually used. _marker: marker::PhantomData T>, - /// Internal dynamic state tracking for this resource. This can be one of - /// four different states: - /// - /// * `BORROW` / `u32::MAX` - this indicates that this is a borrowed - /// resource. The `rep` doesn't live in the host table and this `Resource` - /// instance is transiently available. It's the host's responsibility to - /// discard this resource when the borrow duration has finished. - /// - /// * `NOT_IN_TABLE` / `u32::MAX - 1` - this indicates that this is an owned - /// resource not present in any store's table. This resource is not lent - /// out. It can be passed as an `(own $t)` directly into a guest's table - /// or it can be passed as a borrow to a guest which will insert it into - /// a host store's table for dynamic borrow tracking. - /// - /// * `TAKEN` / `u32::MAX - 2` - while the `rep` is available the resource - /// has been dynamically moved into a guest and cannot be moved in again. - /// This is used for example to prevent the same resource from being - /// passed twice to a guest. - /// - /// * All other values - any other value indicates that the value is an - /// index into a store's table of host resources. It's guaranteed that the - /// table entry represents a host resource and the resource may have - /// borrow tracking associated with it. - /// - /// Note that this is an `AtomicU32` but it's not intended to actually be - /// used in conjunction with threads as generally a `Store` lives on one - /// thread at a time. The `AtomicU32` here is used to ensure that this type - /// is `Send + Sync` when captured as a reference to make async programming - /// more ergonomic. - state: AtomicU32, + state: AtomicResourceState, } -// See comments on `state` above for info about these values. -const BORROW: u32 = u32::MAX; -const NOT_IN_TABLE: u32 = u32::MAX - 1; -const TAKEN: u32 = u32::MAX - 2; +/// Internal dynamic state tracking for this resource. This can be one of +/// four different states: +/// +/// * `BORROW` / `u64::MAX` - this indicates that this is a borrowed +/// resource. The `rep` doesn't live in the host table and this `Resource` +/// instance is transiently available. It's the host's responsibility to +/// discard this resource when the borrow duration has finished. +/// +/// * `NOT_IN_TABLE` / `u64::MAX - 1` - this indicates that this is an owned +/// resource not present in any store's table. This resource is not lent +/// out. It can be passed as an `(own $t)` directly into a guest's table +/// or it can be passed as a borrow to a guest which will insert it into +/// a host store's table for dynamic borrow tracking. +/// +/// * `TAKEN` / `u64::MAX - 2` - while the `rep` is available the resource +/// has been dynamically moved into a guest and cannot be moved in again. +/// This is used for example to prevent the same resource from being +/// passed twice to a guest. +/// +/// * All other values - any other value indicates that the value is an +/// index into a store's table of host resources. It's guaranteed that the +/// table entry represents a host resource and the resource may have +/// borrow tracking associated with it. The low 32-bits of the value are +/// the table index and the upper 32-bits are the generation. +/// +/// Note that this is an `AtomicU64` but it's not intended to actually be +/// used in conjunction with threads as generally a `Store` lives on one +/// thread at a time. The `AtomicU64` here is used to ensure that this type +/// is `Send + Sync` when captured as a reference to make async programming +/// more ergonomic. +struct AtomicResourceState(AtomicU64); + +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +enum ResourceState { + Borrow, + NotInTable, + Taken, + Index(HostResourceIndex), +} + +impl AtomicResourceState { + const BORROW: Self = Self(AtomicU64::new(ResourceState::BORROW)); + const NOT_IN_TABLE: Self = Self(AtomicU64::new(ResourceState::NOT_IN_TABLE)); + + fn get(&self) -> ResourceState { + ResourceState::decode(self.0.load(Relaxed)) + } + + fn swap(&self, state: ResourceState) -> ResourceState { + ResourceState::decode(self.0.swap(state.encode(), Relaxed)) + } +} + +impl ResourceState { + // See comments on `state` above for info about these values. + const BORROW: u64 = u64::MAX; + const NOT_IN_TABLE: u64 = u64::MAX - 1; + const TAKEN: u64 = u64::MAX - 2; + + fn decode(bits: u64) -> ResourceState { + match bits { + Self::BORROW => Self::Borrow, + Self::NOT_IN_TABLE => Self::NotInTable, + Self::TAKEN => Self::Taken, + other => Self::Index(HostResourceIndex(other)), + } + } + + fn encode(&self) -> u64 { + match self { + Self::Borrow => Self::BORROW, + Self::NotInTable => Self::NOT_IN_TABLE, + Self::Taken => Self::TAKEN, + Self::Index(index) => index.0, + } + } +} -/// TODO +/// Metadata tracking the state of resources within a `Store`. +/// +/// This is a borrowed structure created from a `Store` piecemeal from below. +/// The `ResourceTables` type holds most of the raw information and this +/// structure tacks on a reference to `HostResourceData` to track generation +/// numbers of host indices. pub struct HostResourceTables<'a> { tables: ResourceTables<'a>, - host_resource_types: &'a mut Vec, + host_resource_data: &'a mut HostResourceData, +} + +/// Metadata for host-owned resources owned within a `Store`. +/// +/// This metadata is used to prevent the ABA problem with indices handed out as +/// part of `Resource` and `ResourceAny`. Those structures are `Copy` meaning +/// that it's easy to reuse them, possibly accidentally. To prevent issues in +/// the host Wasmtime attaches both an index (within `ResourceTables`) as well +/// as a 32-bit generation counter onto each `HostResourceIndex` which the host +/// actually holds in `Resource` and `ResourceAny`. +/// +/// This structure holds a list which is a parallel list to the "list of reps" +/// that's stored within `ResourceTables` elsewhere in the `Store`. This +/// parallel list holds the last known generation of each element in the table. +/// The generation is then compared on access to make sure it's the same. +/// +/// Whenever a slot in the table is allocated the `cur_generation` field is +/// pushed at the corresponding index of `generation_of_table_slot`. Whenever +/// a field is accessed the current value of `generation_of_table_slot` is +/// checked against the generation of the index. Whenever a slot is deallocated +/// the generation is incremented. Put together this means that any access of a +/// deallocated slot should deterministically provide an error. +#[derive(Default)] +pub struct HostResourceData { + cur_generation: u32, + generation_of_table_slot: Vec, } -struct UnusedHostTableSlot; +/// Host representation of an index into a table slot. +/// +/// This is morally (u32, u32) but is encoded as a 64-bit integer. The low +/// 32-bits are the table index and the upper 32-bits are the generation +/// counter. +#[derive(PartialEq, Eq, Debug, Copy, Clone)] +#[repr(transparent)] +pub struct HostResourceIndex(u64); + +impl HostResourceIndex { + fn new(idx: u32, gen: u32) -> HostResourceIndex { + HostResourceIndex(u64::from(idx) | (u64::from(gen) << 32)) + } + + fn index(&self) -> u32 { + self.0 as u32 + } + fn gen(&self) -> u32 { + (self.0 >> 32) as u32 + } +} impl<'a> HostResourceTables<'a> { pub fn new_host(store: &'a mut StoreOpaque) -> HostResourceTables<'_> { - let (calls, host_table, host_resource_types) = store.component_resource_state(); + let (calls, host_table, host_resource_data) = store.component_resource_state(); HostResourceTables::from_parts( ResourceTables { host_table: Some(host_table), calls, tables: None, }, - host_resource_types, + host_resource_data, ) } pub fn from_parts( tables: ResourceTables<'a>, - host_resource_types: &'a mut Vec, + host_resource_data: &'a mut HostResourceData, ) -> Self { HostResourceTables { tables, - host_resource_types, + host_resource_data, } } @@ -284,14 +379,14 @@ impl<'a> HostResourceTables<'a> { /// Returns an error if `idx` doesn't point to a valid owned resource, if /// `idx` can't be lifted as an `own` (e.g. it has active borrows), or if /// the resource at `idx` does not have the type `expected`. - pub fn host_resource_lift_own(&mut self, idx: u32, expected: ResourceType) -> Result { - self.validate_host_type(idx, expected)?; + pub fn host_resource_lift_own(&mut self, idx: HostResourceIndex) -> Result { + let idx = self.validate_host_index(idx, true)?; self.tables.resource_lift_own(None, idx) } /// See [`HostResourceTables::host_resource_lift_own`]. - pub fn host_resource_lift_borrow(&mut self, idx: u32, expected: ResourceType) -> Result { - self.validate_host_type(idx, expected)?; + pub fn host_resource_lift_borrow(&mut self, idx: HostResourceIndex) -> Result { + let idx = self.validate_host_index(idx, false)?; self.tables.resource_lift_borrow(None, idx) } @@ -301,51 +396,68 @@ impl<'a> HostResourceTables<'a> { /// will point to the `rep` specified as well as recording that it has the /// `ty` specified. The returned index is suitable for conversion into /// either [`Resource`] or [`ResourceAny`]. - pub fn host_resource_lower_own(&mut self, rep: u32, ty: ResourceType) -> u32 { + pub fn host_resource_lower_own(&mut self, rep: u32) -> HostResourceIndex { let idx = self.tables.resource_lower_own(None, rep); - self.register_host_type(idx, ty); - idx + self.new_host_index(idx) } /// See [`HostResourceTables::host_resource_lower_own`]. - pub fn host_resource_lower_borrow(&mut self, rep: u32, ty: ResourceType) -> u32 { + pub fn host_resource_lower_borrow(&mut self, rep: u32) -> HostResourceIndex { let idx = self.tables.resource_lower_borrow(None, rep); - self.register_host_type(idx, ty); - idx + self.new_host_index(idx) } - /// Validates that the host resource at `idx` has the `expected` type. + /// Validates that `idx` is still valid for the host tables, notably + /// ensuring that the generation listed in `idx` is the same as the + /// last recorded generation of the slot itself. /// - /// If `idx` is out-of-bounds or not actively being used then this method - /// does not return an error. That is deferred to retun an error via the - /// lift/drop operation corresponding to this method to return a more - /// precise error. - fn validate_host_type(&mut self, idx: u32, expected: ResourceType) -> Result<()> { - let actual = usize::try_from(idx) - .ok() - .and_then(|i| self.host_resource_types.get(i).copied()); - - // If `idx` is out-of-bounds, or if the slot is known as being - // not-in-use (e.g. dropped by the host) then skip returning an error. - // In such a situation the operation that this is guarding will return a - // more precise error, such as a lift operation. + /// The `is_removal` option indicates whether or not this table access will + /// end up removing the element from the host table. In such a situation the + /// current generation number is incremented. + fn validate_host_index(&mut self, idx: HostResourceIndex, is_removal: bool) -> Result { + let actual = usize::try_from(idx.index()).ok().and_then(|i| { + self.host_resource_data + .generation_of_table_slot + .get(i) + .copied() + }); + + // If `idx` is out-of-bounds then skip returning an error. In such a + // situation the operation that this is guarding will return a more + // precise error, such as a lift operation. if let Some(actual) = actual { - if actual != expected && actual != ResourceType::host::() { + if actual != idx.gen() { bail!("host-owned resource is being used with the wrong type"); } } - Ok(()) + + // Bump the current generation of this is a removal to ensure any + // future item placed in this slot can't be pointed to by the `idx` + // provided above. + if is_removal { + self.host_resource_data.cur_generation += 1; + } + + Ok(idx.index()) } - fn register_host_type(&mut self, idx: u32, ty: ResourceType) { - let idx = idx as usize; - match self.host_resource_types.get_mut(idx) { - Some(slot) => *slot = ty, + /// Creates a new `HostResourceIndex` which will point to the raw table + /// slot provided by `idx`. + /// + /// This will register metadata necessary to track the current generation + /// in the returned `HostResourceIndex` as well. + fn new_host_index(&mut self, idx: u32) -> HostResourceIndex { + let list = &mut self.host_resource_data.generation_of_table_slot; + let gen = self.host_resource_data.cur_generation; + match list.get_mut(idx as usize) { + Some(slot) => *slot = gen, None => { - assert_eq!(idx, self.host_resource_types.len()); - self.host_resource_types.push(ty); + assert_eq!(idx as usize, list.len()); + list.push(gen); } } + + HostResourceIndex::new(idx, gen) } /// Drops a host-owned resource from host tables. @@ -360,13 +472,9 @@ impl<'a> HostResourceTables<'a> { /// Returns an error if `idx` doesn't point to a valid resource, points to /// an `own` with active borrows, or if it doesn't have the type `expected` /// in the host tables. - pub fn host_resource_drop(&mut self, idx: u32, expected: ResourceType) -> Result> { - self.validate_host_type(idx, expected)?; - let ret = self.tables.resource_drop(None, idx); - if ret.is_ok() { - self.host_resource_types[idx as usize] = ResourceType::host::(); - } - ret + pub fn host_resource_drop(&mut self, idx: HostResourceIndex) -> Result> { + let idx = self.validate_host_index(idx, true)?; + self.tables.resource_drop(None, idx) } /// Lowers an `own` resource into the guest, converting the `rep` specified @@ -425,15 +533,6 @@ impl<'a> HostResourceTables<'a> { } } -// fn host_resource_tables(store: &mut StoreOpaque) -> ResourceTables<'_> { -// let (calls, host_table, _) = store.component_resource_state(); -// ResourceTables { -// calls, -// host_table: Some(host_table), -// tables: None, -// } -// } - impl Resource where T: 'static, @@ -444,7 +543,7 @@ where /// `(borrow $t)` or `(own $t)`. pub fn new_own(rep: u32) -> Resource { Resource { - state: AtomicU32::new(NOT_IN_TABLE), + state: AtomicResourceState::NOT_IN_TABLE, rep, _marker: marker::PhantomData, } @@ -459,7 +558,7 @@ where /// embedder. pub fn new_borrow(rep: u32) -> Resource { Resource { - state: AtomicU32::new(BORROW), + state: AtomicResourceState::BORROW, rep, _marker: marker::PhantomData, } @@ -477,20 +576,19 @@ where /// borrowed resources have an owner somewhere else on the stack so can only /// be accessed, not destroyed. pub fn owned(&self) -> bool { - match self.state.load(Relaxed) { - BORROW => false, - _ => true, + match self.state.get() { + ResourceState::Borrow => false, + ResourceState::Taken | ResourceState::NotInTable | ResourceState::Index(_) => true, } } fn lower_to_index(&self, cx: &mut LowerContext<'_, U>, ty: InterfaceType) -> Result { - let rty = ResourceType::host::(); match ty { InterfaceType::Own(t) => { - let rep = match self.state.load(Relaxed) { + let rep = match self.state.get() { // If this is a borrow resource then this is a dynamic // error on behalf of the embedder. - BORROW => { + ResourceState::Borrow => { bail!("cannot lower a `borrow` resource into an `own`") } @@ -498,32 +596,32 @@ where // dynamically transferring ownership to wasm. Record that // it's no longer present and then pass through the // representation. - NOT_IN_TABLE => { - let prev = self.state.swap(TAKEN, Relaxed); - assert_eq!(prev, NOT_IN_TABLE); + ResourceState::NotInTable => { + let prev = self.state.swap(ResourceState::Taken); + assert_eq!(prev, ResourceState::NotInTable); self.rep } // This resource has already been moved into wasm so this is // a dynamic error on behalf of the embedder. - TAKEN => bail!("host resource already consumed"), + ResourceState::Taken => bail!("host resource already consumed"), // If this resource lives in a host table then try to take // it out of the table, which may fail, and on success we // can move the rep into the guest table. - idx => cx.host_resource_lift_own(idx, rty)?, + ResourceState::Index(idx) => cx.host_resource_lift_own(idx)?, }; Ok(cx.guest_resource_lower_own(t, rep)) } InterfaceType::Borrow(t) => { - let rep = match self.state.load(Relaxed) { + let rep = match self.state.get() { // If this is already a borrowed resource, nothing else to // do and the rep is passed through. - BORROW => self.rep, + ResourceState::Borrow => self.rep, // If this resource is already gone, that's a dynamic error // for the embedder. - TAKEN => bail!("host resource already consumed"), + ResourceState::Taken => bail!("host resource already consumed"), // If this resource is not currently in a table then it // needs to move into a table to participate in state @@ -532,16 +630,16 @@ where // state. // // Afterwards this is the same as the `idx` case below. - NOT_IN_TABLE => { - let idx = cx.host_resource_lower_own(self.rep, rty); - let prev = self.state.swap(idx, Relaxed); - assert_eq!(prev, NOT_IN_TABLE); - cx.host_resource_lift_borrow(idx, rty)? + ResourceState::NotInTable => { + let idx = cx.host_resource_lower_own(self.rep); + let prev = self.state.swap(ResourceState::Index(idx)); + assert_eq!(prev, ResourceState::NotInTable); + cx.host_resource_lift_borrow(idx)? } // If this resource lives in a table then it needs to come // out of the table with borrow-tracking employed. - idx => cx.host_resource_lift_borrow(idx, ResourceType::host::())?, + ResourceState::Index(idx) => cx.host_resource_lift_borrow(idx)?, }; Ok(cx.guest_resource_lower_borrow(t, rep)) } @@ -561,7 +659,7 @@ where let (rep, dtor, flags) = cx.guest_resource_lift_own(t, index)?; assert!(dtor.is_some()); assert!(flags.is_none()); - (AtomicU32::new(NOT_IN_TABLE), rep) + (AtomicResourceState::NOT_IN_TABLE, rep) } // The borrow here is lifted from the guest, but note the lack of @@ -574,7 +672,7 @@ where InterfaceType::Borrow(t) => { debug_assert!(cx.resource_type(t) == ResourceType::host::()); let rep = cx.guest_resource_lift_borrow(t, index)?; - (AtomicU32::new(BORROW), rep) + (AtomicResourceState::BORROW, rep) } _ => bad_type_info(), }; @@ -614,11 +712,11 @@ where assert_eq!(store_id, store, "wrong store used to convert resource"); assert!(dtor.is_some(), "destructor must be set"); assert!(flags.is_none(), "flags must not be set"); - let rep = tables.host_resource_lift_own(idx, ty)?; - (AtomicU32::new(NOT_IN_TABLE), rep) + let rep = tables.host_resource_lift_own(idx)?; + (AtomicResourceState::NOT_IN_TABLE, rep) } else { - let rep = tables.host_resource_lift_borrow(idx, ty)?; - (AtomicU32::new(BORROW), rep) + let rep = tables.host_resource_lift_borrow(idx)?; + (AtomicResourceState::BORROW, rep) }; Ok(Resource { state, @@ -693,11 +791,11 @@ unsafe impl Lift for Resource { impl fmt::Debug for Resource { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let state = match self.state.load(Relaxed) { - BORROW => "borrow", - NOT_IN_TABLE => "own (not in table)", - TAKEN => "taken", - _ => "own", + let state = match self.state.get() { + ResourceState::Borrow => "borrow", + ResourceState::NotInTable => "own (not in table)", + ResourceState::Taken => "taken", + ResourceState::Index(_) => "own", }; f.debug_struct("Resource") .field("rep", &self.rep) @@ -732,7 +830,7 @@ impl fmt::Debug for Resource { /// used. #[derive(Debug, PartialEq, Eq, Copy, Clone)] pub struct ResourceAny { - idx: u32, + idx: HostResourceIndex, ty: ResourceType, own_state: Option, } @@ -779,10 +877,10 @@ impl ResourceAny { ensure!(*ty == ResourceType::host::(), "resource type mismatch"); let mut tables = HostResourceTables::new_host(store.0); - let (idx, own_state) = match state.load(Relaxed) { - BORROW => (tables.host_resource_lower_borrow(rep, *ty), None), - NOT_IN_TABLE => { - let idx = tables.host_resource_lower_own(rep, *ty); + let (idx, own_state) = match state.get() { + ResourceState::Borrow => (tables.host_resource_lower_borrow(rep), None), + ResourceState::NotInTable => { + let idx = tables.host_resource_lower_own(rep); ( idx, Some(OwnState { @@ -792,8 +890,8 @@ impl ResourceAny { }), ) } - TAKEN => bail!("host resource already consumed"), - idx => ( + ResourceState::Taken => bail!("host resource already consumed"), + ResourceState::Index(idx) => ( idx, Some(OwnState { dtor: Some(dtor_funcref.into()), @@ -871,7 +969,7 @@ impl ResourceAny { // // This could fail if the index is invalid or if this is removing an // `Own` entry which is currently being borrowed. - let rep = HostResourceTables::new_host(store.0).host_resource_drop(self.idx, self.ty)?; + let rep = HostResourceTables::new_host(store.0).host_resource_drop(self.idx)?; let (rep, state) = match (rep, &self.own_state) { (Some(rep), Some(state)) => (rep, state), @@ -925,14 +1023,14 @@ impl ResourceAny { if cx.resource_type(t) != self.ty { bail!("mismatched resource types"); } - let rep = cx.host_resource_lift_own(self.idx, self.ty)?; + let rep = cx.host_resource_lift_own(self.idx)?; Ok(cx.guest_resource_lower_own(t, rep)) } InterfaceType::Borrow(t) => { if cx.resource_type(t) != self.ty { bail!("mismatched resource types"); } - let rep = cx.host_resource_lift_borrow(self.idx, self.ty)?; + let rep = cx.host_resource_lift_borrow(self.idx)?; Ok(cx.guest_resource_lower_borrow(t, rep)) } _ => bad_type_info(), @@ -944,7 +1042,7 @@ impl ResourceAny { InterfaceType::Own(t) => { let ty = cx.resource_type(t); let (rep, dtor, flags) = cx.guest_resource_lift_own(t, index)?; - let idx = cx.host_resource_lower_own(rep, ty); + let idx = cx.host_resource_lower_own(rep); Ok(ResourceAny { idx, ty, @@ -958,7 +1056,7 @@ impl ResourceAny { InterfaceType::Borrow(t) => { let ty = cx.resource_type(t); let rep = cx.guest_resource_lift_borrow(t, index)?; - let idx = cx.host_resource_lower_borrow(rep, ty); + let idx = cx.host_resource_lower_borrow(rep); Ok(ResourceAny { idx, ty, diff --git a/crates/wasmtime/src/runtime/store.rs b/crates/wasmtime/src/runtime/store.rs index c5e0ccc87c68..383fffc2425c 100644 --- a/crates/wasmtime/src/runtime/store.rs +++ b/crates/wasmtime/src/runtime/store.rs @@ -370,7 +370,7 @@ pub struct StoreOpaque { #[cfg(feature = "component-model")] component_calls: wasmtime_runtime::component::CallContexts, #[cfg(feature = "component-model")] - host_resource_types: Vec, + host_resource_data: crate::component::HostResourceData, } #[cfg(feature = "async")] @@ -523,7 +523,7 @@ impl Store { #[cfg(feature = "component-model")] component_calls: Default::default(), #[cfg(feature = "component-model")] - host_resource_types: Vec::new(), + host_resource_data: Default::default(), }, limiter: None, call_hook: None, @@ -1636,12 +1636,12 @@ at https://bytecodealliance.org/security. ) -> ( &mut wasmtime_runtime::component::CallContexts, &mut wasmtime_runtime::component::ResourceTable, - &mut Vec, + &mut crate::component::HostResourceData, ) { ( &mut self.component_calls, &mut self.component_host_table, - &mut self.host_resource_types, + &mut self.host_resource_data, ) } From a24ad2447fa15e433f36dbb2c8c532bd407791af Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 12 Feb 2024 14:16:05 -0800 Subject: [PATCH 4/4] Review comments --- crates/wasmtime/src/runtime/component/resources.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/wasmtime/src/runtime/component/resources.rs b/crates/wasmtime/src/runtime/component/resources.rs index a8132a896a48..a2efb6a6af96 100644 --- a/crates/wasmtime/src/runtime/component/resources.rs +++ b/crates/wasmtime/src/runtime/component/resources.rs @@ -336,10 +336,11 @@ impl HostResourceIndex { } fn index(&self) -> u32 { - self.0 as u32 + u32::try_from(self.0 & 0xffffffff).unwrap() } + fn gen(&self) -> u32 { - (self.0 >> 32) as u32 + u32::try_from(self.0 >> 32).unwrap() } }