Remove uses of unwrap, improve library safety (#47)

* Remove uses of `unwrap`, improve library safety

* `set_array` doesn't return `Result`

Author: davidcole1340

example/skel/src/lib.rs

@@ -1,4 +1,4 @@
-use std::collections::HashMap;
+use std::{collections::HashMap, convert::TryInto};
 
 use ext_php_rs::{
     call_user_func, info_table_end, info_table_row, info_table_start, parse_args,
@@ -76,7 +76,7 @@ impl Test {
 
         let result = call_user_func!(_fn, "Hello", 5);
 
-        if let Some(r) = result {
+        if let Ok(r) = result {
             println!("{}", r.string().unwrap());
         }
 
@@ -105,37 +105,52 @@ pub extern "C" fn module_init(_type: i32, module_number: i32) -> i32 {
 
     ClassBuilder::new("TestClass")
         .method(
-            FunctionBuilder::constructor(Test::constructor).build(),
+            FunctionBuilder::constructor(Test::constructor)
+                .build()
+                .expect("could not build constructor"),
             MethodFlags::Public,
         )
         .method(
-            FunctionBuilder::new("set", Test::set).build(),
+            FunctionBuilder::new("set", Test::set)
+                .build()
+                .expect("could not build set"),
             MethodFlags::Public,
         )
         .method(
-            FunctionBuilder::new("get", Test::get).build(),
+            FunctionBuilder::new("get", Test::get)
+                .build()
+                .expect("could not build get"),
             MethodFlags::Public,
         )
         .method(
             FunctionBuilder::new("debug", Test::debug)
                 .arg(Arg::new("val", DataType::Object))
-                .build(),
+                .build()
+                .expect("could not build debug"),
             MethodFlags::Public,
         )
         .method(
             FunctionBuilder::new("call", Test::call)
                 .arg(Arg::new("fn", DataType::Callable))
-                .build(),
+                .build()
+                .expect("could not build call"),
             MethodFlags::Public,
         )
         .property("asdf", "world", PropertyFlags::Public)
+        .expect("failed to add asdf property")
         .property("jhki", 12345, PropertyFlags::Public)
+        .expect("failed to add jhki property")
         .constant("TEST", "Hello world")
+        .expect("failed to add test constant")
         .object_override::<Test>()
-        .build();
+        .build()
+        .expect("failed to build TestClass");
 
-    "Test constant".register_constant("SKEL_TEST_CONST", module_number);
-    1234.register_constant("SKEL_TEST_LONG_CONST", module_number);
+    "Test constant"
+        .register_constant("SKEL_TEST_CONST", module_number)
+        .expect("failed to add skel test const");
+    1234.register_constant("SKEL_TEST_LONG_CONST", module_number)
+        .expect("failed to add skel test long const");
 
     0
 }
@@ -148,20 +163,24 @@ pub extern "C" fn get_module() -> *mut ext_php_rs::php::module::ModuleEntry {
         .not_required()
         .arg(Arg::new("c", DataType::Double))
         .returns(DataType::String, false, false)
-        .build();
+        .build()
+        .expect("failed to build sheleton_version");
 
     let array = FunctionBuilder::new("skel_array", skeleton_array)
         .arg(Arg::new("arr", DataType::Array))
-        .build();
+        .build()
+        .expect("failed to build skel_array");
 
     let t = FunctionBuilder::new("test_array", test_array)
         .returns(DataType::Array, false, false)
-        .build();
+        .build()
+        .expect("failed to build test_array");
 
     let iter = FunctionBuilder::new("skel_unpack", skel_unpack)
         .arg(Arg::new("arr", DataType::String))
         .returns(DataType::String, false, false)
-        .build();
+        .build()
+        .expect("failed to build skel_unpack");
 
     ModuleBuilder::new("ext-skel", "0.1.0")
         .info_function(php_module_info)
@@ -171,6 +190,7 @@ pub extern "C" fn get_module() -> *mut ext_php_rs::php::module::ModuleEntry {
         .function(t)
         .function(iter)
         .build()
+        .expect("failed to build module")
         .into_raw()
 }
 
@@ -182,7 +202,7 @@ pub extern "C" fn skeleton_version(execute_data: &mut ExecutionData, retval: &mu
 
     parse_args!(execute_data, x, y; z);
     dbg!(x);
-    retval.set_string("Hello");
+    retval.set_string("Hello", false);
 }
 
 #[no_mangle]
@@ -201,13 +221,18 @@ pub extern "C" fn skeleton_array(execute_data: &mut ExecutionData, _retval: &mut
     }
 
     let mut new = ZendHashTable::new();
-    new.insert("Hello", "WOrld");
+    new.insert("Hello", "WOrld")
+        .expect("couldnt insert into hashtable");
     let _ = _retval.set_array(new);
 }
 
 #[no_mangle]
 pub extern "C" fn test_array(_execute_data: &mut ExecutionData, retval: &mut Zval) {
-    retval.set_array(vec![1, 2, 3, 4]);
+    retval.set_array(
+        vec![1, 2, 3, 4]
+            .try_into()
+            .expect("failed to convert vec to hashtable"),
+    );
 }
 
 pub extern "C" fn skel_unpack(execute_data: &mut ExecutionData, retval: &mut Zval) {

ext-php-rs-derive/src/lib.rs

@@ -32,10 +32,12 @@ pub fn object_handler_derive(input: TokenStream) -> TokenStream {
                         #handlers = Some(::ext_php_rs::php::types::object::ZendObjectHandlers::init::<#name>());
                     }
 
+                    // The handlers unwrap can never fail - we check that it is none above.
+                    // Unwrapping the result from `new_ptr` is nessacary as C cannot handle results.
                     ::ext_php_rs::php::types::object::ZendClassObject::<#name>::new_ptr(
                         ce,
                         #handlers.unwrap()
-                    )
+                    ).expect("Failed to allocate memory for new Zend object.")
                 }
             }
         }

