Merge #1809

1809: fix(c-api) Remove memory leaks and address `TODO`s r=Hywan a=Hywan

~Build on top of #1795. The real diff can be seen here until the base branch is merged: Hywan/wasmer@test-c-api-inline-c-rs...Hywan:fix-c-api-memory-leaks-externs.~

This PR does several things while reviewing the entire C API. It:

* Removes several memory leaks (6 if I count correctly),
* Addresses many `TODO`s,
* Write a `DEVELOPMENT.md` document to uniformize the Rust patterns to handle C behaviors,
* Apply what is written in `DEVELOPMENT.md`,
* Allow more functions to handle null pointers,
* Remove all `rustc` warnings.

Co-authored-by: Ivan Enderlin <[email protected]>

Author: bors[bot]

Cargo.lock

@@ -0,0 +1,132 @@
+# Development Notes
+
+## Ownerships
+
+The [`wasm.h`] header thankfully defines the [`own`] “annotation”. It
+specifies _who_ owns _what_. For example, in the following code:
+
+```c
+WASM_API_EXTERN own wasm_importtype_t* wasm_importtype_new(
+  own wasm_name_t* module, own wasm_name_t* name, own wasm_externtype_t*);
+```
+
+We must read that `wasm_importtype_new` takes the ownership of all its
+three arguments. This function is then responsible to free those
+data. We must also read that the returned value is owned by the caller
+of this function.
+
+### Rust Pattern
+
+This ownership property translates well in Rust. We have decided to
+use the `Box<T>` type to represent an owned pointer. `Box<T>` drops
+its content when it's dropped.
+
+Consequently, apart from other patterns, the code above can be written
+as follows in Rust:
+
+```rust
+#[no_mangle]
+pub extern "C" fn wasm_importtype_new(
+    module: Box<wasm_name_t>,
+    name: Box<wasm_name_t>,
+    extern_type: Box<wasm_externtype_t>,
+) -> Box<wasm_importtype_t> {
+    …
+}
+```
+
+By reading the code, it is clear that `wasm_importtype_new` takes the
+ownership for `module`, `name`, and `extern_type`, and that the result
+is owned by the caller.
+
+## `const *T`
+
+A constant pointer can be interpreted in C as an immutable
+pointer. Without the [`own`] annotation, it means the ownership is not
+transfered anywhere (see [the Ownerships Section][#ownerships]).
+
+### Rust Pattern
+
+`const *T` translates to Rust as `&T`, it's a reference.
+
+## Null Pointer
+
+The [`wasm.h`] header does not say anything about null pointer. The
+behavior we agreed on in that passing a null pointer where it is not
+expected (i.e. everywhere) will make the function to return null too
+with no error.
+
+### Rust Pattern
+
+A nice type property in Rust is that it is possible to write
+`Option<NonNull<T>>` to nicely handle null pointer of kind `T`. For an
+argument, it translates as follows:
+
+* When the given pointer is null, the argument holds `None`,
+* When the given pointer is not null, the arguments holds
+  `Some(NonNull<T>)`.
+
+Considering [the Ownerships Section][#ownerships], if the pointer is
+owned, we can also write `Option<Box<T>>`. This pattern is largely
+used in this codebase to represent a “nullable” owned
+pointer. Consequently, a code like:
+
+```c
+WASM_API_EXTERN own wasm_importtype_t* wasm_importtype_new(
+  own wasm_name_t* module, own wasm_name_t* name, own wasm_externtype_t*);
+```
+
+translates into Rust as:
+
+```rust
+#[no_mangle]
+pub extern "C" fn wasm_importtype_new(
+    module: Option<Box<wasm_name_t>>,
+    name: Option<Box<wasm_name_t>>,
+    extern_type: Option<Box<wasm_externtype_t>>,
+) -> Option<Box<wasm_importtype_t>> {
+    Some(Box::new(wasm_importtype_t {
+        name: name?,
+        module: module?,
+        extern_type: extern_type?,
+    }))
+}
+```
+
+What `name?` (and others) means? It is basically [the `Try` trait
+implemented for
+`Option`](https://doc.rust-lang.org/std/ops/trait.Try.html#impl-Try):
+It returns `None` if the value is `None`, otherwise it unwraps the
+`Option`.
+
+Because the function returns `Option<Box<T>>`, `None` represents a
+null pointer.
+
+Considering [the `const *T` Section][#const-t], if the pointer is not
+owned, we can either write `Option<NonNull<T>>` or `Option<&T>`. It
+has been decided to use the second pattern in all the codebase.
+
+## Destructors
+
+The [`wasm.h`] defines `wasm_*_delete` functions. It represents destructors.
+
+## Rust Pattern
+
+The destructors in Rust translate as follow:
+
+```rust
+#[no_mangle]
+pub unsafe extern "C" fn wasm_*_delete(_: Option<Box<wasm_*_t>>) {}
+```
+
+`Box<T>` will take the ownership of the value. It means that Rust will
+drop it automatically as soon as it goes out of the
+scope. Consequently, the “C destructors” really are the “Rust
+destructors”.
+
+The `Option` is here to handle the situation where a null pointer is
+passed to the destructor.
+
+
+[`own`](https://github.com/wasmerio/wasmer/blob/f548f268f2335693b97ad7ca08af72c320daf59a/lib/c-api/tests/wasm_c_api/wasm-c-api/include/wasm.h#L46-L65)
+[`wasm.h`](https://github.com/wasmerio/wasmer/blob/f548f268f2335693b97ad7ca08af72c320daf59a/lib/c-api/tests/wasm_c_api/wasm-c-api/include/wasm.h)

lib/c-api/src/wasm_c_api/DEVELOPMENT.md

@@ -1,3 +1,4 @@
+use crate::error::{update_last_error, CApiError};
 use cfg_if::cfg_if;
 use std::sync::Arc;
 use wasmer::Engine;
@@ -182,14 +183,24 @@ cfg_if! {
 
 /// cbindgen:ignore
 #[no_mangle]
-pub unsafe extern "C" fn wasm_engine_delete(_wasm_engine_address: Option<Box<wasm_engine_t>>) {}
+pub unsafe extern "C" fn wasm_engine_delete(_engine: Option<Box<wasm_engine_t>>) {}
 
 /// cbindgen:ignore
 #[no_mangle]
 pub extern "C" fn wasm_engine_new_with_config(
     config: Box<wasm_config_t>,
 ) -> Option<Box<wasm_engine_t>> {
-    // TODO: return useful error messages in failure branches
+    fn return_with_error<M>(msg: M) -> Option<Box<wasm_engine_t>>
+    where
+        M: ToString,
+    {
+        update_last_error(CApiError {
+            msg: msg.to_string(),
+        });
+
+        return None;
+    };
+
     cfg_if! {
         if #[cfg(feature = "compiler")] {
             #[allow(unused_mut)]
@@ -199,7 +210,7 @@ pub extern "C" fn wasm_engine_new_with_config(
                         if #[cfg(feature = "cranelift")] {
                             Box::new(wasmer_compiler_cranelift::Cranelift::default())
                         } else {
-                            return None;
+                            return return_with_error("Wasmer has not been compiled with the `cranelift` feature.");
                         }
                     }
                 },
@@ -208,7 +219,7 @@ pub extern "C" fn wasm_engine_new_with_config(
                         if #[cfg(feature = "llvm")] {
                             Box::new(wasmer_compiler_llvm::LLVM::default())
                         } else {
-                            return None;
+                            return return_with_error("Wasmer has not been compiled with the `llvm` feature.");
                         }
                     }
                 },
@@ -217,7 +228,7 @@ pub extern "C" fn wasm_engine_new_with_config(
                         if #[cfg(feature = "singlepass")] {
                             Box::new(wasmer_compiler_singlepass::Singlepass::default())
                         } else {
-                            return None;
+                            return return_with_error("Wasmer has not been compiled with the `singlepass` feature.");
                         }
                     }
                 },
@@ -229,7 +240,7 @@ pub extern "C" fn wasm_engine_new_with_config(
                         if #[cfg(feature = "jit")] {
                             Arc::new(JIT::new(&*compiler_config).engine())
                         } else {
-                            return None;
+                            return return_with_error("Wasmer has not been compiled with the `jit` feature.");
                         }
                     }
                 },
@@ -238,7 +249,7 @@ pub extern "C" fn wasm_engine_new_with_config(
                         if #[cfg(feature = "native")] {
                             Arc::new(Native::new(&mut *compiler_config).engine())
                         } else {
-                            return None;
+                            return return_with_error("Wasmer has not been compiled with the `native` feature.");
                         }
                     }
                 },
