forked/hyper
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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
This commit is contained in:
Sean McArthur
2021-08-18 14:15:14 -07:00
committed by GitHub
parent c35153998e
commit 3b26572876
8 changed files with 85 additions and 93 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
capi/examples/upload.c
View File

@@ -154,8 +154,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);
if (headers) {
write(1, hyper_buf_bytes(headers), hyper_buf_len(headers));
}
}
typedef enum {
EXAMPLE_NOT_SET = 0, // tasks we don't know about won't have a userdata set

18
src/ffi/body.rs
View File

@@ -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;
}
}

35
src/ffi/client.rs
View File

@@ -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
}

6
src/ffi/error.rs
View File

@@ -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)
}
}

40
src/ffi/http_types.rs
View File

@@ -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)) => {

8
src/ffi/io.rs
View File

@@ -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;
}
}

22
src/ffi/macros.rs
View File

@@ -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)
}};
}

39
src/ffi/task.rs
View File

@@ -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()
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();
}
}