From 778cd8490f07a2747a0b5cfc68357a8fffbdafcd Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Fri, 25 Oct 2024 23:36:35 -0700 Subject: [PATCH] fix(bindings): correct poll_flush implementation (#4859) --- bindings/rust/s2n-tls/src/connection.rs | 12 +++++- docs/usage-guide/topics/ch07-io.md | 6 ++- tests/unit/s2n_send_test.c | 55 +++++++++++++++++++++++++ tls/s2n_internal.h | 16 ++++--- tls/s2n_send.c | 3 ++ 5 files changed, 84 insertions(+), 8 deletions(-) diff --git a/bindings/rust/s2n-tls/src/connection.rs b/bindings/rust/s2n-tls/src/connection.rs index a1fe2b67b4c..0ac0a389650 100644 --- a/bindings/rust/s2n-tls/src/connection.rs +++ b/bindings/rust/s2n-tls/src/connection.rs @@ -650,8 +650,18 @@ impl Connection { } /// Attempts to flush any data previously buffered by a call to [send](`Self::poll_send`). + /// + /// poll_flush can only flush data that s2n-tls has already encrypted and + /// buffered for sending. poll_send may need to be called again to fully send + /// all data. See the [Usage Guide](https://github.com/aws/s2n-tls/blob/main/docs/usage-guide/topics/ch07-io.md) + /// for more details. pub fn poll_flush(&mut self) -> Poll> { - self.poll_send(&[0; 0]).map_ok(|_| self) + let mut blocked = s2n_blocked_status::NOT_BLOCKED; + unsafe { + s2n_flush(self.connection.as_ptr(), &mut blocked) + .into_poll() + .map_ok(|_| self) + } } /// Gets the number of bytes that are currently available in the buffer to be read. diff --git a/docs/usage-guide/topics/ch07-io.md b/docs/usage-guide/topics/ch07-io.md index 0040559147b..0b93391e37c 100644 --- a/docs/usage-guide/topics/ch07-io.md +++ b/docs/usage-guide/topics/ch07-io.md @@ -103,7 +103,7 @@ connections aborted while active. A single call to `s2n_send()` may involve multiple system calls to write the provided application data. s2n-tls breaks the application data into fixed-sized -records before encryption, and calls write for each record. +records before encryption, then calls write for each record. [See the record size documentation for how record size may impact performance](./ch08-record-sizes.md). In non-blocking mode, `s2n_send()` will send data from the provided buffer and return the number of @@ -117,7 +117,9 @@ able to send more data. This can be determined by using methods like [`poll`](https://linux.die.net/man/2/poll) or [`select`](https://linux.die.net/man/2/select). Unlike OpenSSL, repeated calls to `s2n_send()` should not duplicate the original -parameters, but should update the inputs per the indication of size written. +parameters, but should update the inputs per the indication of size written. s2n-tls +will attempt to sanity check that the inputs are properly updated between calls. +Because of those sanity checks, a zero-length send call cannot be used as a flushing mechanism. `s2n_sendv_with_offset()` behaves like `s2n_send()`, but supports vectorized buffers. The offset input should be updated between calls to reflect the data already written. diff --git a/tests/unit/s2n_send_test.c b/tests/unit/s2n_send_test.c index c536a48d24f..13a685a72fb 100644 --- a/tests/unit/s2n_send_test.c +++ b/tests/unit/s2n_send_test.c @@ -750,5 +750,60 @@ int main(int argc, char **argv) } }; + /* Test: s2n_flush is necessary and not achievable with s2n_send */ + { + bool use_send[] = { true, false }; + + /* To reproduce the problematic scenario, we need to block on + * sending a record during a multi-record write. + */ + struct s2n_send_result results[] = { + BLOCK_SEND_RESULT, + OK_SEND_RESULT, + OK_SEND_RESULT, + OK_SEND_RESULT, + }; + + for (size_t i = 0; i < s2n_array_len(use_send); i++) { + DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT), + s2n_connection_ptr_free); + EXPECT_NOT_NULL(conn); + EXPECT_OK(s2n_connection_set_secrets(conn)); + + struct s2n_send_context context = { + .results = results, + .results_len = s2n_array_len(results) + }; + EXPECT_SUCCESS(s2n_connection_set_send_cb(conn, s2n_test_send_cb)); + EXPECT_SUCCESS(s2n_connection_set_send_ctx(conn, (void *) &context)); + + s2n_blocked_status blocked = 0; + + /* First attempt blocks */ + EXPECT_FAILURE_WITH_ERRNO(s2n_send(conn, large_test_data, sizeof(large_test_data), &blocked), + S2N_ERR_IO_BLOCKED); + EXPECT_EQUAL(blocked, S2N_BLOCKED_ON_WRITE); + EXPECT_EQUAL(context.bytes_sent, 0); + + /* For our control case, we attempt to use a zero-length send as flush */ + if (use_send[i]) { + EXPECT_FAILURE_WITH_ERRNO( + s2n_send(conn, large_test_data, 0, &blocked), + S2N_ERR_SEND_SIZE); + continue; + } + + /* Unlike the zero-length send, s2n_flush succeeds */ + EXPECT_SUCCESS(s2n_flush(conn, &blocked)); + EXPECT_EQUAL(context.bytes_sent, max_frag_bytes_sent[S2N_MFL_DEFAULT]); + + /* We can also successfully finish sending */ + EXPECT_EQUAL( + s2n_send(conn, large_test_data, sizeof(large_test_data), &blocked), + sizeof(large_test_data)); + EXPECT_EQUAL(context.bytes_sent, large_test_data_bytes_sent); + } + }; + END_TEST(); } diff --git a/tls/s2n_internal.h b/tls/s2n_internal.h index c1ab6a3e700..7c51f656a24 100644 --- a/tls/s2n_internal.h +++ b/tls/s2n_internal.h @@ -15,14 +15,14 @@ #pragma once +#include + #if ((__GNUC__ >= 4) || defined(__clang__)) && defined(S2N_EXPORTS) #define S2N_PRIVATE_API __attribute__((visibility("default"))) #else #define S2N_PRIVATE_API #endif /* __GNUC__ >= 4 || defined(__clang__) */ -#include - /* * Internal APIs. * @@ -30,9 +30,6 @@ * used for testing purposes. All Internal APIs are subject to change without notice. */ -struct s2n_config; -struct s2n_connection; - /* * Gets the config set on the connection. * @@ -53,3 +50,12 @@ S2N_PRIVATE_API int s2n_connection_get_config(struct s2n_connection *conn, struc */ S2N_PRIVATE_API int s2n_config_add_cert_chain(struct s2n_config *config, uint8_t *cert_chain_pem, uint32_t cert_chain_pem_size); + +/* + * Attempts to flush any data buffered for sending. + * + * This method is not sufficient to complete a previous partial send. It can only + * attempt to flush data that has been encrypted and buffered, not data that + * is still waiting for encryption. + */ +S2N_PRIVATE_API int s2n_flush(struct s2n_connection *conn, s2n_blocked_status *blocked); diff --git a/tls/s2n_send.c b/tls/s2n_send.c index 98ce35b96d8..042a3e8d5d5 100644 --- a/tls/s2n_send.c +++ b/tls/s2n_send.c @@ -24,6 +24,7 @@ #include "tls/s2n_cipher_suites.h" #include "tls/s2n_connection.h" #include "tls/s2n_handshake.h" +#include "tls/s2n_internal.h" #include "tls/s2n_ktls.h" #include "tls/s2n_post_handshake.h" #include "tls/s2n_record.h" @@ -82,6 +83,8 @@ bool s2n_should_flush(struct s2n_connection *conn, ssize_t total_message_size) int s2n_flush(struct s2n_connection *conn, s2n_blocked_status *blocked) { + POSIX_ENSURE_REF(conn); + POSIX_ENSURE_REF(blocked); *blocked = S2N_BLOCKED_ON_WRITE; /* Write any data that's already pending */