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.

Sign up for GitHub

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

Jump to bottom

Validate Invalid Network Interface Names at Client Initialization #456

Merged
merged 13 commits into from
Oct 16, 2024
+125 −39
Merged

Validate Invalid Network Interface Names at Client Initialization #456

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
17ef1d4
fail cilent at init time for invalid network interface name
waahm7 Oct 15, 2024
5c996d0
skip test on windows
waahm7 Oct 15, 2024
ebb3ac0
use else directive
waahm7 Oct 15, 2024
d44a652
lint
waahm7 Oct 15, 2024
609ba14
void allocator
waahm7 Oct 15, 2024
5d22320
lint
waahm7 Oct 15, 2024
3f43e21
Move code around to get rid of early_fail
waahm7 Oct 15, 2024
cbb1b29
Revert "Move code around to get rid of early_fail"
waahm7 Oct 15, 2024
7bb4774
don't move the code around
waahm7 Oct 15, 2024
81109f5
lint
waahm7 Oct 15, 2024
5d0a2e2
add test for early exit
waahm7 Oct 15, 2024
d5e5a94
update test name
waahm7 Oct 15, 2024
f4fb728
revert move
waahm7 Oct 15, 2024
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
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,6 @@ cmake-build*
# js package locks irrelevant to the overall package's security
benchmarks/benchmarks-stack/benchmarks-stack/package-lock.json
benchmarks/dashboard-stack/package-lock.json

# virtual environment
.venv/
86 changes: 54 additions & 32 deletions source/s3_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,10 @@ struct aws_s3_client *aws_s3_client_new(
}
}

if (aws_mutex_init(&client->synced_data.lock) != AWS_OP_SUCCESS) {
goto on_early_fail;
}

client->buffer_pool = aws_s3_buffer_pool_new(allocator, part_size, mem_limit);

