From e7713e87ece5ad4569a3901b54d328a7b4e2d485 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Sun, 23 Feb 2025 17:06:35 -0500 Subject: [PATCH] Prevent extensions from accidentally freeing value types, fix comments --- core/Cargo.toml | 2 +- core/types.rs | 4 +--- core/vdbe/mod.rs | 3 --- extensions/core/Cargo.toml | 2 +- extensions/core/src/types.rs | 36 ++++++++++++++---------------------- 5 files changed, 17 insertions(+), 30 deletions(-) diff --git a/core/Cargo.toml b/core/Cargo.toml index deb3c98cf..2c97ab2fa 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -37,7 +37,7 @@ mimalloc = { version = "0.1", default-features = false } libloading = "0.8.6" [dependencies] -limbo_ext = { workspace = true } +limbo_ext = { workspace = true, features = ["core_only"] } cfg_block = "0.1.1" fallible-iterator = "0.3.0" hex = "0.4.3" diff --git a/core/types.rs b/core/types.rs index 170ce7252..710c4c8ee 100644 --- a/core/types.rs +++ b/core/types.rs @@ -203,9 +203,7 @@ impl OwnedValue { } } }; - unsafe { - v.free(); - } + unsafe { v.__free_internal_type() }; res } } diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index e001a5e6c..e436f58f5 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -190,9 +190,6 @@ macro_rules! call_external_function { } let argv_ptr = ext_values.as_ptr(); let result_c_value: ExtValue = unsafe { ($func_ptr)($arg_count as i32, argv_ptr) }; - for arg in ext_values { - unsafe { arg.free() }; - } match OwnedValue::from_ffi(result_c_value) { Ok(result_ov) => { $state.registers[$dest_register] = result_ov; diff --git a/extensions/core/Cargo.toml b/extensions/core/Cargo.toml index ff51ca204..1389e39c1 100644 --- a/extensions/core/Cargo.toml +++ b/extensions/core/Cargo.toml @@ -8,7 +8,7 @@ repository.workspace = true description = "Limbo extensions core" [features] -default = [] +core_only = [] static = [] [dependencies] diff --git a/extensions/core/src/types.rs b/extensions/core/src/types.rs index f08fe099e..22ef47bae 100644 --- a/extensions/core/src/types.rs +++ b/extensions/core/src/types.rs @@ -108,7 +108,7 @@ impl std::fmt::Debug for Value { } #[repr(C)] -pub struct TextValue { +struct TextValue { text: *const u8, len: u32, } @@ -162,7 +162,7 @@ impl TextValue { } #[repr(C)] -pub struct ErrValue { +struct ErrValue { code: ResultCode, message: *mut TextValue, } @@ -186,16 +186,10 @@ impl ErrValue { message: Box::into_raw(Box::new(text_value)), } } - - unsafe fn free(self) { - if !self.message.is_null() { - let _ = Box::from_raw(self.message); // Freed by the same library - } - } } #[repr(C)] -pub struct Blob { +struct Blob { data: *const u8, size: u64, } @@ -229,10 +223,6 @@ impl Value { } /// Returns the value type of the Value - /// # Safety - /// This function accesses the value_type field of the union. - /// it is safe to call this function as long as the value was properly - /// constructed with one of the provided methods pub fn value_type(&self) -> ValueType { self.value_type } @@ -356,8 +346,6 @@ impl Value { } /// Creates a new text Value from a String - /// This function allocates/leaks the string - /// and must be free'd manually pub fn from_text(s: String) -> Self { let txt_value = TextValue::new_boxed(s); let ptr = Box::into_raw(txt_value); @@ -368,8 +356,6 @@ impl Value { } /// Creates a new error Value from a ResultCode - /// This function allocates/leaks the error - /// and must be free'd manually pub fn error(code: ResultCode) -> Self { let err_val = ErrValue::new(code); Self { @@ -381,7 +367,6 @@ impl Value { } /// Creates a new error Value from a ResultCode and a message - /// This function allocates/leaks the error, must be free'd manually pub fn error_with_message(message: String) -> Self { let err_value = ErrValue::new_with_message(ResultCode::CustomError, message); let err_box = Box::new(err_value); @@ -394,8 +379,6 @@ impl Value { } /// Creates a new blob Value from a Vec - /// This function allocates/leaks the blob - /// and must be free'd manually pub fn from_blob(value: Vec) -> Self { let boxed = Box::new(Blob::new(value.as_ptr(), value.len() as u64)); std::mem::forget(value); @@ -408,11 +391,18 @@ impl Value { } /// Extension authors should __not__ use this function. + /// Extensions should _not_ use this method directly. When used properly, + /// core will own all Value types and they should not need to be manually free'd + /// in any extension code. However, if you are arbitrarily creating `Value` types + /// that do not follow the intended control flow/API of the exposed traits, and are + /// not returned to `core`, your extension _will_ leak the underlying memory. + /// /// # Safety /// consumes the value while freeing the underlying memory with null check. /// however this does assume that the type was properly constructed with /// the appropriate value_type and value. - pub unsafe fn free(self) { + #[cfg(feature = "core_only")] + pub unsafe fn __free_internal_type(self) { match self.value_type { ValueType::Text => { let _ = Box::from_raw(self.value.text as *mut TextValue); @@ -422,7 +412,9 @@ impl Value { } ValueType::Error => { let err_val = Box::from_raw(self.value.error as *mut ErrValue); - err_val.free(); + if !err_val.message.is_null() { + let _ = Box::from_raw(err_val.message); + } } _ => {} }