Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(bindings): correct poll_flush implementation #4859

Merged
merged 3 commits into from
Oct 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion bindings/rust/s2n-tls/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Result<&mut Self, Error>> {
goatgoose marked this conversation as resolved.
Show resolved Hide resolved
goatgoose marked this conversation as resolved.
Show resolved Hide resolved
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.
Expand Down
6 changes: 4 additions & 2 deletions docs/usage-guide/topics/ch07-io.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
55 changes: 55 additions & 0 deletions tests/unit/s2n_send_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
16 changes: 11 additions & 5 deletions tls/s2n_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,21 @@

#pragma once

#include <s2n.h>

Comment on lines +18 to +19
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely remember that we didn't want to include "s2n.h" in this header, but I don't remember why and I couldn't find the discussion in the original PR.

However, I think this is necessary now because s2n_flush has s2n_blocked_status *blocked as an argument. s2n_blocked_status is currently an anonymous enum we typedef:

s2n-tls/api/s2n.h

Lines 2164 to 2172 in 9cd117f

*/
typedef enum {
S2N_NOT_BLOCKED = 0,
S2N_BLOCKED_ON_READ,
S2N_BLOCKED_ON_WRITE,
S2N_BLOCKED_ON_APPLICATION_INPUT,
S2N_BLOCKED_ON_EARLY_DATA,
} s2n_blocked_status;

The only way I could think to fix this without including "s2n.h" would be to switch s2n_blocked_status from an anonymous enum to a named enum, like in this example. But I'm not a fan of having two different ways to refer to the same enum.

#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 <stdint.h>

/*
* Internal APIs.
*
* These APIs change the behavior of S2N in potentially dangerous ways and should only be
* 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.
*
Expand All @@ -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);
goatgoose marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions tls/s2n_send.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 */
Expand Down
Loading