src/errors.rs

@@ -36,6 +36,10 @@ pub enum Error {
     InvalidPointer,
     /// The given property name does not exist.
     InvalidProperty,
+    /// The string could not be converted into a C-string due to the presence of a NUL character.
+    InvalidCString,
+    /// Could not call the given function.
+    Callable,
 }
 
 impl Display for Error {
@@ -58,6 +62,11 @@ impl Display for Error {
             Error::InvalidScope => write!(f, "Invalid scope."),
             Error::InvalidPointer => write!(f, "Invalid pointer."),
             Error::InvalidProperty => write!(f, "Property does not exist on object."),
+            Error::InvalidCString => write!(
+                f,
+                "String given contains NUL-bytes which cannot be present in a C string."
+            ),
+            Error::Callable => write!(f, "Could not call given function."),
         }
     }
 }

src/functions.rs

@@ -1,5 +1,6 @@
 //! Helper functions useful for interacting with PHP and Zend.
 
+use crate::errors::{Error, Result};
 use std::ffi::CString;
 
 /// Takes a Rust string object, converts it into a C string
@@ -14,7 +15,7 @@ use std::ffi::CString;
 /// use std::ffi::CString;
 /// use ext_php_rs::functions::c_str;
 ///
-/// let mut ptr = c_str("Hello");
+/// let mut ptr = c_str("Hello").unwrap();
 ///
 /// unsafe {
 ///     assert_eq!(b'H', *ptr as u8);
@@ -28,9 +29,16 @@ use std::ffi::CString;
 ///     let _ = CString::from_raw(ptr as *mut i8);
 /// }
 /// ```
-pub fn c_str<S>(s: S) -> *const i8
+///
+/// # Errors
+///
+/// Returns an error if the given string contains a NUL byte, which for obvious reasons cannot be
+/// contained inside a C string.
+pub fn c_str<S>(s: S) -> Result<*const i8>
 where
     S: AsRef<str>,
 {
-    CString::into_raw(CString::new(s.as_ref()).unwrap())
+    Ok(CString::into_raw(
+        CString::new(s.as_ref()).map_err(|_| Error::InvalidCString)?,
+    ))
 }

src/lib.rs

@@ -1,3 +1,4 @@
+#![deny(clippy::unwrap_used)]
 #![allow(non_upper_case_globals)]
 #![allow(non_camel_case_types)]
 #![allow(non_snake_case)]

src/macros.rs

