From 4db57de98e705ea3759ad07d56dc6af054610b48 Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Mon, 25 Nov 2024 13:16:00 -0800 Subject: [PATCH] fix: remove unwrap() from extism_compiled_plugin_new `extism_compiled_plugin_new` would panic on bad manifest input (there's a test in the python sdk [1] that hits this directly.) Catch the error and set `errmsg` appropriately instead. Additionally: golf the function pointer casting for the various `plugin_new` calls, with the goal of reducing the number of allocations by leaning on iterator size hints (and reducing the scope of `unsafe{}` use.) This part can be severed: it introduces a breaking change. Previously `NULL` values were silently accepted in the `functions**` list; now they fail. This maintains consistency with behavior on "consumed" `ExtismFunction` pointer. [1]: https://github.com/extism/python-sdk/blob/main/tests/test_extism.py#L50-L53 --- runtime/src/sdk.rs | 160 +++++++++++++++++++++++++++------------------ 1 file changed, 98 insertions(+), 62 deletions(-) diff --git a/runtime/src/sdk.rs b/runtime/src/sdk.rs index 576e801..fde37cd 100644 --- a/runtime/src/sdk.rs +++ b/runtime/src/sdk.rs @@ -314,30 +314,50 @@ pub unsafe extern "C" fn extism_compiled_plugin_new( ) -> *mut CompiledPlugin { trace!("Call to extism_plugin_new with wasm pointer {:?}", wasm); let data = std::slice::from_raw_parts(wasm, wasm_size as usize); + let mut builder = PluginBuilder::new(data).with_wasi(with_wasi); if !functions.is_null() { - for i in 0..n_functions { - unsafe { - let f = *functions.add(i as usize); - if f.is_null() { - continue; + let funcs = (0..n_functions) + .map(|i| unsafe { *functions.add(i as usize) }) + .map(|ptr| { + if ptr.is_null() { + return Err("Cannot pass null pointer"); } - if let Some(f) = (*f).0.take() { - builder.options.functions.push(f); - } else { - let e = std::ffi::CString::new( - "Function cannot be registered with multiple different Plugins", - ) - .unwrap(); + + let ExtismFunction(func) = &*ptr; + let Some(func) = func.take() else { + return Err("Function cannot be registered with multiple different Plugins"); + }; + + Ok(func) + }) + .collect::, _>>() + .unwrap_or_else(|e| { + if !errmsg.is_null() { + let e = std::ffi::CString::new(e.to_string()).unwrap(); *errmsg = e.into_raw(); - return std::ptr::null_mut(); } - } + Vec::new() + }); + + if funcs.len() != n_functions as usize { + return std::ptr::null_mut(); } + + builder = builder.with_functions(funcs); } - Box::into_raw(Box::new(CompiledPlugin::new(builder).unwrap())) + CompiledPlugin::new(builder) + .map(|v| Box::into_raw(Box::new(v))) + .unwrap_or_else(|e| { + if !errmsg.is_null() { + let e = std::ffi::CString::new(format!("Unable to compile Extism plugin: {}", e)) + .unwrap(); + *errmsg = e.into_raw(); + } + std::ptr::null_mut() + }) } /// Free `ExtismCompiledPlugin` @@ -369,41 +389,49 @@ pub unsafe extern "C" fn extism_plugin_new( errmsg: *mut *mut std::ffi::c_char, ) -> *mut Plugin { let data = std::slice::from_raw_parts(wasm, wasm_size as usize); - let mut funcs = vec![]; - - if !functions.is_null() { - for i in 0..n_functions { - unsafe { - let f = *functions.add(i as usize); - if f.is_null() { - continue; + let funcs = if functions.is_null() { + vec![] + } else { + let funcs = (0..n_functions) + .map(|i| unsafe { *functions.add(i as usize) }) + .map(|ptr| { + if ptr.is_null() { + return Err("Cannot pass null pointer"); } - if let Some(f) = (*f).0.take() { - funcs.push(f); - } else { - let e = std::ffi::CString::new( - "Function cannot be registered with multiple different Plugins", - ) - .unwrap(); + + let ExtismFunction(func) = &*ptr; + let Some(func) = func.take() else { + return Err("Function cannot be registered with multiple different Plugins"); + }; + + Ok(func) + }) + .collect::, _>>() + .unwrap_or_else(|e| { + if !errmsg.is_null() { + let e = std::ffi::CString::new(e.to_string()).unwrap(); *errmsg = e.into_raw(); - return std::ptr::null_mut(); } - } - } - } + Vec::new() + }); - let plugin = Plugin::new(data, funcs, with_wasi); - match plugin { - Err(e) => { + if funcs.len() != n_functions as usize { + return std::ptr::null_mut(); + } + + funcs + }; + + Plugin::new(data, funcs, with_wasi) + .map(|v| Box::into_raw(Box::new(v))) + .unwrap_or_else(|e| { if !errmsg.is_null() { - let e = std::ffi::CString::new(format!("Unable to create Extism plugin: {}", e)) + let e = std::ffi::CString::new(format!("Unable to compile Extism plugin: {}", e)) .unwrap(); *errmsg = e.into_raw(); } std::ptr::null_mut() - } - Ok(p) => Box::into_raw(Box::new(p)), - } + }) } /// Create a new plugin from an `ExtismCompiledPlugin` @@ -442,30 +470,38 @@ pub unsafe extern "C" fn extism_plugin_new_with_fuel_limit( wasm ); let data = std::slice::from_raw_parts(wasm, wasm_size as usize); - let mut funcs = vec![]; + let funcs = if functions.is_null() { + vec![] + } else { + let funcs = (0..n_functions) + .map(|i| unsafe { *functions.add(i as usize) }) + .map(|ptr| { + if ptr.is_null() { + return Err("Cannot pass null pointer"); + } - if !functions.is_null() { - for i in 0..n_functions { - unsafe { - let f = *functions.add(i as usize); - if f.is_null() { - continue; + let ExtismFunction(func) = &*ptr; + let Some(func) = func.take() else { + return Err("Function cannot be registered with multiple different Plugins"); + }; + + Ok(func) + }) + .collect::, _>>() + .unwrap_or_else(|e| { + if !errmsg.is_null() { + let e = std::ffi::CString::new(e.to_string()).unwrap(); + *errmsg = e.into_raw(); } - if let Some(f) = (*f).0.take() { - funcs.push(f); - } else { - if !errmsg.is_null() { - let e = std::ffi::CString::new( - "Function cannot be registered with multiple different Plugins", - ) - .unwrap(); - *errmsg = e.into_raw(); - } - return std::ptr::null_mut(); - } - } + Vec::new() + }); + + if funcs.len() != n_functions as usize { + return std::ptr::null_mut(); } - } + + funcs + }; let compiled = match CompiledPlugin::new( PluginBuilder::new(data)