Skip to content

Commit

Permalink
Fix memory leaks in Windows TLS code (#652)
Browse files Browse the repository at this point in the history
**Issue:**

A C++ SDK user found a memory leak in the Windows TLS channel-handler code and submitted the following PR: #651

In evaluating this 2-line fix, I noticed that it sat in the middle of some (preexisting) sloppy error-handling.

**Description of changes:**
- Fix memory leak, as shown in @normanade's PR, by calling `FreeContextBuffer()`.
    - Change it so we **always** call `FreeContextBuffer()` after calling to `AcceptSecurityContext()`/`InitializeSecurityContextA()`. Previously we'd only free the buffer in certain circumstances, and were probably leaking.
- Remove (often sloppy) error-handling that tries to deal with allocation failures of `aws_io_message`. Officially declare that message allocations can't fail.
    - NOTE: Ever since [this change in 2021](awslabs/aws-c-common#830), our allocators will never return `NULL`. They deal with OOM by immediately terminating the program. This was done because error-handling code for allocation failure makes everything so much more complex, and it's seldom tested. We tried for years to handle it. It's not worth the effort.
- Fix some error-handling that forgot to delete the `aws_io_message` after checking its capacity
- In the Windows TLS code, replace some `AWS_ASSERT()` about buffers being large enough with `AWS_FATAL_ASSERT()`. Similarly, add an `AWS_FATAL_ASSERT()` in one place that never checked or asserted before doing the copy.
    - I don't know enough about TLS or Windows APIs to say whether or not these deserve real error-handling. So I'm just preserving the status quo (but crashing instead of doing undefined behavior).

Co-authored-by: @normanade
  • Loading branch information
graebm authored Jul 10, 2024
1 parent 9072f86 commit 0e65ff4
Show file tree
Hide file tree
Showing 9 changed files with 179 additions and 174 deletions.
10 changes: 9 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,15 @@ jobs:
- name: Build ${{ env.PACKAGE_NAME }} + consumers
run: |
python -c "from urllib.request import urlretrieve; urlretrieve('${{ env.BUILDER_HOST }}/${{ env.BUILDER_SOURCE }}/${{ env.BUILDER_VERSION }}/builder.pyz?run=${{ env.RUN }}', 'builder.pyz')"
python builder.pyz build -p ${{ env.PACKAGE_NAME }} --compiler msvc-16
python builder.pyz build -p ${{ env.PACKAGE_NAME }}
windows-debug:
runs-on: windows-2022 # latest
steps:
- name: Build ${{ env.PACKAGE_NAME }} + consumers
run: |
python -c "from urllib.request import urlretrieve; urlretrieve('${{ env.BUILDER_HOST }}/${{ env.BUILDER_SOURCE }}/${{ env.BUILDER_VERSION }}/builder.pyz?run=${{ env.RUN }}', 'builder.pyz')"
python builder.pyz build -p ${{ env.PACKAGE_NAME }} --config Debug
windows-vc14:
runs-on: windows-2019 # windows-2019 is last env with Visual Studio 2015 (v14.0)
Expand Down
4 changes: 2 additions & 2 deletions include/aws/io/channel.h
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ int aws_channel_remove_local_object(
/**
* Acquires a message from the event loop's message pool. size_hint is merely a hint, it may be smaller than you
* requested and you are responsible for checking the bounds of it. If the returned message is not large enough, you
* must send multiple messages.
* must send multiple messages. This cannot fail, it never returns NULL.
*/
AWS_IO_API
struct aws_io_message *aws_channel_acquire_message_from_pool(
Expand Down Expand Up @@ -403,7 +403,7 @@ int aws_channel_slot_send_message(
/**
* Convenience function that invokes aws_channel_acquire_message_from_pool(),
* asking for the largest reasonable DATA message that can be sent in the write direction,
* with upstream overhead accounted for.
* with upstream overhead accounted for. This cannot fail, it never returns NULL.
*/
AWS_IO_API
struct aws_io_message *aws_channel_slot_acquire_max_message_for_write(struct aws_channel_slot *slot);
Expand Down
29 changes: 11 additions & 18 deletions source/channel.c
Original file line number Diff line number Diff line change
Expand Up @@ -422,18 +422,15 @@ struct aws_io_message *aws_channel_acquire_message_from_pool(
size_t size_hint) {

struct aws_io_message *message = aws_message_pool_acquire(channel->msg_pool, message_type, size_hint);

if (AWS_LIKELY(message)) {
message->owning_channel = channel;
AWS_LOGF_TRACE(
AWS_LS_IO_CHANNEL,
"id=%p: acquired message %p of capacity %zu from pool %p. Requested size was %zu",
(void *)channel,
(void *)message,
message->message_data.capacity,
(void *)channel->msg_pool,
size_hint);
}
message->owning_channel = channel;
AWS_LOGF_TRACE(
AWS_LS_IO_CHANNEL,
"id=%p: acquired message %p of capacity %zu from pool %p. Requested size was %zu",
(void *)channel,
(void *)message,
message->message_data.capacity,
(void *)channel->msg_pool,
size_hint);

return message;
}
Expand Down Expand Up @@ -818,12 +815,8 @@ struct aws_io_message *aws_channel_slot_acquire_max_message_for_write(struct aws
AWS_PRECONDITION(aws_channel_thread_is_callers_thread(slot->channel));

const size_t overhead = aws_channel_slot_upstream_message_overhead(slot);
if (overhead >= g_aws_channel_max_fragment_size) {
AWS_LOGF_ERROR(
AWS_LS_IO_CHANNEL, "id=%p: Upstream overhead exceeds channel's max message size.", (void *)slot->channel);
aws_raise_error(AWS_ERROR_INVALID_STATE);
return NULL;
}
AWS_FATAL_ASSERT(
overhead < g_aws_channel_max_fragment_size && "Upstream overhead cannot exceed channel's max message size");

const size_t size_hint = g_aws_channel_max_fragment_size - overhead;
return aws_channel_acquire_message_from_pool(slot->channel, AWS_IO_MESSAGE_APPLICATION_DATA, size_hint);
Expand Down
8 changes: 2 additions & 6 deletions source/darwin/secure_transport_tls_channel_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ static OSStatus s_write_cb(SSLConnectionRef conn, const void *data, size_t *len)
struct aws_io_message *message = aws_channel_acquire_message_from_pool(
handler->parent_slot->channel, AWS_IO_MESSAGE_APPLICATION_DATA, message_size_hint);

if (!message || message->message_data.capacity <= overhead) {
if (message->message_data.capacity <= overhead) {
aws_mem_release(message->allocator, message);
return errSecMemoryError;
}

Expand Down Expand Up @@ -612,11 +613,6 @@ static int s_process_read_message(

struct aws_io_message *outgoing_read_message = aws_channel_acquire_message_from_pool(
slot->channel, AWS_IO_MESSAGE_APPLICATION_DATA, downstream_window - processed);
if (!outgoing_read_message) {
/* even though this is a failure, this handler has taken ownership of the message */
aws_channel_shutdown(secure_transport_handler->parent_slot->channel, aws_last_error());
return AWS_OP_SUCCESS;
}

size_t read = 0;
status = SSLRead(
Expand Down
7 changes: 2 additions & 5 deletions source/message_pool.c
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,10 @@ struct aws_io_message *aws_message_pool_acquire(
break;
default:
AWS_ASSERT(0);
aws_raise_error(AWS_IO_CHANNEL_UNKNOWN_MESSAGE_TYPE);
return NULL;
break;
}

if (!message_wrapper) {
return NULL;
}
AWS_FATAL_ASSERT(message_wrapper);

message_wrapper->message.message_type = message_type;
message_wrapper->message.message_tag = 0;
Expand Down
6 changes: 2 additions & 4 deletions source/s2n/s2n_tls_channel_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,8 @@ static int s_generic_send(struct s2n_handler *handler, struct aws_byte_buf *buf)
struct aws_io_message *message = aws_channel_acquire_message_from_pool(
handler->slot->channel, AWS_IO_MESSAGE_APPLICATION_DATA, message_size_hint);

if (!message || message->message_data.capacity <= overhead) {
if (message->message_data.capacity <= overhead) {
aws_mem_release(message->allocator, message);
errno = ENOMEM;
return -1;
}
Expand Down Expand Up @@ -554,9 +555,6 @@ static int s_s2n_handler_process_read_message(

struct aws_io_message *outgoing_read_message = aws_channel_acquire_message_from_pool(
slot->channel, AWS_IO_MESSAGE_APPLICATION_DATA, downstream_window - processed);
if (!outgoing_read_message) {
return AWS_OP_ERR;
}

ssize_t read = s2n_recv(
s2n_handler->connection,
Expand Down
5 changes: 0 additions & 5 deletions source/socket_channel_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -150,11 +150,6 @@ static void s_do_read(struct socket_handler *socket_handler) {
struct aws_io_message *message = aws_channel_acquire_message_from_pool(
socket_handler->slot->channel, AWS_IO_MESSAGE_APPLICATION_DATA, iter_max_read);

if (!message) {
last_error = aws_last_error();
break;
}

if (aws_socket_read(socket_handler->socket, &message->message_data, &read)) {
last_error = aws_last_error();
aws_mem_release(message->allocator, message);
Expand Down
Loading

0 comments on commit 0e65ff4

Please sign in to comment.