if (client->buffer_pool == NULL) {
Expand All @@ -352,6 +356,7 @@ struct aws_s3_client *aws_s3_client_new(
struct aws_s3_buffer_pool_usage_stats pool_usage = aws_s3_buffer_pool_get_usage(client->buffer_pool);

if (client_config->max_part_size > pool_usage.mem_limit) {
aws_s3_buffer_pool_destroy(client->buffer_pool);
Copy link
Contributor

Choose a reason for hiding this comment

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

trying to clean up behind yourself is hard in C
which is why we prefer the "goto error, cleanup everything" pattern,
and why our cleanup/destroy functions are the only ones that tolerate unexpected NULL args

This function already has 2 different "goto error" labels.
Now 1 of those labels you have to be careful to also cleanup a specific variable before you goto it. this is too fragile 😓

Seems better to completely remove the on_early_fail label from this function, and just always try to cleanup everything via goto on_error?

Another option: instead of the on_error: label sharing 90% the same cleanup code as s_s3_client_finish_destroy_default(), have them share code, and tolerate destroying a half-assembled client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have moved code around to get rid of on_early_fail.

AWS_LOGF_ERROR(
AWS_LS_S3_CLIENT,
"Cannot create client from client_config; configured max part size should not exceed memory limit."
Expand All @@ -364,10 +369,6 @@ struct aws_s3_client *aws_s3_client_new(

aws_ref_count_init(&client->ref_count, client, (aws_simple_completion_callback *)s_s3_client_start_destroy);

if (aws_mutex_init(&client->synced_data.lock) != AWS_OP_SUCCESS) {
goto on_early_fail;
}

aws_linked_list_init(&client->synced_data.pending_meta_request_work);
aws_linked_list_init(&client->synced_data.prepared_requests);

Expand Down Expand Up @@ -488,6 +489,44 @@ struct aws_s3_client *aws_s3_client_new(
}
}

client->num_network_interface_names = client_config->num_network_interface_names;
if (client_config->num_network_interface_names > 0) {
AWS_LOGF_DEBUG(
AWS_LS_S3_CLIENT,
"id=%p Client received network interface names array with length %zu.",
(void *)client,
client->num_network_interface_names);
aws_array_list_init_dynamic(
&client->network_interface_names,
client->allocator,
client_config->num_network_interface_names,
sizeof(struct aws_string *));
client->network_interface_names_cursor_array = aws_mem_calloc(
client->allocator, client_config->num_network_interface_names, sizeof(struct aws_byte_cursor));
for (size_t i = 0; i < client_config->num_network_interface_names; i++) {
struct aws_byte_cursor interface_name = client_config->network_interface_names_array[i];
struct aws_string *interface_name_str = aws_string_new_from_cursor(client->allocator, &interface_name);
aws_array_list_push_back(&client->network_interface_names, &interface_name_str);
if (aws_is_network_interface_name_valid(aws_string_c_str(interface_name_str)) == false) {
AWS_LOGF_ERROR(
AWS_LS_S3_CLIENT,
"id=%p network_interface_names_array[%zu]=" PRInSTR " is not valid.",
(void *)client,
i,
AWS_BYTE_CURSOR_PRI(interface_name));
aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
goto on_error;
}
client->network_interface_names_cursor_array[i] = aws_byte_cursor_from_string(interface_name_str);
AWS_LOGF_DEBUG(
AWS_LS_S3_CLIENT,
"id=%p network_interface_names_array[%zu]=" PRInSTR "",
(void *)client,
i,
AWS_BYTE_CURSOR_PRI(client->network_interface_names_cursor_array[i]));
}
}

/* Set up body streaming ELG */
{
uint16_t num_event_loops =
Expand Down Expand Up @@ -579,34 +618,6 @@ struct aws_s3_client *aws_s3_client_new(
*((bool *)&client->enable_read_backpressure) = client_config->enable_read_backpressure;
*((size_t *)&client->initial_read_window) = client_config->initial_read_window;

client->num_network_interface_names = client_config->num_network_interface_names;
if (client_config->num_network_interface_names > 0) {
AWS_LOGF_DEBUG(
AWS_LS_S3_CLIENT,
"id=%p Client received network interface names array with length %zu.",
(void *)client,
client->num_network_interface_names);
aws_array_list_init_dynamic(
&client->network_interface_names,
client->allocator,
client_config->num_network_interface_names,
sizeof(struct aws_string *));
client->network_interface_names_cursor_array = aws_mem_calloc(
client->allocator, client_config->num_network_interface_names, sizeof(struct aws_byte_cursor));
for (size_t i = 0; i < client_config->num_network_interface_names; i++) {
struct aws_byte_cursor interface_name = client_config->network_interface_names_array[i];
struct aws_string *interface_name_str = aws_string_new_from_cursor(client->allocator, &interface_name);
aws_array_list_push_back(&client->network_interface_names, &interface_name_str);
client->network_interface_names_cursor_array[i] = aws_byte_cursor_from_string(interface_name_str);
AWS_LOGF_DEBUG(
AWS_LS_S3_CLIENT,
"id=%p network_interface_names_array[%zu]=" PRInSTR "",
(void *)client,
i,
AWS_BYTE_CURSOR_PRI(client->network_interface_names_cursor_array[i]));
}
}

return client;

on_error:
Expand All @@ -631,6 +642,17 @@ struct aws_s3_client *aws_s3_client_new(
aws_event_loop_group_release(client->client_bootstrap->event_loop_group);
aws_client_bootstrap_release(client->client_bootstrap);
aws_mutex_clean_up(&client->synced_data.lock);

aws_mem_release(client->allocator, client->network_interface_names_cursor_array);
for (size_t i = 0; i < aws_array_list_length(&client->network_interface_names); i++) {
struct aws_string *interface_name = NULL;
aws_array_list_get_at(&client->network_interface_names, &interface_name, i);
aws_string_destroy(interface_name);
}

aws_array_list_clean_up(&client->network_interface_names);
aws_s3_buffer_pool_destroy(client->buffer_pool);

on_early_fail:
aws_mem_release(client->allocator, client);
return NULL;
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ add_test_case(test_s3_abort_multipart_upload_message_new)

add_net_test_case(test_s3_client_create_destroy)
add_net_test_case(test_s3_client_create_error)
add_net_test_case(test_s3_client_create_error_with_invalid_network_interface)
add_net_test_case(test_s3_client_monitoring_options_override)
add_net_test_case(test_s3_client_proxy_ev_settings_override)
add_net_test_case(test_s3_client_tcp_keep_alive_options_override)
Expand Down
28 changes: 28 additions & 0 deletions tests/s3_data_plane_tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,34 @@ static int s_test_s3_client_create_error(struct aws_allocator *allocator, void *
return 0;
}

AWS_TEST_CASE(
test_s3_client_create_error_with_invalid_network_interface,
s_test_s3_client_create_error_with_invalid_network_interface)
static int s_test_s3_client_create_error_with_invalid_network_interface(struct aws_allocator *allocator, void *ctx) {
(void)ctx;

struct aws_s3_tester tester;
AWS_ZERO_STRUCT(tester);
ASSERT_SUCCESS(aws_s3_tester_init(allocator, &tester));

struct aws_s3_client_config client_config;
AWS_ZERO_STRUCT(client_config);
ASSERT_SUCCESS(aws_s3_tester_bind_client(&tester, &client_config, AWS_S3_TESTER_BIND_CLIENT_REGION));

struct aws_byte_cursor *interface_names_array = aws_mem_calloc(allocator, 1, sizeof(struct aws_byte_cursor));
interface_names_array[0] = aws_byte_cursor_from_c_str("invalid-nic");
client_config.network_interface_names_array = interface_names_array;
client_config.num_network_interface_names = 1;

struct aws_s3_client *client = aws_s3_client_new(allocator, &client_config);
ASSERT_TRUE(client == NULL);
tester.bound_to_client = false;

aws_s3_tester_clean_up(&tester);
aws_mem_release(allocator, interface_names_array);
return 0;
}

AWS_TEST_CASE(test_s3_client_monitoring_options_override, s_test_s3_client_monitoring_options_override)
static int s_test_s3_client_monitoring_options_override(struct aws_allocator *allocator, void *ctx) {
(void)ctx;
Expand Down
16 changes: 10 additions & 6 deletions tests/s3_mock_server_tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,16 +169,19 @@ TEST_CASE(multipart_upload_mock_server) {

TEST_CASE(multipart_upload_with_network_interface_names_mock_server) {
(void)ctx;

#if defined(AWS_OS_WINDOWS)
(void)allocator;
return AWS_OP_SKIP;
#else
struct aws_s3_tester tester;
ASSERT_SUCCESS(aws_s3_tester_init(allocator, &tester));
struct aws_byte_cursor *interface_names_array = aws_mem_calloc(allocator, 2, sizeof(struct aws_byte_cursor));
char *localhost_interface = "\0";
#if defined(AWS_OS_APPLE)
# if defined(AWS_OS_APPLE)
localhost_interface = "lo0";
#else
# else
localhost_interface = "lo";
#endif
# endif
interface_names_array[0] = aws_byte_cursor_from_c_str(localhost_interface);
interface_names_array[1] = aws_byte_cursor_from_c_str(localhost_interface);

Expand Down Expand Up @@ -212,11 +215,11 @@ TEST_CASE(multipart_upload_with_network_interface_names_mock_server) {
aws_s3_meta_request_test_results_init(&out_results, allocator);
ASSERT_SUCCESS(aws_s3_tester_send_meta_request_with_options(&tester, &put_options, &out_results));
if (out_results.finished_error_code != 0) {
#if !defined(AWS_OS_APPLE) && !defined(AWS_OS_LINUX)
# if !defined(AWS_OS_APPLE) && !defined(AWS_OS_LINUX)
if (out_results.finished_error_code == AWS_ERROR_PLATFORM_NOT_SUPPORTED) {
return AWS_OP_SKIP;
}
#endif
# endif
ASSERT_TRUE(false, "aws_s3_tester_send_meta_request_with_options(() failed");
}
aws_s3_meta_request_test_results_clean_up(&out_results);
Expand All @@ -225,6 +228,7 @@ TEST_CASE(multipart_upload_with_network_interface_names_mock_server) {
aws_mem_release(allocator, interface_names_array);

return AWS_OP_SUCCESS;
#endif
}

TEST_CASE(multipart_upload_checksum_with_retry_mock_server) {
Expand Down