fix(ext/ffi): throw errors instead of panic

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<Self, AnyError> {
+    let value = match native_type {
       NativeType::Void => Self { void_value: () },
       NativeType::U8 => Self {
-        u8_value: value_as_uint::<u8>(value),
+        u8_value: value_as_uint::<u8>(value)?,
       },
       NativeType::I8 => Self {
-        i8_value: value_as_int::<i8>(value),
+        i8_value: value_as_int::<i8>(value)?,
       },
       NativeType::U16 => Self {
-        u16_value: value_as_uint::<u16>(value),
+        u16_value: value_as_uint::<u16>(value)?,
       },
       NativeType::I16 => Self {
-        i16_value: value_as_int::<i16>(value),
+        i16_value: value_as_int::<i16>(value)?,
       },
       NativeType::U32 => Self {
-        u32_value: value_as_uint::<u32>(value),
+        u32_value: value_as_uint::<u32>(value)?,
       },
       NativeType::I32 => Self {
-        i32_value: value_as_int::<i32>(value),
+        i32_value: value_as_int::<i32>(value)?,
       },
       NativeType::U64 => Self {
-        u64_value: value_as_uint::<u64>(value),
+        u64_value: value_as_uint::<u64>(value)?,
       },
       NativeType::I64 => Self {
-        i64_value: value_as_int::<i64>(value),
+        i64_value: value_as_int::<i64>(value)?,
       },
       NativeType::USize => Self {
-        usize_value: value_as_uint::<usize>(value),
+        usize_value: value_as_uint::<usize>(value)?,
       },
       NativeType::ISize => Self {
-        isize_value: value_as_int::<isize>(value),
+        isize_value: value_as_int::<isize>(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() {
@@ -247,7 +259,8 @@ impl NativeValue {
           }
         }
       }
-    }
+    };
+    Ok(value)
   }
 
   fn buffer(ptr: *const u8) -> Self {
@@ -274,28 +287,38 @@ impl NativeValue {
   }
 }
 
-fn value_as_uint<T: TryFrom<u64>>(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<T: TryFrom<u64>>(value: Value) -> Result<T, AnyError> {
+  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 {:?}",
+      value
+    ))),
+  }
 }
 
-fn value_as_int<T: TryFrom<i64>>(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<T: TryFrom<i64>>(value: Value) -> Result<T, AnyError> {
+  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<f32, AnyError> {
+  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<f64, AnyError> {
+  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 +473,30 @@ fn ffi_call(args: FfiCallArgs, symbol: &Symbol) -> Result<Value, AnyError> {
     .map(|buffer| buffer.as_ref().map(|buffer| &buffer[..]))
     .collect();
 
-  let native_values = symbol
+  let mut native_values: Vec<NativeValue> = 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).unwrap().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::<Vec<_>>();
+    }
+  }
 
   let call_args = symbol
     .parameter_types

test_ffi/tests/integration_tests.rs

@@ -38,6 +38,7 @@ fn basic() {
   assert!(output.status.success());
   let expected = "\
     dlopen doesn't panic\n\
+    error includes symbol name? true\n\
     something\n\
     [1, 2, 3, 4, 5, 6, 7, 8]\n\
     [1, 2, 3, 4, 5, 6, 7, 8] [9, 10]\n\
@@ -51,6 +52,8 @@ fn basic() {
     true\n\
     false\n\
     579\n\
+    passing negative integer in u32 argument doesn't panic\n\
+    passing null in u32 argument doesn't panic\n\
     579\n\
     579\n\
     579\n\

test_ffi/tests/test.js

@@ -18,6 +18,19 @@ try {
   console.log("dlopen doesn't panic");
 }
 
+try {
+  Deno.dlopen(libPath, {
+    non_existent_symbol: {
+      parameters: [],
+      result: "void",
+    },
+  });
+} catch (e) {
+  console.log(
+    `error includes symbol name? ${e.message.includes("non_existent_symbol")}`,
+  );
+}
+
 const dylib = Deno.dlopen(libPath, {
   "print_something": { parameters: [], result: "void" },
   "print_buffer": { parameters: ["pointer", "usize"], result: "void" },
@@ -75,6 +88,16 @@ 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));
+try {
+  dylib.symbols.add_u32(-1, 100);
+} catch (_) {
+  console.log("passing negative integer in u32 argument doesn't panic");
+}
+try {
+  dylib.symbols.add_u32(null, 100);
+} catch (_) {
+  console.log("passing null in u32 argument doesn't panic");
+}
 console.log(dylib.symbols.add_i32(123, 456));
 console.log(dylib.symbols.add_u64(123, 456));
 console.log(dylib.symbols.add_i64(123, 456));