From 82b7fa592c0cc7ee0a3db251576ebd94b5819a43 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 28 Aug 2023 08:11:54 +1000 Subject: [PATCH] docs(ffi): clarify ownership responsibilities in the C API Curl had two memory leaks in its usage of the hyper C API. I fixed these in https://github.com/curl/curl/pull/11729. While fixing these, as a hyper newbie I found a lack of clarity in the docs about where the responsibilities lie for allocating and deallocating things. I had to repeatedly look at the `Box::into_raw`/`Box::from_raw` calls in the C API code to properly understand things. This commit extends the docs. - For all functions that allocate things, make it clear which functions can be used to subsequently deallocate. - For all functions that free things, make it clear if there are (or aren't) other functions that may have otherwise consumed the thing, thus removing the need for explicit freeing. - Clarify some functions that consume things. This is the documentation I wished was present when I was fixing the leaks in curl. --- capi/include/hyper.h | 103 +++++++++++++++++++++++++++++++++++++----- src/ffi/body.rs | 23 +++++++++- src/ffi/client.rs | 26 +++++++++-- src/ffi/error.rs | 2 + src/ffi/http_types.rs | 15 +++++- src/ffi/io.rs | 9 ++-- src/ffi/task.rs | 28 +++++++++++- 7 files changed, 182 insertions(+), 24 deletions(-) diff --git a/capi/include/hyper.h b/capi/include/hyper.h index 591cb9771c..a57e3a27a9 100644 --- a/capi/include/hyper.h +++ b/capi/include/hyper.h @@ -229,11 +229,17 @@ const char *hyper_version(void); Create a new "empty" body. If not configured, this body acts as an empty payload. + + To avoid a memory leak, the body must eventually be consumed by + `hyper_body_free`, `hyper_body_foreach`, or `hyper_request_set_body`. */ struct hyper_body *hyper_body_new(void); /* - Free a `hyper_body *`. + Free a body. + + This should only be used if the request isn't consumed by + `hyper_body_foreach` or `hyper_request_set_body`. */ void hyper_body_free(struct hyper_body *body); @@ -246,6 +252,10 @@ void hyper_body_free(struct hyper_body *body); - `HYPER_TASK_ERROR`: An error retrieving the data. - `HYPER_TASK_EMPTY`: The body has finished streaming data. + To avoid a memory leak, the task must eventually be consumed by + `hyper_task_free`, or taken ownership of by `hyper_executor_push` + without subsequently being given back by `hyper_executor_poll`. + This does not consume the `hyper_body *`, so it may be used to again. However, it MUST NOT be used or freed until the related task completes. */ @@ -255,6 +265,10 @@ struct hyper_task *hyper_body_data(struct hyper_body *body); Return a task that will poll the body and execute the callback with each body chunk that is received. + To avoid a memory leak, the task must eventually be consumed by + `hyper_task_free`, or taken ownership of by `hyper_executor_push` + without subsequently being given back by `hyper_executor_poll`. + The `hyper_buf` pointer is only a borrowed reference, it cannot live outside the execution of the callback. You must make a copy to retain it. @@ -299,7 +313,10 @@ void hyper_body_set_data_func(struct hyper_body *body, hyper_body_data_callback Create a new `hyper_buf *` by copying the provided bytes. This makes an owned copy of the bytes, so the `buf` argument can be - freed or changed afterwards. + freed (with `hyper_buf_free`) or changed afterwards. + + To avoid a memory leak, the copy must eventually be consumed by + `hyper_buf_free`. This returns `NULL` if allocating a new buffer fails. */ @@ -323,6 +340,8 @@ size_t hyper_buf_len(const struct hyper_buf *buf); /* Free this buffer. + + This should be used for any buffer once it is no longer needed. */ void hyper_buf_free(struct hyper_buf *buf); @@ -331,9 +350,14 @@ void hyper_buf_free(struct hyper_buf *buf); and options. Both the `io` and the `options` are consumed in this function call. + They should not be used or freed afterwards. + + The returned task must be polled with an executor until the handshake + completes, at which point the value can be taken. - The returned `hyper_task *` must be polled with an executor until the - handshake completes, at which point the value can be taken. + To avoid a memory leak, the task must eventually be consumed by + `hyper_task_free`, or taken ownership of by `hyper_executor_push` + without subsequently being given back by `hyper_executor_poll`. */ struct hyper_task *hyper_clientconn_handshake(struct hyper_io *io, struct hyper_clientconn_options *options); @@ -341,18 +365,30 @@ struct hyper_task *hyper_clientconn_handshake(struct hyper_io *io, /* Send a request on the client connection. + This consumes the request. You should not use or free the request + afterwards. + Returns a task that needs to be polled until it is ready. When ready, the task yields a `hyper_response *`. + + To avoid a memory leak, the task must eventually be consumed by + `hyper_task_free`, or taken ownership of by `hyper_executor_push` + without subsequently being given back by `hyper_executor_poll`. */ struct hyper_task *hyper_clientconn_send(struct hyper_clientconn *conn, struct hyper_request *req); /* Free a `hyper_clientconn *`. + + This should be used for any connection once it is no longer needed. */ void hyper_clientconn_free(struct hyper_clientconn *conn); /* Creates a new set of HTTP clientconn options to be used in a handshake. + + To avoid a memory leak, the options must eventually be consumed by + `hyper_clientconn_options_free` or `hyper_clientconn_handshake`. */ struct hyper_clientconn_options *hyper_clientconn_options_new(void); @@ -373,7 +409,10 @@ void hyper_clientconn_options_set_preserve_header_order(struct hyper_clientconn_ int enabled); /* - Free a `hyper_clientconn_options *`. + Free a set of HTTP clientconn options. + + This should only be used if the options aren't consumed by + `hyper_clientconn_handshake`. */ void hyper_clientconn_options_free(struct hyper_clientconn_options *opts); @@ -404,6 +443,8 @@ enum hyper_code hyper_clientconn_options_http1_allow_multiline_headers(struct hy /* Frees a `hyper_error`. + + This should be used for any error once it is no longer needed. */ void hyper_error_free(struct hyper_error *err); @@ -424,11 +465,17 @@ size_t hyper_error_print(const struct hyper_error *err, uint8_t *dst, size_t dst /* Construct a new HTTP request. + + To avoid a memory leak, the request must eventually be consumed by + `hyper_request_free` or `hyper_clientconn_send`. */ struct hyper_request *hyper_request_new(void); /* - Free an HTTP request if not going to send it on a client. + Free an HTTP request. + + This should only be used if the request isn't consumed by + `hyper_clientconn_send`. */ void hyper_request_free(struct hyper_request *req); @@ -526,7 +573,9 @@ enum hyper_code hyper_request_on_informational(struct hyper_request *req, void *data); /* - Free an HTTP response after using it. + Free an HTTP response. + + This should be used for any response once it is no longer needed. */ void hyper_response_free(struct hyper_response *resp); @@ -581,6 +630,9 @@ struct hyper_headers *hyper_response_headers(struct hyper_response *resp); Take ownership of the body of this response. It is safe to free the response even after taking ownership of its body. + + To avoid a memory leak, the body must eventually be consumed by + `hyper_body_free`, `hyper_body_foreach`, or `hyper_request_set_body`. */ struct hyper_body *hyper_response_body(struct hyper_response *resp); @@ -624,14 +676,17 @@ enum hyper_code hyper_headers_add(struct hyper_headers *headers, The read and write functions of this transport should be set with `hyper_io_set_read` and `hyper_io_set_write`. + + To avoid a memory leak, the IO handle must eventually be consumed by + `hyper_io_free` or `hyper_clientconn_handshake`. */ struct hyper_io *hyper_io_new(void); /* - Free an unused `hyper_io *`. + Free an IO handle. - 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()`. + This should only be used if the request isn't consumed by + `hyper_clientconn_handshake`. */ void hyper_io_free(struct hyper_io *io); @@ -681,18 +736,23 @@ void hyper_io_set_write(struct hyper_io *io, hyper_io_write_callback func); /* Creates a new task executor. + + To avoid a memory leak, the executor must eventually be consumed by + `hyper_executor_free`. */ const struct hyper_executor *hyper_executor_new(void); /* Frees an executor and any incomplete tasks still part of it. + + This should be used for any executor once it is no longer needed. */ void hyper_executor_free(const struct hyper_executor *exec); /* Push a task onto the executor. - The executor takes ownership of the task, it should not be accessed + The executor takes ownership of the task, which should not be accessed again unless returned back to the user with `hyper_executor_poll`. */ enum hyper_code hyper_executor_push(const struct hyper_executor *exec, struct hyper_task *task); @@ -703,12 +763,20 @@ enum hyper_code hyper_executor_push(const struct hyper_executor *exec, struct hy If ready, returns a task from the executor that has completed. + To avoid a memory leak, the task must eventually be consumed by + `hyper_task_free`, or taken ownership of by `hyper_executor_push` + without subsequently being given back by `hyper_executor_poll`. + If there are no ready tasks, this returns `NULL`. */ struct hyper_task *hyper_executor_poll(const struct hyper_executor *exec); /* Free a task. + + This should only be used if the task isn't consumed by + `hyper_clientconn_handshake` or taken ownership of by + `hyper_executor_push`. */ void hyper_task_free(struct hyper_task *task); @@ -719,6 +787,11 @@ void hyper_task_free(struct hyper_task *task); this task. Use `hyper_task_type` to determine the type of the `void *` return value. + + To avoid a memory leak, a non-empty return value must eventually be + consumed by a function appropriate for its type, one of + `hyper_error_free`, `hyper_clientconn_free`, `hyper_response_free`, or + `hyper_buf_free`. */ void *hyper_task_value(struct hyper_task *task); @@ -742,11 +815,17 @@ void *hyper_task_userdata(struct hyper_task *task); /* Copies a waker out of the task context. + + To avoid a memory leak, the waker must eventually be consumed by + `hyper_waker_free` or `hyper_waker_wake`. */ struct hyper_waker *hyper_context_waker(struct hyper_context *cx); /* - Free a waker that hasn't been woken. + Free a waker. + + This should only be used if the request isn't consumed by + `hyper_waker_wake`. */ void hyper_waker_free(struct hyper_waker *waker); diff --git a/src/ffi/body.rs b/src/ffi/body.rs index 5a56356d72..91c5c7342f 100644 --- a/src/ffi/body.rs +++ b/src/ffi/body.rs @@ -32,13 +32,19 @@ ffi_fn! { /// Create a new "empty" body. /// /// If not configured, this body acts as an empty payload. + /// + /// To avoid a memory leak, the body must eventually be consumed by + /// `hyper_body_free`, `hyper_body_foreach`, or `hyper_request_set_body`. fn hyper_body_new() -> *mut hyper_body { Box::into_raw(Box::new(hyper_body(IncomingBody::ffi()))) } ?= ptr::null_mut() } ffi_fn! { - /// Free a `hyper_body *`. + /// Free a body. + /// + /// This should only be used if the request isn't consumed by + /// `hyper_body_foreach` or `hyper_request_set_body`. fn hyper_body_free(body: *mut hyper_body) { drop(non_null!(Box::from_raw(body) ?= ())); } @@ -53,6 +59,10 @@ ffi_fn! { /// - `HYPER_TASK_ERROR`: An error retrieving the data. /// - `HYPER_TASK_EMPTY`: The body has finished streaming data. /// + /// To avoid a memory leak, the task must eventually be consumed by + /// `hyper_task_free`, or taken ownership of by `hyper_executor_push` + /// without subsequently being given back by `hyper_executor_poll`. + /// /// This does not consume the `hyper_body *`, so it may be used to again. /// However, it MUST NOT be used or freed until the related task completes. fn hyper_body_data(body: *mut hyper_body) -> *mut hyper_task { @@ -81,6 +91,10 @@ ffi_fn! { /// Return a task that will poll the body and execute the callback with each /// body chunk that is received. /// + /// To avoid a memory leak, the task must eventually be consumed by + /// `hyper_task_free`, or taken ownership of by `hyper_executor_push` + /// without subsequently being given back by `hyper_executor_poll`. + /// /// The `hyper_buf` pointer is only a borrowed reference, it cannot live outside /// the execution of the callback. You must make a copy to retain it. /// @@ -194,7 +208,10 @@ ffi_fn! { /// Create a new `hyper_buf *` by copying the provided bytes. /// /// This makes an owned copy of the bytes, so the `buf` argument can be - /// freed or changed afterwards. + /// freed (with `hyper_buf_free`) or changed afterwards. + /// + /// To avoid a memory leak, the copy must eventually be consumed by + /// `hyper_buf_free`. /// /// This returns `NULL` if allocating a new buffer fails. fn hyper_buf_copy(buf: *const u8, len: size_t) -> *mut hyper_buf { @@ -227,6 +244,8 @@ ffi_fn! { ffi_fn! { /// Free this buffer. + /// + /// This should be used for any buffer once it is no longer needed. fn hyper_buf_free(buf: *mut hyper_buf) { drop(unsafe { Box::from_raw(buf) }); } diff --git a/src/ffi/client.rs b/src/ffi/client.rs index a300076d8f..4de81d9b06 100644 --- a/src/ffi/client.rs +++ b/src/ffi/client.rs @@ -44,9 +44,14 @@ ffi_fn! { /// and options. /// /// Both the `io` and the `options` are consumed in this function call. + /// They should not be used or freed afterwards. /// - /// The returned `hyper_task *` must be polled with an executor until the - /// handshake completes, at which point the value can be taken. + /// The returned task must be polled with an executor until the handshake + /// completes, at which point the value can be taken. + /// + /// To avoid a memory leak, the task must eventually be consumed by + /// `hyper_task_free`, or taken ownership of by `hyper_executor_push` + /// without subsequently being given back by `hyper_executor_poll`. fn hyper_clientconn_handshake(io: *mut hyper_io, options: *mut hyper_clientconn_options) -> *mut hyper_task { let options = non_null! { Box::from_raw(options) ?= ptr::null_mut() }; let io = non_null! { Box::from_raw(io) ?= ptr::null_mut() }; @@ -86,8 +91,15 @@ ffi_fn! { ffi_fn! { /// Send a request on the client connection. /// + /// This consumes the request. You should not use or free the request + /// afterwards. + /// /// Returns a task that needs to be polled until it is ready. When ready, the /// task yields a `hyper_response *`. + /// + /// To avoid a memory leak, the task must eventually be consumed by + /// `hyper_task_free`, or taken ownership of by `hyper_executor_push` + /// without subsequently being given back by `hyper_executor_poll`. fn hyper_clientconn_send(conn: *mut hyper_clientconn, req: *mut hyper_request) -> *mut hyper_task { let mut req = non_null! { Box::from_raw(req) ?= ptr::null_mut() }; @@ -109,6 +121,8 @@ ffi_fn! { ffi_fn! { /// Free a `hyper_clientconn *`. + /// + /// This should be used for any connection once it is no longer needed. fn hyper_clientconn_free(conn: *mut hyper_clientconn) { drop(non_null! { Box::from_raw(conn) ?= () }); } @@ -124,6 +138,9 @@ unsafe impl AsTaskType for hyper_clientconn { ffi_fn! { /// Creates a new set of HTTP clientconn options to be used in a handshake. + /// + /// To avoid a memory leak, the options must eventually be consumed by + /// `hyper_clientconn_options_free` or `hyper_clientconn_handshake`. fn hyper_clientconn_options_new() -> *mut hyper_clientconn_options { Box::into_raw(Box::new(hyper_clientconn_options { http1_allow_obsolete_multiline_headers_in_responses: false, @@ -156,7 +173,10 @@ ffi_fn! { } ffi_fn! { - /// Free a `hyper_clientconn_options *`. + /// Free a set of HTTP clientconn options. + /// + /// This should only be used if the options aren't consumed by + /// `hyper_clientconn_handshake`. fn hyper_clientconn_options_free(opts: *mut hyper_clientconn_options) { drop(non_null! { Box::from_raw(opts) ?= () }); } diff --git a/src/ffi/error.rs b/src/ffi/error.rs index 015e595aee..f545aa2737 100644 --- a/src/ffi/error.rs +++ b/src/ffi/error.rs @@ -57,6 +57,8 @@ impl hyper_error { ffi_fn! { /// Frees a `hyper_error`. + /// + /// This should be used for any error once it is no longer needed. fn hyper_error_free(err: *mut hyper_error) { drop(non_null!(Box::from_raw(err) ?= ())); } diff --git a/src/ffi/http_types.rs b/src/ffi/http_types.rs index 473feefa84..565ab095ce 100644 --- a/src/ffi/http_types.rs +++ b/src/ffi/http_types.rs @@ -37,13 +37,19 @@ type hyper_request_on_informational_callback = extern "C" fn(*mut c_void, *mut h ffi_fn! { /// Construct a new HTTP request. + /// + /// To avoid a memory leak, the request must eventually be consumed by + /// `hyper_request_free` or `hyper_clientconn_send`. fn hyper_request_new() -> *mut hyper_request { Box::into_raw(Box::new(hyper_request(Request::new(IncomingBody::empty())))) } ?= std::ptr::null_mut() } ffi_fn! { - /// Free an HTTP request if not going to send it on a client. + /// Free an HTTP request. + /// + /// This should only be used if the request isn't consumed by + /// `hyper_clientconn_send`. fn hyper_request_free(req: *mut hyper_request) { drop(non_null!(Box::from_raw(req) ?= ())); } @@ -238,7 +244,9 @@ impl hyper_request { // ===== impl hyper_response ===== ffi_fn! { - /// Free an HTTP response after using it. + /// Free an HTTP response. + /// + /// This should be used for any response once it is no longer needed. fn hyper_response_free(resp: *mut hyper_response) { drop(non_null!(Box::from_raw(resp) ?= ())); } @@ -312,6 +320,9 @@ ffi_fn! { /// Take ownership of the body of this response. /// /// It is safe to free the response even after taking ownership of its body. + /// + /// To avoid a memory leak, the body must eventually be consumed by + /// `hyper_body_free`, `hyper_body_foreach`, or `hyper_request_set_body`. fn hyper_response_body(resp: *mut hyper_response) -> *mut hyper_body { let body = std::mem::replace(non_null!(&mut *resp ?= std::ptr::null_mut()).0.body_mut(), IncomingBody::empty()); Box::into_raw(Box::new(hyper_body(body))) diff --git a/src/ffi/io.rs b/src/ffi/io.rs index 1d198820a6..c1ba87a02b 100644 --- a/src/ffi/io.rs +++ b/src/ffi/io.rs @@ -31,6 +31,9 @@ ffi_fn! { /// /// The read and write functions of this transport should be set with /// `hyper_io_set_read` and `hyper_io_set_write`. + /// + /// To avoid a memory leak, the IO handle must eventually be consumed by + /// `hyper_io_free` or `hyper_clientconn_handshake`. fn hyper_io_new() -> *mut hyper_io { Box::into_raw(Box::new(hyper_io { read: read_noop, @@ -41,10 +44,10 @@ ffi_fn! { } ffi_fn! { - /// Free an unused `hyper_io *`. + /// Free an IO handle. /// - /// 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()`. + /// This should only be used if the request isn't consumed by + /// `hyper_clientconn_handshake`. fn hyper_io_free(io: *mut hyper_io) { drop(non_null!(Box::from_raw(io) ?= ())); } diff --git a/src/ffi/task.rs b/src/ffi/task.rs index a973a7bab3..e75dfc1a12 100644 --- a/src/ffi/task.rs +++ b/src/ffi/task.rs @@ -191,6 +191,9 @@ where ffi_fn! { /// Creates a new task executor. + /// + /// To avoid a memory leak, the executor must eventually be consumed by + /// `hyper_executor_free`. fn hyper_executor_new() -> *const hyper_executor { Arc::into_raw(hyper_executor::new()) } ?= ptr::null() @@ -198,6 +201,8 @@ ffi_fn! { ffi_fn! { /// Frees an executor and any incomplete tasks still part of it. + /// + /// This should be used for any executor once it is no longer needed. fn hyper_executor_free(exec: *const hyper_executor) { drop(non_null!(Arc::from_raw(exec) ?= ())); } @@ -206,7 +211,7 @@ ffi_fn! { ffi_fn! { /// Push a task onto the executor. /// - /// The executor takes ownership of the task, it should not be accessed + /// The executor takes ownership of the task, which 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 { let exec = non_null!(&*exec ?= hyper_code::HYPERE_INVALID_ARG); @@ -222,6 +227,10 @@ ffi_fn! { /// /// If ready, returns a task from the executor that has completed. /// + /// To avoid a memory leak, the task must eventually be consumed by + /// `hyper_task_free`, or taken ownership of by `hyper_executor_push` + /// without subsequently being given back by `hyper_executor_poll`. + /// /// If there are no ready tasks, this returns `NULL`. fn hyper_executor_poll(exec: *const hyper_executor) -> *mut hyper_task { let exec = non_null!(&*exec ?= ptr::null_mut()); @@ -272,6 +281,10 @@ impl Future for TaskFuture { ffi_fn! { /// Free a task. + /// + /// This should only be used if the task isn't consumed by + /// `hyper_clientconn_handshake` or taken ownership of by + /// `hyper_executor_push`. fn hyper_task_free(task: *mut hyper_task) { drop(non_null!(Box::from_raw(task) ?= ())); } @@ -284,6 +297,11 @@ ffi_fn! { /// this task. /// /// Use `hyper_task_type` to determine the type of the `void *` return value. + /// + /// To avoid a memory leak, a non-empty return value must eventually be + /// consumed by a function appropriate for its type, one of + /// `hyper_error_free`, `hyper_clientconn_free`, `hyper_response_free`, or + /// `hyper_buf_free`. fn hyper_task_value(task: *mut hyper_task) -> *mut c_void { let task = non_null!(&mut *task ?= ptr::null_mut()); @@ -389,6 +407,9 @@ impl hyper_context<'_> { ffi_fn! { /// Copies a waker out of the task context. + /// + /// To avoid a memory leak, the waker must eventually be consumed by + /// `hyper_waker_free` or `hyper_waker_wake`. fn hyper_context_waker(cx: *mut hyper_context<'_>) -> *mut hyper_waker { let waker = non_null!(&mut *cx ?= ptr::null_mut()).0.waker().clone(); Box::into_raw(Box::new(hyper_waker { waker })) @@ -398,7 +419,10 @@ ffi_fn! { // ===== impl hyper_waker ===== ffi_fn! { - /// Free a waker that hasn't been woken. + /// Free a waker. + /// + /// This should only be used if the request isn't consumed by + /// `hyper_waker_wake`. fn hyper_waker_free(waker: *mut hyper_waker) { drop(non_null!(Box::from_raw(waker) ?= ())); }