refactor(ffi): check pointer arguments for NULL (#2624)

This changes all the extern C functions in `hyper::ffi` to check passed
pointer arguments for being `NULL` before trying to use them. Before, we
would just assume the programmer had passed a good pointer, which could
result in segmentation faults. Now:

- In debug builds, it will assert they aren't null, and so if they are,
  a message identifying the argument name will be printed and then the
  process will crash.
- In release builds, it will still check for null, but if found, it will
  return early, with a return value indicating failure if the return type
  allows (such as returning NULL, or `HYPERE_INVALID_ARG`).

Closes #2620

Author: seanmonstar

capi/examples/upload.c

@@ -153,8 +153,10 @@ static void print_informational(void *userdata, const hyper_response *resp) {
 
     printf("\nInformational (1xx): %d\n", http_status);
 
-    const hyper_buf* headers = hyper_response_headers_raw(resp);
-    write(1, hyper_buf_bytes(headers), hyper_buf_len(headers));
+    const hyper_buf *headers = hyper_response_headers_raw(resp);
+    if (headers) {
+        write(1, hyper_buf_bytes(headers), hyper_buf_len(headers));
+    }
 }
 
 typedef enum {

src/ffi/body.rs

@@ -40,11 +40,7 @@ ffi_fn! {
 ffi_fn! {
     /// Free a `hyper_body *`.
     fn hyper_body_free(body: *mut hyper_body) {
-        if body.is_null() {
-            return;
-        }
-
-        drop(unsafe { Box::from_raw(body) });
+        drop(non_null!(Box::from_raw(body) ?= ()));
     }
 }
 
@@ -61,7 +57,7 @@ ffi_fn! {
     /// However, it MUST NOT be used or freed until the related task completes.
     fn hyper_body_data(body: *mut hyper_body) -> *mut hyper_task {
         // This doesn't take ownership of the Body, so don't allow destructor
-        let mut body = ManuallyDrop::new(unsafe { Box::from_raw(body) });
+        let mut body = ManuallyDrop::new(non_null!(Box::from_raw(body) ?= ptr::null_mut()));
 
         Box::into_raw(hyper_task::boxed(async move {
             body.0.data().await.map(|res| res.map(hyper_buf))
@@ -81,11 +77,7 @@ ffi_fn! {
     ///
     /// This will consume the `hyper_body *`, you shouldn't use it anymore or free it.
     fn hyper_body_foreach(body: *mut hyper_body, func: hyper_body_foreach_callback, userdata: *mut c_void) -> *mut hyper_task {
-        if body.is_null() {
-            return ptr::null_mut();
-        }
-
-        let mut body = unsafe { Box::from_raw(body) };
+        let mut body = non_null!(Box::from_raw(body) ?= ptr::null_mut());
         let userdata = UserDataPointer(userdata);
 
         Box::into_raw(hyper_task::boxed(async move {
@@ -103,7 +95,7 @@ ffi_fn! {
 ffi_fn! {
     /// Set userdata on this body, which will be passed to callback functions.
     fn hyper_body_set_userdata(body: *mut hyper_body, userdata: *mut c_void) {
-        let b = unsafe { &mut *body };
+        let b = non_null!(&mut *body ?= ());
         b.0.as_ffi_mut().userdata = userdata;
     }
 }
@@ -129,7 +121,7 @@ ffi_fn! {
     /// If some error has occurred, you can return `HYPER_POLL_ERROR` to abort
     /// the body.
     fn hyper_body_set_data_func(body: *mut hyper_body, func: hyper_body_data_callback) {
-        let b = unsafe { &mut *body };
+        let b = non_null!{ &mut *body ?= () };
         b.0.as_ffi_mut().data_func = func;
     }
 }

src/ffi/client.rs

@@ -1,3 +1,4 @@
+use std::ptr;
 use std::sync::Arc;
 
 use libc::c_int;
@@ -37,15 +38,8 @@ ffi_fn! {
     /// The returned `hyper_task *` must be polled with an executor until the
     /// handshake completes, at which point the value can be taken.
     fn hyper_clientconn_handshake(io: *mut hyper_io, options: *mut hyper_clientconn_options) -> *mut hyper_task {
-        if io.is_null() {
-            return std::ptr::null_mut();
-        }
-        if options.is_null() {
-            return std::ptr::null_mut();
-        }
-
-        let options = unsafe { Box::from_raw(options) };
-        let io = unsafe { Box::from_raw(io) };
+        let options = non_null! { Box::from_raw(options) ?= ptr::null_mut() };
+        let io = non_null! { Box::from_raw(io) ?= ptr::null_mut() };
 
         Box::into_raw(hyper_task::boxed(async move {
             options.builder.handshake::<_, crate::Body>(io)
@@ -66,19 +60,12 @@ ffi_fn! {
     /// Returns a task that needs to be polled until it is ready. When ready, the
     /// task yields a `hyper_response *`.
     fn hyper_clientconn_send(conn: *mut hyper_clientconn, req: *mut hyper_request) -> *mut hyper_task {
-        if conn.is_null() {
-            return std::ptr::null_mut();
-        }
-        if req.is_null() {
-            return std::ptr::null_mut();
-        }
-
-        let mut req = unsafe { Box::from_raw(req) };
+        let mut req = non_null! { Box::from_raw(req) ?= ptr::null_mut() };
 
         // Update request with original-case map of headers
         req.finalize_request();
 
-        let fut = unsafe { &mut *conn }.tx.send_request(req.0);
+        let fut = non_null! { &mut *conn ?= ptr::null_mut() }.tx.send_request(req.0);
 
         let fut = async move {
             fut.await.map(hyper_response::wrap)
@@ -91,7 +78,7 @@ ffi_fn! {
 ffi_fn! {
     /// Free a `hyper_clientconn *`.
     fn hyper_clientconn_free(conn: *mut hyper_clientconn) {
-        drop(unsafe { Box::from_raw(conn) });
+        drop(non_null! { Box::from_raw(conn) ?= () });
     }
 }
 
@@ -119,7 +106,7 @@ ffi_fn! {
 ffi_fn! {
     /// Free a `hyper_clientconn_options *`.
     fn hyper_clientconn_options_free(opts: *mut hyper_clientconn_options) {
-        drop(unsafe { Box::from_raw(opts) });
+        drop(non_null! { Box::from_raw(opts) ?= () });
     }
 }
 
@@ -128,9 +115,9 @@ ffi_fn! {
     ///
     /// This does not consume the `options` or the `exec`.
     fn hyper_clientconn_options_exec(opts: *mut hyper_clientconn_options, exec: *const hyper_executor) {
-        let opts = unsafe { &mut *opts };
+        let opts = non_null! { &mut *opts ?= () };
 
-        let exec = unsafe { Arc::from_raw(exec) };
+        let exec = non_null! { Arc::from_raw(exec) ?= () };
         let weak_exec = hyper_executor::downgrade(&exec);
         std::mem::forget(exec);
 
@@ -146,7 +133,7 @@ ffi_fn! {
     fn hyper_clientconn_options_http2(opts: *mut hyper_clientconn_options, enabled: c_int) -> hyper_code {
         #[cfg(feature = "http2")]
         {
-            let opts = unsafe { &mut *opts };
+            let opts = non_null! { &mut *opts ?= hyper_code::HYPERE_INVALID_ARG };
             opts.builder.http2_only(enabled != 0);
             hyper_code::HYPERE_OK
         }
@@ -168,7 +155,7 @@ ffi_fn! {
     ///
     /// If enabled, see `hyper_response_headers_raw()` for usage.
     fn hyper_clientconn_options_headers_raw(opts: *mut hyper_clientconn_options, enabled: c_int) -> hyper_code {
-        let opts = unsafe { &mut *opts };
+        let opts = non_null! { &mut *opts ?= hyper_code::HYPERE_INVALID_ARG };
         opts.builder.http1_headers_raw(enabled != 0);
         hyper_code::HYPERE_OK
     }

src/ffi/error.rs

@@ -58,14 +58,14 @@ impl hyper_error {
 ffi_fn! {
     /// Frees a `hyper_error`.
     fn hyper_error_free(err: *mut hyper_error) {
-        drop(unsafe { Box::from_raw(err) });
+        drop(non_null!(Box::from_raw(err) ?= ()));
     }
 }
 
 ffi_fn! {
     /// Get an equivalent `hyper_code` from this error.
     fn hyper_error_code(err: *const hyper_error) -> hyper_code {
-        unsafe { &*err }.code()
+        non_null!(&*err ?= hyper_code::HYPERE_INVALID_ARG).code()
     }
 }
 
@@ -80,6 +80,6 @@ ffi_fn! {
         let dst = unsafe {
             std::slice::from_raw_parts_mut(dst, dst_len)
         };
-        unsafe { &*err }.print_to(dst)
+        non_null!(&*err ?= 0).print_to(dst)
     }
 }

src/ffi/http_types.rs

@@ -48,7 +48,7 @@ ffi_fn! {
 ffi_fn! {
     /// Free an HTTP request if not going to send it on a client.
     fn hyper_request_free(req: *mut hyper_request) {
-        drop(unsafe { Box::from_raw(req) });
+        drop(non_null!(Box::from_raw(req) ?= ()));
     }
 }
 
@@ -58,9 +58,10 @@ ffi_fn! {
         let bytes = unsafe {
             std::slice::from_raw_parts(method, method_len as usize)
         };
+        let req = non_null!(&mut *req ?= hyper_code::HYPERE_INVALID_ARG);
         match Method::from_bytes(bytes) {
             Ok(m) => {
-                *unsafe { &mut *req }.0.method_mut() = m;
+                *req.0.method_mut() = m;
                 hyper_code::HYPERE_OK
             },
             Err(_) => {
@@ -76,9 +77,10 @@ ffi_fn! {
         let bytes = unsafe {
             std::slice::from_raw_parts(uri, uri_len as usize)
         };
+        let req = non_null!(&mut *req ?= hyper_code::HYPERE_INVALID_ARG);
         match Uri::from_maybe_shared(bytes) {
             Ok(u) => {
-                *unsafe { &mut *req }.0.uri_mut() = u;
+                *req.0.uri_mut() = u;
                 hyper_code::HYPERE_OK
             },
             Err(_) => {
@@ -98,7 +100,8 @@ ffi_fn! {
     fn hyper_request_set_version(req: *mut hyper_request, version: c_int) -> hyper_code {
         use http::Version;
 
-        *unsafe { &mut *req }.0.version_mut() = match version {
+        let req = non_null!(&mut *req ?= hyper_code::HYPERE_INVALID_ARG);
+        *req.0.version_mut() = match version {
             super::HYPER_HTTP_VERSION_NONE => Version::HTTP_11,
             super::HYPER_HTTP_VERSION_1_0 => Version::HTTP_10,
             super::HYPER_HTTP_VERSION_1_1 => Version::HTTP_11,
@@ -130,8 +133,9 @@ ffi_fn! {
     /// This takes ownership of the `hyper_body *`, you must not use it or
     /// free it after setting it on the request.
     fn hyper_request_set_body(req: *mut hyper_request, body: *mut hyper_body) -> hyper_code {
-        let body = unsafe { Box::from_raw(body) };
-        *unsafe { &mut *req }.0.body_mut() = body.0;
+        let body = non_null!(Box::from_raw(body) ?= hyper_code::HYPERE_INVALID_ARG);
+        let req = non_null!(&mut *req ?= hyper_code::HYPERE_INVALID_ARG);
+        *req.0.body_mut() = body.0;
         hyper_code::HYPERE_OK
     }
 }
@@ -157,7 +161,8 @@ ffi_fn! {
             func: callback,
             data: UserDataPointer(data),
         };
-        unsafe { &mut *req }.0.extensions_mut().insert(ext);
+        let req = non_null!(&mut *req ?= hyper_code::HYPERE_INVALID_ARG);
+        req.0.extensions_mut().insert(ext);
         hyper_code::HYPERE_OK
     }
 }
@@ -176,7 +181,7 @@ impl hyper_request {
 ffi_fn! {
     /// Free an HTTP response after using it.
     fn hyper_response_free(resp: *mut hyper_response) {
-        drop(unsafe { Box::from_raw(resp) });
+        drop(non_null!(Box::from_raw(resp) ?= ()));
     }
 }
 
@@ -185,7 +190,7 @@ ffi_fn! {
     ///
     /// It will always be within the range of 100-599.
     fn hyper_response_status(resp: *const hyper_response) -> u16 {
-        unsafe { &*resp }.0.status().as_u16()
+        non_null!(&*resp ?= 0).0.status().as_u16()
     }
 }
 
@@ -200,7 +205,7 @@ ffi_fn! {
     /// Use `hyper_response_reason_phrase_len()` to get the length of this
     /// buffer.
     fn hyper_response_reason_phrase(resp: *const hyper_response) -> *const u8 {
-        unsafe { &*resp }.reason_phrase().as_ptr()
+        non_null!(&*resp ?= std::ptr::null()).reason_phrase().as_ptr()
     } ?= std::ptr::null()
 }
 
@@ -209,7 +214,7 @@ ffi_fn! {
     ///
     /// Use `hyper_response_reason_phrase()` to get the buffer pointer.
     fn hyper_response_reason_phrase_len(resp: *const hyper_response) -> size_t {
-        unsafe { &*resp }.reason_phrase().len()
+        non_null!(&*resp ?= 0).reason_phrase().len()
     }
 }
 
@@ -226,7 +231,8 @@ ffi_fn! {
     /// The buffer is not null-terminated, see the `hyper_buf` functions for
     /// getting the bytes and length.
     fn hyper_response_headers_raw(resp: *const hyper_response) -> *const hyper_buf {
-        match unsafe { &*resp }.0.extensions().get::<RawHeaders>() {
+        let resp = non_null!(&*resp ?= std::ptr::null());
+        match resp.0.extensions().get::<RawHeaders>() {
             Some(raw) => &raw.0,
             None => std::ptr::null(),
         }
@@ -245,7 +251,7 @@ ffi_fn! {
     fn hyper_response_version(resp: *const hyper_response) -> c_int {
         use http::Version;
 
-        match unsafe { &*resp }.0.version() {
+        match non_null!(&*resp ?= 0).0.version() {
             Version::HTTP_10 => super::HYPER_HTTP_VERSION_1_0,
             Version::HTTP_11 => super::HYPER_HTTP_VERSION_1_1,
             Version::HTTP_2 => super::HYPER_HTTP_VERSION_2,
@@ -269,7 +275,7 @@ ffi_fn! {
     ///
     /// It is safe to free the response even after taking ownership of its body.
     fn hyper_response_body(resp: *mut hyper_response) -> *mut hyper_body {
-        let body = std::mem::take(unsafe { &mut *resp }.0.body_mut());
+        let body = std::mem::take(non_null!(&mut *resp ?= std::ptr::null_mut()).0.body_mut());
         Box::into_raw(Box::new(hyper_body(body)))
     } ?= std::ptr::null_mut()
 }
@@ -331,7 +337,7 @@ ffi_fn! {
     /// The callback should return `HYPER_ITER_CONTINUE` to keep iterating, or
     /// `HYPER_ITER_BREAK` to stop.
     fn hyper_headers_foreach(headers: *const hyper_headers, func: hyper_headers_foreach_callback, userdata: *mut c_void) {
-        let headers = unsafe { &*headers };
+        let headers = non_null!(&*headers ?= ());
         // For each header name/value pair, there may be a value in the casemap
         // that corresponds to the HeaderValue. So, we iterator all the keys,
         // and for each one, try to pair the originally cased name with the value.
@@ -366,7 +372,7 @@ ffi_fn! {
     ///
     /// This overwrites any previous value set for the header.
     fn hyper_headers_set(headers: *mut hyper_headers, name: *const u8, name_len: size_t, value: *const u8, value_len: size_t) -> hyper_code {
-        let headers = unsafe { &mut *headers };
+        let headers = non_null!(&mut *headers ?= hyper_code::HYPERE_INVALID_ARG);
         match unsafe { raw_name_value(name, name_len, value, value_len) } {
             Ok((name, value, orig_name)) => {
                 headers.headers.insert(&name, value);
@@ -384,7 +390,7 @@ ffi_fn! {
     /// If there were already existing values for the name, this will append the
     /// new value to the internal list.
     fn hyper_headers_add(headers: *mut hyper_headers, name: *const u8, name_len: size_t, value: *const u8, value_len: size_t) -> hyper_code {
-        let headers = unsafe { &mut *headers };
+        let headers = non_null!(&mut *headers ?= hyper_code::HYPERE_INVALID_ARG);
 
         match unsafe { raw_name_value(name, name_len, value, value_len) } {
             Ok((name, value, orig_name)) => {

src/ffi/io.rs

@@ -46,7 +46,7 @@ ffi_fn! {
     /// This is typically only useful if you aren't going to pass ownership
     /// of the IO handle to hyper, such as with `hyper_clientconn_handshake()`.
     fn hyper_io_free(io: *mut hyper_io) {
-        drop(unsafe { Box::from_raw(io) });
+        drop(non_null!(Box::from_raw(io) ?= ()));
     }
 }
 
@@ -55,7 +55,7 @@ ffi_fn! {
     ///
     /// This value is passed as an argument to the read and write callbacks.
     fn hyper_io_set_userdata(io: *mut hyper_io, data: *mut c_void) {
-        unsafe { &mut *io }.userdata = data;
+        non_null!(&mut *io ?= ()).userdata = data;
     }
 }
 
@@ -77,7 +77,7 @@ ffi_fn! {
     /// If there is an irrecoverable error reading data, then `HYPER_IO_ERROR`
     /// should be the return value.
     fn hyper_io_set_read(io: *mut hyper_io, func: hyper_io_read_callback) {
-        unsafe { &mut *io }.read = func;
+        non_null!(&mut *io ?= ()).read = func;
     }
 }
 
@@ -96,7 +96,7 @@ ffi_fn! {
     /// If there is an irrecoverable error reading data, then `HYPER_IO_ERROR`
     /// should be the return value.
     fn hyper_io_set_write(io: *mut hyper_io, func: hyper_io_write_callback) {
-        unsafe { &mut *io }.write = func;
+        non_null!(&mut *io ?= ()).write = func;
     }
 }