@@ -249,7 +260,7 @@ pub extern "C" fn wasm_engine_new_with_config(
                         if #[cfg(feature = "object-file")] {
                             Arc::new(ObjectFile::headless().engine())
                         } else {
-                            return None;
+                            return return_with_error("Wasmer has not been compiled with the `object-file` feature.");
                         }
                     }
                 },
@@ -262,7 +273,7 @@ pub extern "C" fn wasm_engine_new_with_config(
                         if #[cfg(feature = "jit")] {
                             Arc::new(JIT::headless().engine())
                         } else {
-                            return None;
+                            return return_with_error("Wasmer has not been compiled with the `jit` feature.");
                         }
                     }
                 },
@@ -271,7 +282,7 @@ pub extern "C" fn wasm_engine_new_with_config(
                         if #[cfg(feature = "native")] {
                             Arc::new(Native::headless().engine())
                         } else {
-                            return None;
+                            return return_with_error("Wasmer has not been compiled with the `native` feature.");
                         }
                     }
                 },
@@ -280,7 +291,7 @@ pub extern "C" fn wasm_engine_new_with_config(
                         if #[cfg(feature = "object-file")] {
                             Arc::new(ObjectFile::headless().engine())
                         } else {
-                            return None;
+                            return return_with_error("Wasmer has not been compiled with the `object-file` feature.");
                         }
                     }
                 },

