Validate Invalid Network Interface Names at Client Initialization #456
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #456 +/- ##
==========================================
+ Coverage 89.30% 89.68% +0.37%
==========================================
Files 20 20
Lines 6132 6144 +12
==========================================
+ Hits 5476 5510 +34
+ Misses 656 634 -22
|
source/s3_client.c
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks, I have moved code around to get rid of on_early_fail.
source/s3_client.c
Outdated
| client->buffer_pool = aws_s3_buffer_pool_new(allocator, part_size, mem_limit); | ||
|
|
||
| if (client->buffer_pool == NULL) { | ||
| goto on_early_fail; | ||
| } |
There was a problem hiding this comment.
do we still need to move any of this code around? Can we keep it where it was, and change goto on_early_fail -> goto on_error and call it a day?
There was a problem hiding this comment.
Yeah, like one issue is that we need to make sure we acquire the bootstrap reference before Nevermind, we will release the on_error, since on_error releases the reference. There might be other similar issues. client->bootstrap. Looking.
There was a problem hiding this comment.
Thanks, updated with a test.
CRT was returning error during first operation before if it provided with a non-existent network interface name. But with awslabs/aws-c-s3#456, it started failing during the client creation phase. Our tests were written for the previous behaviour and was expecting client creation to succeed even with an invalid network interface. The test is updated to expect errors during client creation. Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
* Update CRT submodules to latest releases Signed-off-by: Burak Varlı <burakvar@amazon.co.uk> * Update non-existent network interface initialization test CRT was returning error during first operation before if it provided with a non-existent network interface name. But with awslabs/aws-c-s3#456, it started failing during the client creation phase. Our tests were written for the previous behaviour and was expecting client creation to succeed even with an invalid network interface. The test is updated to expect errors during client creation. Signed-off-by: Burak Varlı <burakvar@amazon.co.uk> * Update CHANGELOG Co-authored-by: Monthon Klongklaew <monthonk@amazon.com> Signed-off-by: Burak <unexge@gmail.com> --------- Signed-off-by: Burak Varlı <burakvar@amazon.co.uk> Signed-off-by: Burak <unexge@gmail.com> Co-authored-by: Monthon Klongklaew <monthonk@amazon.com>
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.