From 766059075d32214b83eb8fb58c98748308665ade Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Wed, 18 Aug 2021 12:15:13 -0700 Subject: [PATCH] refactor(ffi): check pointer arguments for NULL 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`). --- capi/examples/upload.c | 6 ++++-- src/ffi/body.rs | 18 +++++------------- src/ffi/client.rs | 35 +++++++++++----------------------- src/ffi/error.rs | 6 +++--- src/ffi/http_types.rs | 40 ++++++++++++++++++++++----------------- src/ffi/io.rs | 8 ++++---- src/ffi/macros.rs | 22 +++++++++++++++++++++ src/ffi/task.rs | 43 +++++++++++++----------------------------- 8 files changed, 85 insertions(+), 93 deletions(-) diff --git a/capi/examples/upload.c b/capi/examples/upload.c index fa7134f369..5492944241 100644 --- a/capi/examples/upload.c +++ b/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 { diff --git a/src/ffi/body.rs b/src/ffi/body.rs index d6e1394371..932200b54d 100644 --- a/src/ffi/body.rs +++ b/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; } } diff --git a/src/ffi/client.rs b/src/ffi/client.rs index 2e73e6695a..1e5f29d548 100644 --- a/src/ffi/client.rs +++ b/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 } diff --git a/src/ffi/error.rs b/src/ffi/error.rs index 7b85407099..0decc57617 100644 --- a/src/ffi/error.rs +++ b/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) } } diff --git a/src/ffi/http_types.rs b/src/ffi/http_types.rs index e26557cebf..ec5b344b52 100644 --- a/src/ffi/http_types.rs +++ b/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::() { + let resp = non_null!(&*resp ?= std::ptr::null()); + match resp.0.extensions().get::() { 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)) => { diff --git a/src/ffi/io.rs b/src/ffi/io.rs index 7fb4538815..13fec013d3 100644 --- a/src/ffi/io.rs +++ b/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; } } diff --git a/src/ffi/macros.rs b/src/ffi/macros.rs index 12064d41a2..022711baaa 100644 --- a/src/ffi/macros.rs +++ b/src/ffi/macros.rs @@ -29,3 +29,25 @@ macro_rules! ffi_fn { ffi_fn!($(#[$doc])* fn $name($($arg: $arg_ty),*) -> () $body); }; } + +macro_rules! non_null { + ($ptr:ident, $eval:expr, $err:expr) => {{ + debug_assert!(!$ptr.is_null(), "{:?} must not be null", stringify!($ptr)); + if $ptr.is_null() { + return $err; + } + unsafe { $eval } + }}; + (&*$ptr:ident ?= $err:expr) => {{ + non_null!($ptr, &*$ptr, $err) + }}; + (&mut *$ptr:ident ?= $err:expr) => {{ + non_null!($ptr, &mut *$ptr, $err) + }}; + (Box::from_raw($ptr:ident) ?= $err:expr) => {{ + non_null!($ptr, Box::from_raw($ptr), $err) + }}; + (Arc::from_raw($ptr:ident) ?= $err:expr) => {{ + non_null!($ptr, Arc::from_raw($ptr), $err) + }}; +} diff --git a/src/ffi/task.rs b/src/ffi/task.rs index f92798f0c8..b822af0592 100644 --- a/src/ffi/task.rs +++ b/src/ffi/task.rs @@ -195,7 +195,7 @@ ffi_fn! { ffi_fn! { /// Frees an executor and any incomplete tasks still part of it. fn hyper_executor_free(exec: *const hyper_executor) { - drop(unsafe { Arc::from_raw(exec) }); + drop(non_null! { Arc::from_raw(exec) ?= () }); } } @@ -205,11 +205,8 @@ ffi_fn! { /// The executor takes ownership of the task, it should not be accessed /// again unless returned back to the user with `hyper_executor_poll`. fn hyper_executor_push(exec: *const hyper_executor, task: *mut hyper_task) -> hyper_code { - if exec.is_null() || task.is_null() { - return hyper_code::HYPERE_INVALID_ARG; - } - let exec = unsafe { &*exec }; - let task = unsafe { Box::from_raw(task) }; + let exec = non_null! { &*exec ?= hyper_code::HYPERE_INVALID_ARG }; + let task = non_null! { Box::from_raw(task) ?= hyper_code::HYPERE_INVALID_ARG }; exec.spawn(task); hyper_code::HYPERE_OK } @@ -223,9 +220,7 @@ ffi_fn! { /// /// If there are no ready tasks, this returns `NULL`. fn hyper_executor_poll(exec: *const hyper_executor) -> *mut hyper_task { - // We only want an `&Arc` in here, so wrap in a `ManuallyDrop` so we - // don't accidentally trigger a ref_dec of the Arc. - let exec = unsafe { &*exec }; + let exec = non_null! { &*exec ?= ptr::null_mut() }; match exec.poll_next() { Some(task) => Box::into_raw(task), None => ptr::null_mut(), @@ -274,7 +269,7 @@ impl Future for TaskFuture { ffi_fn! { /// Free a task. fn hyper_task_free(task: *mut hyper_task) { - drop(unsafe { Box::from_raw(task) }); + drop(non_null! { Box::from_raw(task) ?= () }); } } @@ -286,11 +281,7 @@ ffi_fn! { /// /// Use `hyper_task_type` to determine the type of the `void *` return value. fn hyper_task_value(task: *mut hyper_task) -> *mut c_void { - if task.is_null() { - return ptr::null_mut(); - } - - let task = unsafe { &mut *task }; + let task = non_null! { &mut *task ?= ptr::null_mut() }; if let Some(val) = task.output.take() { let p = Box::into_raw(val) as *mut c_void; @@ -309,13 +300,9 @@ ffi_fn! { ffi_fn! { /// Query the return type of this task. fn hyper_task_type(task: *mut hyper_task) -> hyper_task_return_type { - if task.is_null() { - // instead of blowing up spectacularly, just say this null task - // doesn't have a value to retrieve. - return hyper_task_return_type::HYPER_TASK_EMPTY; - } - - unsafe { &*task }.output_type() + // instead of blowing up spectacularly, just say this null task + // doesn't have a value to retrieve. + non_null! { &*task ?= hyper_task_return_type::HYPER_TASK_EMPTY }.output_type() } } @@ -336,11 +323,7 @@ ffi_fn! { ffi_fn! { /// Retrieve the userdata that has been set via `hyper_task_set_userdata`. fn hyper_task_userdata(task: *mut hyper_task) -> *mut c_void { - if task.is_null() { - return ptr::null_mut(); - } - - unsafe { &*task }.userdata.0 + non_null! { &*task ?= ptr::null_mut() }.userdata.0 } ?= ptr::null_mut() } @@ -403,7 +386,7 @@ impl hyper_context<'_> { ffi_fn! { /// Copies a waker out of the task context. fn hyper_context_waker(cx: *mut hyper_context<'_>) -> *mut hyper_waker { - let waker = unsafe { &mut *cx }.0.waker().clone(); + let waker = non_null! { &mut *cx ?= ptr::null_mut() }.0.waker().clone(); Box::into_raw(Box::new(hyper_waker { waker })) } ?= ptr::null_mut() } @@ -413,7 +396,7 @@ ffi_fn! { ffi_fn! { /// Free a waker that hasn't been woken. fn hyper_waker_free(waker: *mut hyper_waker) { - drop(unsafe { Box::from_raw(waker) }); + drop(non_null! { Box::from_raw(waker) ?= () }); } } @@ -422,7 +405,7 @@ ffi_fn! { /// /// NOTE: This consumes the waker. You should not use or free the waker afterwards. fn hyper_waker_wake(waker: *mut hyper_waker) { - let waker = unsafe { Box::from_raw(waker) }; + let waker = non_null! { Box::from_raw(waker) ?= () }; waker.waker.wake(); } }