lib/c-api/src/wasm_c_api/engine.rs

@@ -7,6 +7,7 @@ use std::ffi::c_void;
 use std::sync::Arc;
 use wasmer::{Function, Instance, RuntimeError, Val};
 
+#[derive(Debug)]
 #[allow(non_camel_case_types)]
 pub struct wasm_func_t {
     pub(crate) inner: Function,
@@ -32,11 +33,14 @@ pub type wasm_env_finalizer_t = unsafe extern "C" fn(c_void);
 
 #[no_mangle]
 pub unsafe extern "C" fn wasm_func_new(
-    store: &wasm_store_t,
-    function_type: &wasm_functype_t,
-    callback: wasm_func_callback_t,
+    store: Option<&wasm_store_t>,
+    function_type: Option<&wasm_functype_t>,
+    callback: Option<wasm_func_callback_t>,
 ) -> Option<Box<wasm_func_t>> {
-    // TODO: handle null pointers?
+    let store = store?;
+    let function_type = function_type?;
+    let callback = callback?;
+
     let func_sig = &function_type.inner().function_type;
     let num_rets = func_sig.results().len();
     let inner_callback = move |args: &[Val]| -> Result<Vec<Val>, RuntimeError> {
@@ -84,13 +88,16 @@ pub unsafe extern "C" fn wasm_func_new(
 
 #[no_mangle]
 pub unsafe extern "C" fn wasm_func_new_with_env(
-    store: &wasm_store_t,
-    function_type: &wasm_functype_t,
-    callback: wasm_func_callback_with_env_t,
+    store: Option<&wasm_store_t>,
+    function_type: Option<&wasm_functype_t>,
+    callback: Option<wasm_func_callback_with_env_t>,
     env: *mut c_void,
     finalizer: wasm_env_finalizer_t,
 ) -> Option<Box<wasm_func_t>> {
-    // TODO: handle null pointers?
+    let store = store?;
+    let function_type = function_type?;
+    let callback = callback?;
+
     let func_sig = &function_type.inner().function_type;
     let num_rets = func_sig.results().len();
     let inner_callback =
@@ -143,10 +150,13 @@ pub unsafe extern "C" fn wasm_func_delete(_func: Option<Box<wasm_func_t>>) {}
 
 #[no_mangle]
 pub unsafe extern "C" fn wasm_func_call(
-    func: &wasm_func_t,
-    args: &wasm_val_vec_t,
+    func: Option<&wasm_func_t>,
+    args: Option<&wasm_val_vec_t>,
     results: &mut wasm_val_vec_t,
 ) -> Option<Box<wasm_trap_t>> {
+    let func = func?;
+    let args = args?;
+
     let params = args
         .into_slice()
         .map(|slice| {
@@ -200,6 +210,8 @@ pub unsafe extern "C" fn wasm_func_result_arity(func: &wasm_func_t) -> usize {
 }
 
 #[no_mangle]
-pub extern "C" fn wasm_func_type(func: &wasm_func_t) -> Box<wasm_functype_t> {
-    Box::new(wasm_functype_t::new(func.inner.ty().clone()))
+pub extern "C" fn wasm_func_type(func: Option<&wasm_func_t>) -> Option<Box<wasm_functype_t>> {
+    let func = func?;
+
+    Some(Box::new(wasm_functype_t::new(func.inner.ty().clone())))
 }