@@ -58,7 +58,7 @@ macro_rules! _info_table_row {
 #[macro_export]
 macro_rules! call_user_func {
     ($fn: expr, $($param: expr),*) => {
-        $fn.try_call(vec![$($param.into()),*])
+        $fn.try_call(vec![$(&$param),*])
     };
 }
 

src/php/args.rs

@@ -2,7 +2,11 @@
 
 use std::convert::{TryFrom, TryInto};
 
-use super::{enums::DataType, execution_data::ExecutionData, types::zval::Zval};
+use super::{
+    enums::DataType,
+    execution_data::ExecutionData,
+    types::zval::{IntoZval, Zval},
+};
 
 use crate::{
     bindings::{
@@ -97,20 +101,16 @@ impl<'a> Arg<'a> {
 
     /// Attempts to call the argument as a callable with a list of arguments to pass to the function.
     /// Note that a thrown exception inside the callable is not detectable, therefore you should
-    /// check if the return value is valid rather than unwrapping.
+    /// check if the return value is valid rather than unwrapping. Returns a result containing the
+    /// return value of the function, or an error.
     ///
     /// You should not call this function directly, rather through the [`call_user_func`] macro.
     ///
     /// # Parameters
     ///
     /// * `params` - A list of parameters to call the function with.
-    ///
-    /// # Returns
-    ///
-    /// * `Some(Zval)` - The result of the function call.
-    /// * `None` - The argument was empty, the argument was not callable or the call failed.
-    pub fn try_call(&self, params: Vec<Zval>) -> Option<Zval> {
-        self.zval()?.try_call(params)
+    pub fn try_call(&self, params: Vec<&dyn IntoZval>) -> Result<Zval> {
+        self.zval().ok_or(Error::Callable)?.try_call(params)
     }
 }
 
@@ -139,15 +139,15 @@ impl From<Arg<'_>> for _zend_expected_type {
 pub type ArgInfo = zend_internal_arg_info;
 
 /// Parses the arguments of a function.
-pub struct ArgParser<'a, 'b> {
-    args: Vec<&'a mut Arg<'b>>,
+pub struct ArgParser<'a, 'arg, 'zval> {
+    args: Vec<&'arg mut Arg<'zval>>,
     min_num_args: Option<u32>,
-    execute_data: *mut ExecutionData,
+    execute_data: &'a mut ExecutionData,
 }
 
-impl<'a, 'b> ArgParser<'a, 'b> {
+impl<'a, 'arg, 'zval> ArgParser<'a, 'arg, 'zval> {
     /// Builds a new function argument parser.
-    pub fn new(execute_data: *mut ExecutionData) -> Self {
+    pub fn new(execute_data: &'a mut ExecutionData) -> Self {
         ArgParser {
             args: vec![],
             min_num_args: None,
@@ -160,7 +160,7 @@ impl<'a, 'b> ArgParser<'a, 'b> {
     /// # Parameters
     ///
     /// * `arg` - The argument to add to the parser.
-    pub fn arg(mut self, arg: &'a mut Arg<'b>) -> Self {
+    pub fn arg(mut self, arg: &'arg mut Arg<'zval>) -> Self {
         self.args.push(arg);
         self
     }
@@ -172,35 +172,36 @@ impl<'a, 'b> ArgParser<'a, 'b> {
     }
 
     /// Uses the argument parser to parse the arguments contained in the given
-    /// `ExecutionData` object.
+    /// `ExecutionData` object. Returns successfully if the arguments were parsed.
+    ///
+    /// This function can only be safely called from within an exported PHP function.
     ///
     /// # Parameters
     ///
     /// * `execute_data` - The execution data from the function.
     ///
-    /// # Returns
+    /// # Errors
     ///
-    /// * `Ok(())` - The arguments were successfully parsed.
-    /// * `Err(String)` - There were too many or too little arguments
-    /// passed to the function. The user has already been notified so you
-    /// can discard and return from the function if an `Err` is received.
+    /// Returns an [`Error`] type if there were too many or too little arguments passed to the
+    /// function. The user has already been notified so you should break execution after seeing an
+    /// error type.
     pub fn parse(mut self) -> Result<()> {
-        let execute_data = unsafe { self.execute_data.as_ref() }.unwrap();
-        let num_args = unsafe { execute_data.This.u2.num_args };
+        let num_args = unsafe { self.execute_data.This.u2.num_args };
         let max_num_args = self.args.len() as u32;
         let min_num_args = match self.min_num_args {
             Some(n) => n,
             None => max_num_args,
         };
 
         if num_args < min_num_args || num_args > max_num_args {
+            // SAFETY: Exported C function is safe, return value is unused and parameters are copied.
             unsafe { zend_wrong_parameters_count_error(min_num_args, max_num_args) };
 
             return Err(Error::IncorrectArguments(num_args, min_num_args));
         }
 
         for (i, arg) in self.args.iter_mut().enumerate() {
-            arg.zval = unsafe { execute_data.zend_call_arg(i) };
+            arg.zval = unsafe { self.execute_data.zend_call_arg(i) };
         }
 
         Ok(())