From 35fb0638f4f56238d6a8f553ec63ffbf9ab16d35 Mon Sep 17 00:00:00 2001 From: makspll Date: Sat, 22 Feb 2025 22:03:32 +0000 Subject: [PATCH] fix: functions not releasing accesses correctly on error --- .../src/bindings/function/script_function.rs | 88 ++++++++++++------- .../src/bindings/world.rs | 18 +++- 2 files changed, 72 insertions(+), 34 deletions(-) diff --git a/crates/bevy_mod_scripting_core/src/bindings/function/script_function.rs b/crates/bevy_mod_scripting_core/src/bindings/function/script_function.rs index 3249a40d..47481263 100644 --- a/crates/bevy_mod_scripting_core/src/bindings/function/script_function.rs +++ b/crates/bevy_mod_scripting_core/src/bindings/function/script_function.rs @@ -592,39 +592,42 @@ macro_rules! impl_script_function { $( let $context = caller_context; )? let world = caller_context.world()?; - world.begin_access_scope()?; - let mut current_arg = 0; - - $( - current_arg += 1; - let $param = args.pop_front(); - let $param = match $param { - Some($param) => $param, - None => { - if let Some(default) = <$param>::default_value() { - default - } else { - return Err(InteropError::function_call_error(FunctionError::ArgCountMismatch{ - expected: expected_arg_count, - received: received_args_len - })); - } - } - }; - let $param = <$param>::from_script($param, world.clone()) - .map_err(|e| InteropError::function_arg_conversion_error(current_arg.to_string(), e))?; - )* - - let ret = { - let out = self( $( $context,)? $( $param.into(), )* ); - $( - let $out = out?; - let out = $out; - )? - out.into_script(world.clone()).map_err(|e| InteropError::function_arg_conversion_error("return value".to_owned(), e)) + // Safety: we're not holding any references to the world, the arguments which might have aliased will always be dropped + let ret: Result = unsafe { + world.with_access_scope(||{ + let mut current_arg = 0; + + $( + current_arg += 1; + let $param = args.pop_front(); + let $param = match $param { + Some($param) => $param, + None => { + if let Some(default) = <$param>::default_value() { + default + } else { + return Err(InteropError::function_call_error(FunctionError::ArgCountMismatch{ + expected: expected_arg_count, + received: received_args_len + })); + } + } + }; + let $param = <$param>::from_script($param, world.clone()) + .map_err(|e| InteropError::function_arg_conversion_error(current_arg.to_string(), e))?; + )* + + let ret = { + let out = self( $( $context,)? $( $param.into(), )* ); + $( + let $out = out?; + let out = $out; + )? + out.into_script(world.clone()).map_err(|e| InteropError::function_arg_conversion_error("return value".to_owned(), e)) + }; + ret + })? }; - // Safety: we're not holding any references to the world, the arguments which might have aliased have been dropped - unsafe { world.end_access_scope()? }; ret })(); let script_value: ScriptValue = res.into(); @@ -704,6 +707,27 @@ mod test { }); } + #[test] + fn test_interrupted_call_releases_access_scope() { + #[derive(bevy::prelude::Component, Reflect)] + struct Comp; + + let fn_ = |_a: crate::bindings::function::from::Mut| 0usize; + let script_function = fn_.into_dynamic_script_function().with_name("my_fn"); + + with_local_world(|| { + let out = script_function.call( + vec![ScriptValue::from(1), ScriptValue::from(2)], + FunctionCallContext::default(), + ); + + assert!(out.is_err()); + let world = FunctionCallContext::default().world().unwrap(); + // assert no access is held + assert!(world.list_accesses().is_empty()); + }); + } + #[test] fn test_overloaded_script_function() { let mut registry = ScriptFunctionRegistry::default(); diff --git a/crates/bevy_mod_scripting_core/src/bindings/world.rs b/crates/bevy_mod_scripting_core/src/bindings/world.rs index 9e4c11ab..cab2369b 100644 --- a/crates/bevy_mod_scripting_core/src/bindings/world.rs +++ b/crates/bevy_mod_scripting_core/src/bindings/world.rs @@ -123,8 +123,22 @@ impl<'w> WorldAccessGuard<'w> { self.0.cell.replace(None); } + /// Runs a closure within an isolated access scope, releasing leftover accesses, should only be used in a single-threaded context. + /// + /// Safety: + /// - The caller must ensure it's safe to release any potentially locked accesses. + pub(crate) unsafe fn with_access_scope O>( + &self, + f: F, + ) -> Result { + self.begin_access_scope()?; + let o = f(); + unsafe { self.end_access_scope()? }; + Ok(o) + } + /// Begins a new access scope. Currently this simply throws an erorr if there are any accesses held. Should only be used in a single-threaded context - pub(crate) fn begin_access_scope(&self) -> Result<(), InteropError> { + fn begin_access_scope(&self) -> Result<(), InteropError> { if self.0.accesses.count_accesses() != 0 { return Err(InteropError::invalid_access_count(self.0.accesses.count_accesses(), 0, "When beginning access scope, presumably for a function call, some accesses are still held".to_owned())); } @@ -133,7 +147,7 @@ impl<'w> WorldAccessGuard<'w> { } /// Ends the access scope, releasing all accesses. Should only be used in a single-threaded context - pub(crate) unsafe fn end_access_scope(&self) -> Result<(), InteropError> { + unsafe fn end_access_scope(&self) -> Result<(), InteropError> { self.0.accesses.release_all_accesses(); Ok(())