diff --git a/ext/ffi/lib.rs b/ext/ffi/lib.rs index d42dddc0a4afc0..c38788d30f8ae8 100644 --- a/ext/ffi/lib.rs +++ b/ext/ffi/lib.rs @@ -1,7 +1,9 @@ // Copyright 2021 the Deno authors. All rights reserved. MIT license. use deno_core::error::bad_resource_id; +use deno_core::error::generic_error; use deno_core::error::range_error; +use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::include_js_files; use deno_core::op_async; @@ -79,7 +81,17 @@ impl DynamicLibraryResource { symbol: String, foreign_fn: ForeignFunction, ) -> Result<(), AnyError> { - let fn_ptr = unsafe { self.lib.symbol::<*const c_void>(&symbol) }?; + // By default, Err returned by this function does not tell + // which symbol wasn't exported. So we'll modify the error + // message to include the name of symbol. + let fn_ptr = match unsafe { self.lib.symbol::<*const c_void>(&symbol) } { + Ok(value) => Ok(value), + Err(err) => Err(generic_error(format!( + "Failed to register symbol {}: {}", + symbol, + err.to_string() + ))), + }?; let ptr = libffi::middle::CodePtr::from_ptr(fn_ptr as _); let cif = libffi::middle::Cif::new( foreign_fn @@ -194,44 +206,44 @@ union NativeValue { } impl NativeValue { - fn new(native_type: NativeType, value: Value) -> Self { - match native_type { + fn new(native_type: NativeType, value: Value) -> Result { + let value = match native_type { NativeType::Void => Self { void_value: () }, NativeType::U8 => Self { - u8_value: value_as_uint::(value), + u8_value: value_as_uint::(value)?, }, NativeType::I8 => Self { - i8_value: value_as_int::(value), + i8_value: value_as_int::(value)?, }, NativeType::U16 => Self { - u16_value: value_as_uint::(value), + u16_value: value_as_uint::(value)?, }, NativeType::I16 => Self { - i16_value: value_as_int::(value), + i16_value: value_as_int::(value)?, }, NativeType::U32 => Self { - u32_value: value_as_uint::(value), + u32_value: value_as_uint::(value)?, }, NativeType::I32 => Self { - i32_value: value_as_int::(value), + i32_value: value_as_int::(value)?, }, NativeType::U64 => Self { - u64_value: value_as_uint::(value), + u64_value: value_as_uint::(value)?, }, NativeType::I64 => Self { - i64_value: value_as_int::(value), + i64_value: value_as_int::(value)?, }, NativeType::USize => Self { - usize_value: value_as_uint::(value), + usize_value: value_as_uint::(value)?, }, NativeType::ISize => Self { - isize_value: value_as_int::(value), + isize_value: value_as_int::(value)?, }, NativeType::F32 => Self { - f32_value: value_as_f32(value), + f32_value: value_as_f32(value)?, }, NativeType::F64 => Self { - f64_value: value_as_f64(value), + f64_value: value_as_f64(value)?, }, NativeType::Pointer => { if value.is_null() { @@ -240,14 +252,13 @@ impl NativeValue { } } else { Self { - pointer: u64::from( - serde_json::from_value::(value) - .expect("Expected ffi arg value to be a tuple of the low and high bits of a pointer address") - ) as *const u8, + pointer: u64::from(serde_json::from_value::(value)?) + as *const u8, } } } - } + }; + Ok(value) } fn buffer(ptr: *const u8) -> Self { @@ -274,28 +285,38 @@ impl NativeValue { } } -fn value_as_uint>(value: Value) -> T { - value - .as_u64() - .and_then(|v| T::try_from(v).ok()) - .expect("Expected ffi arg value to be an unsigned integer") +fn value_as_uint>(value: Value) -> Result { + match value.as_u64().and_then(|v| T::try_from(v).ok()) { + Some(value) => Ok(value), + None => Err(type_error(format!( + "Expected FFI argument to be an unsigned integer, but got {:?}", + value + ))), + } } -fn value_as_int>(value: Value) -> T { - value - .as_i64() - .and_then(|v| T::try_from(v).ok()) - .expect("Expected ffi arg value to be a signed integer") +fn value_as_int>(value: Value) -> Result { + match value.as_i64().and_then(|v| T::try_from(v).ok()) { + Some(value) => Ok(value), + None => Err(type_error(format!( + "Expected FFI argument to be a signed integer, but got {:?}", + value + ))), + } } -fn value_as_f32(value: Value) -> f32 { - value_as_f64(value) as f32 +fn value_as_f32(value: Value) -> Result { + Ok(value_as_f64(value)? as f32) } -fn value_as_f64(value: Value) -> f64 { - value - .as_f64() - .expect("Expected ffi arg value to be a float") +fn value_as_f64(value: Value) -> Result { + match value.as_f64() { + Some(value) => Ok(value), + None => Err(type_error(format!( + "Expected FFI argument to be a double, but got {:?}", + value + ))), + } } #[derive(Serialize, Deserialize, Debug)] @@ -450,22 +471,35 @@ fn ffi_call(args: FfiCallArgs, symbol: &Symbol) -> Result { .map(|buffer| buffer.as_ref().map(|buffer| &buffer[..])) .collect(); - let native_values = symbol + let mut native_values: Vec = vec![]; + + for (&native_type, value) in symbol .parameter_types .iter() .zip(args.parameters.into_iter()) - .map(|(&native_type, value)| { - if let NativeType::Pointer = native_type { - if let Some(idx) = value.as_u64() { - if let Some(&Some(buf)) = buffers.get(idx as usize) { - return NativeValue::buffer(buf.as_ptr()); - } + { + match native_type { + NativeType::Pointer => match value.as_u64() { + Some(idx) => { + let buf = buffers + .get(idx as usize) + .ok_or_else(|| { + generic_error(format!("No buffer present at index {}", idx)) + })? + .unwrap(); + native_values.push(NativeValue::buffer(buf.as_ptr())); } + _ => { + let value = NativeValue::new(native_type, value)?; + native_values.push(value); + } + }, + _ => { + let value = NativeValue::new(native_type, value)?; + native_values.push(value); } - - NativeValue::new(native_type, value) - }) - .collect::>(); + } + } let call_args = symbol .parameter_types diff --git a/test_ffi/tests/integration_tests.rs b/test_ffi/tests/integration_tests.rs index 99a17f0a901e3f..13c4e148547f87 100644 --- a/test_ffi/tests/integration_tests.rs +++ b/test_ffi/tests/integration_tests.rs @@ -24,6 +24,7 @@ fn basic() { .arg("--allow-ffi") .arg("--allow-read") .arg("--unstable") + .arg("--quiet") .arg("tests/test.js") .env("NO_COLOR", "1") .output() @@ -37,7 +38,6 @@ fn basic() { println!("{:?}", output.status); assert!(output.status.success()); let expected = "\ - dlopen doesn't panic\n\ something\n\ [1, 2, 3, 4, 5, 6, 7, 8]\n\ [1, 2, 3, 4, 5, 6, 7, 8] [9, 10]\n\ diff --git a/test_ffi/tests/test.js b/test_ffi/tests/test.js index 16e4d76b6d51d1..db760352104786 100644 --- a/test_ffi/tests/test.js +++ b/test_ffi/tests/test.js @@ -1,6 +1,8 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. // deno-lint-ignore-file +import { assertThrows } from "../../test_util/std/testing/asserts.ts"; + const targetDir = Deno.execPath().replace(/[^\/\\]+$/, ""); const [libPrefix, libSuffix] = { darwin: ["lib", "dylib"], @@ -12,11 +14,22 @@ const libPath = `${targetDir}/${libPrefix}test_ffi.${libSuffix}`; const resourcesPre = Deno.resources(); // dlopen shouldn't panic -try { +assertThrows(() => { Deno.dlopen("cli/src/main.rs", {}); -} catch (_) { - console.log("dlopen doesn't panic"); -} +}); + +assertThrows( + () => { + Deno.dlopen(libPath, { + non_existent_symbol: { + parameters: [], + result: "void", + }, + }); + }, + Error, + "Failed to register symbol non_existent_symbol", +); const dylib = Deno.dlopen(libPath, { "print_something": { parameters: [], result: "void" }, @@ -75,6 +88,20 @@ console.log(Boolean(dylib.symbols.is_null_ptr(ptr))); console.log(Boolean(dylib.symbols.is_null_ptr(null))); console.log(Boolean(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(into)))); console.log(dylib.symbols.add_u32(123, 456)); +assertThrows( + () => { + dylib.symbols.add_u32(-1, 100); + }, + TypeError, + "Expected FFI argument to be an unsigned integer, but got Number(-1)", +); +assertThrows( + () => { + dylib.symbols.add_u32(null, 100); + }, + TypeError, + "Expected FFI argument to be an unsigned integer, but got Null", +); console.log(dylib.symbols.add_i32(123, 456)); console.log(dylib.symbols.add_u64(123, 456)); console.log(dylib.symbols.add_i64(123, 456));