Conversation
hyper_buf_copy, hyper_buf_bytes, hyper_buf_len and hyper_buf_free dereferenced their pointer argument without the non_null! check that the other FFI functions use. hyper_buf_free(NULL) crashed, unlike every other hyper_*_free, and hyper_buf_copy(NULL, n) read from NULL. hyper_buf_copy now returns NULL for a NULL `buf`, hyper_buf_bytes returns NULL, hyper_buf_len returns 0 and hyper_buf_free does nothing.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
hyper_buf_*functions are the only FFI functions that dereference their pointer argument without thenon_null!check. This follows up on the comment in #4084: "For defensive programming, we can add a null check, the crate has a macro to make that easier."The one that matters most in practice is
hyper_buf_free(NULL). Every otherhyper_*_freedoes nothing when passed NULL, but this one crashes. A NULLhyper_buf *is a normal value in C code, sincehyper_buf_copyis documented to return NULL andhyper_task_valuereturns NULL for an empty task.What changes:
hyper_buf_copy(NULL, len)returns NULL. Before, it readlenbytes from address 0. The doc comment now says so; the first line, whichhyper.huses, is unchanged.hyper_buf_bytes(NULL)returns NULL, andhyper_buf_len(NULL)returns 0.hyper_buf_free(NULL)does nothing.They all use
non_null!, the same ashyper_body_free,hyper_error_codeand the other functions, so debug builds still assert.Tests
I added
ffi::body::tests:buf_copy_bytes_len: round-trips a buffer through copy, bytes, len and free.buf_functions_reject_null: checks NULL handling. It is#[cfg(not(debug_assertions))]becausenon_null!'sdebug_assert!panics inside anffi_fn!without a default, and that aborts.On
master(c954d80),RUSTFLAGS="--cfg hyper_unstable_ffi" cargo test --release --features client,http1,http2,ffi --lib ffi::bodycrashes with SIGSEGV. A test that only callshyper_buf_free(ptr::null_mut())crashes the same way. With this change, it passes (2/2). The CI FFI command,cargo test --features client,http1,http2,ffi --lib(debug), passes 97 tests.rustfmt --check --edition 2021is clean.I used an AI assistant to write this change. I reviewed the code and ran the tests above myself.