diff --git a/tests/cbmc/proofs/s2n_dh_compute_shared_secret_as_server/Makefile b/tests/cbmc/proofs/s2n_dh_compute_shared_secret_as_server/Makefile index 0c2fedd975a..c4cea27a8f0 100644 --- a/tests/cbmc/proofs/s2n_dh_compute_shared_secret_as_server/Makefile +++ b/tests/cbmc/proofs/s2n_dh_compute_shared_secret_as_server/Makefile @@ -45,7 +45,5 @@ UNWINDSET += REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_cleanup_impl REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_free_mlock_impl REMOVE_FUNCTION_BODY += s2n_blob_slice -REMOVE_FUNCTION_BODY += s2n_blob_zero -REMOVE_FUNCTION_BODY += s2n_free include ../Makefile.common diff --git a/tests/cbmc/proofs/s2n_dh_params_to_p_g_Ys/Makefile b/tests/cbmc/proofs/s2n_dh_params_to_p_g_Ys/Makefile index 53b91ba8f0b..57582bfd8fb 100644 --- a/tests/cbmc/proofs/s2n_dh_params_to_p_g_Ys/Makefile +++ b/tests/cbmc/proofs/s2n_dh_params_to_p_g_Ys/Makefile @@ -37,7 +37,6 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_mem.c PROJECT_SOURCES += $(SRCDIR)/utils/s2n_safety.c REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_cleanup_impl -REMOVE_FUNCTION_BODY += s2n_free REMOVE_FUNCTION_BODY += s2n_realloc REMOVE_FUNCTION_BODY += s2n_stuffer_wipe diff --git a/tests/cbmc/proofs/s2n_dup/Makefile b/tests/cbmc/proofs/s2n_dup/Makefile index 7c68184aed1..9fafe78ad96 100644 --- a/tests/cbmc/proofs/s2n_dup/Makefile +++ b/tests/cbmc/proofs/s2n_dup/Makefile @@ -36,8 +36,6 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_safety.c # We abstract these functions because manual inspection demonstrates they are unreachable. REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_cleanup_impl REMOVE_FUNCTION_BODY += s2n_blob_slice -REMOVE_FUNCTION_BODY += s2n_blob_zero -REMOVE_FUNCTION_BODY += s2n_free UNWINDSET += diff --git a/tests/cbmc/proofs/s2n_stuffer_alloc_ro_from_string/Makefile b/tests/cbmc/proofs/s2n_stuffer_alloc_ro_from_string/Makefile index 86f7cf68fc9..538db5f2ebe 100644 --- a/tests/cbmc/proofs/s2n_stuffer_alloc_ro_from_string/Makefile +++ b/tests/cbmc/proofs/s2n_stuffer_alloc_ro_from_string/Makefile @@ -37,10 +37,8 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_safety.c # We abstract these functions because manual inspection demonstrates they are unreachable. REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_cleanup_impl REMOVE_FUNCTION_BODY += s2n_blob_slice -REMOVE_FUNCTION_BODY += s2n_free REMOVE_FUNCTION_BODY += s2n_stuffer_resize REMOVE_FUNCTION_BODY += s2n_add_overflow -REMOVE_FUNCTION_BODY += s2n_blob_zero UNWINDSET += strlen.0:$(call addone,$(MAX_STRING_LEN)) diff --git a/tests/cbmc/proofs/s2n_stuffer_free/s2n_stuffer_free_harness.c b/tests/cbmc/proofs/s2n_stuffer_free/s2n_stuffer_free_harness.c index 9fe8d613714..e10d3a8c33d 100644 --- a/tests/cbmc/proofs/s2n_stuffer_free/s2n_stuffer_free_harness.c +++ b/tests/cbmc/proofs/s2n_stuffer_free/s2n_stuffer_free_harness.c @@ -29,7 +29,7 @@ void s2n_stuffer_free_harness() nondet_s2n_mem_init(); __CPROVER_assume(s2n_result_is_ok(s2n_stuffer_validate(stuffer))); const bool old_alloced = stuffer ? stuffer->alloced : false; - struct s2n_blob old_blob; + struct s2n_blob old_blob = { 0 }; old_blob.data = stuffer ? stuffer->blob.data : NULL; old_blob.growable = stuffer ? stuffer->blob.growable : NULL; diff --git a/tests/unit/s2n_ecc_evp_test.c b/tests/unit/s2n_ecc_evp_test.c index 81ef323dee3..642850b9803 100644 --- a/tests/unit/s2n_ecc_evp_test.c +++ b/tests/unit/s2n_ecc_evp_test.c @@ -239,7 +239,7 @@ int main(int argc, char** argv) struct s2n_ecc_evp_params write_params = { 0 }; struct s2n_ecc_evp_params read_params = { 0 }; struct s2n_stuffer wire = { 0 }; - struct s2n_blob ecdh_params_sent, ecdh_params_received; + struct s2n_blob ecdh_params_sent = { 0 }, ecdh_params_received = { 0 }; EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&wire, 1024)); @@ -277,8 +277,8 @@ int main(int argc, char** argv) struct s2n_ecc_evp_params read_params = { 0 }; struct s2n_ecc_evp_params client_params = { 0 }; struct s2n_stuffer wire = { 0 }; - struct s2n_blob ecdh_params_sent, ecdh_params_received; - struct s2n_blob server_shared_secret, client_shared_secret; + struct s2n_blob ecdh_params_sent = { 0 }, ecdh_params_received = { 0 }; + struct s2n_blob server_shared_secret = { 0 }, client_shared_secret = { 0 }; EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&wire, 1024)); @@ -334,7 +334,7 @@ int main(int argc, char** argv) for (size_t i = 0; i < s2n_all_supported_curves_list_len; i++) { struct s2n_ecc_evp_params server_params = { 0 }, client_params = { 0 }; struct s2n_stuffer wire = { 0 }; - struct s2n_blob server_shared, client_shared, ecdh_params_sent, ecdh_params_received; + struct s2n_blob server_shared = { 0 }, client_shared = { 0 }, ecdh_params_sent, ecdh_params_received; EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&wire, 1024)); diff --git a/tests/unit/s2n_mem_test.c b/tests/unit/s2n_mem_test.c index 2e3a55006c2..3669d65b489 100644 --- a/tests/unit/s2n_mem_test.c +++ b/tests/unit/s2n_mem_test.c @@ -34,6 +34,26 @@ int s2n_failing_mem_free_cb(void *ptr, uint32_t size) return S2N_FAILURE; } +/* Track allocations to detect leaks in s2n_alloc */ +static int s2n_test_malloc_count = 0; +static int s2n_test_free_count = 0; + +static int s2n_test_counting_malloc(void **ptr, uint32_t requested, uint32_t *allocated) +{ + *ptr = malloc(requested); + POSIX_ENSURE(*ptr != NULL, S2N_ERR_ALLOC); + *allocated = requested; + s2n_test_malloc_count++; + return S2N_SUCCESS; +} + +static int s2n_test_counting_free(void *ptr, uint32_t size) +{ + free(ptr); + s2n_test_free_count++; + return S2N_SUCCESS; +} + int main(int argc, char **argv) { BEGIN_TEST(); @@ -141,5 +161,93 @@ int main(int argc, char **argv) EXPECT_OK(s2n_mem_override_callbacks(mem_init_cb, mem_cleanup_cb, mem_malloc_cb, mem_free_cb)); }; + /* Test: s2n_alloc on a blob with pre-existing allocation frees old memory */ + { + s2n_mem_init_callback mem_init_cb = NULL; + s2n_mem_cleanup_callback mem_cleanup_cb = NULL; + s2n_mem_malloc_callback mem_malloc_cb = NULL; + s2n_mem_free_callback mem_free_cb = NULL; + EXPECT_OK(s2n_mem_get_callbacks(&mem_init_cb, &mem_cleanup_cb, &mem_malloc_cb, &mem_free_cb)); + EXPECT_OK(s2n_mem_override_callbacks(mem_init_cb, mem_cleanup_cb, + s2n_test_counting_malloc, s2n_test_counting_free)); + + s2n_test_malloc_count = 0; + s2n_test_free_count = 0; + + struct s2n_blob blob = { 0 }; + + /* First allocation */ + EXPECT_SUCCESS(s2n_alloc(&blob, 32)); + EXPECT_EQUAL(s2n_test_malloc_count, 1); + EXPECT_EQUAL(s2n_test_free_count, 0); + + /* Second allocation on the same blob - must free the first */ + EXPECT_SUCCESS(s2n_alloc(&blob, 64)); + EXPECT_EQUAL(s2n_test_malloc_count, 2); + EXPECT_EQUAL(s2n_test_free_count, 1); + + EXPECT_EQUAL(blob.size, 64); + + /* Final free */ + EXPECT_SUCCESS(s2n_free(&blob)); + EXPECT_EQUAL(s2n_test_malloc_count, 2); + EXPECT_EQUAL(s2n_test_free_count, 2); + + EXPECT_OK(s2n_mem_override_callbacks(mem_init_cb, mem_cleanup_cb, mem_malloc_cb, mem_free_cb)); + }; + + /* Test: repeated s2n_alloc calls don't accumulate leaked memory */ + { + s2n_mem_init_callback mem_init_cb = NULL; + s2n_mem_cleanup_callback mem_cleanup_cb = NULL; + s2n_mem_malloc_callback mem_malloc_cb = NULL; + s2n_mem_free_callback mem_free_cb = NULL; + EXPECT_OK(s2n_mem_get_callbacks(&mem_init_cb, &mem_cleanup_cb, &mem_malloc_cb, &mem_free_cb)); + EXPECT_OK(s2n_mem_override_callbacks(mem_init_cb, mem_cleanup_cb, + s2n_test_counting_malloc, s2n_test_counting_free)); + + s2n_test_malloc_count = 0; + s2n_test_free_count = 0; + + struct s2n_blob blob = { 0 }; + + for (int i = 0; i < 10; i++) { + EXPECT_SUCCESS(s2n_alloc(&blob, (i + 1) * 16)); + } + + /* 10 mallocs, 9 frees (each alloc frees the previous except the first) */ + EXPECT_EQUAL(s2n_test_malloc_count, 10); + EXPECT_EQUAL(s2n_test_free_count, 9); + + EXPECT_SUCCESS(s2n_free(&blob)); + EXPECT_EQUAL(s2n_test_free_count, 10); + + EXPECT_OK(s2n_mem_override_callbacks(mem_init_cb, mem_cleanup_cb, mem_malloc_cb, mem_free_cb)); + }; + + /* Test: s2n_alloc on an empty blob (allocated == 0) does not call free */ + { + s2n_mem_init_callback mem_init_cb = NULL; + s2n_mem_cleanup_callback mem_cleanup_cb = NULL; + s2n_mem_malloc_callback mem_malloc_cb = NULL; + s2n_mem_free_callback mem_free_cb = NULL; + EXPECT_OK(s2n_mem_get_callbacks(&mem_init_cb, &mem_cleanup_cb, &mem_malloc_cb, &mem_free_cb)); + EXPECT_OK(s2n_mem_override_callbacks(mem_init_cb, mem_cleanup_cb, + s2n_test_counting_malloc, s2n_test_counting_free)); + + s2n_test_malloc_count = 0; + s2n_test_free_count = 0; + + struct s2n_blob blob = { 0 }; + + EXPECT_SUCCESS(s2n_alloc(&blob, 32)); + EXPECT_EQUAL(s2n_test_malloc_count, 1); + EXPECT_EQUAL(s2n_test_free_count, 0); + + EXPECT_SUCCESS(s2n_free(&blob)); + + EXPECT_OK(s2n_mem_override_callbacks(mem_init_cb, mem_cleanup_cb, mem_malloc_cb, mem_free_cb)); + }; + END_TEST(); } diff --git a/tests/unit/s2n_quic_support_io_test.c b/tests/unit/s2n_quic_support_io_test.c index 74eb2dad2fb..e21bc7052f2 100644 --- a/tests/unit/s2n_quic_support_io_test.c +++ b/tests/unit/s2n_quic_support_io_test.c @@ -338,7 +338,7 @@ int main(int argc, char **argv) } /* Write test message */ - DEFER_CLEANUP(struct s2n_blob server_hello, s2n_free); + DEFER_CLEANUP(struct s2n_blob server_hello = { 0 }, s2n_free); EXPECT_OK(s2n_write_test_message(&server_hello, TLS_SERVER_HELLO)); /* Setup IO buffers */ @@ -398,7 +398,7 @@ int main(int argc, char **argv) EXPECT_OK(s2n_setup_conn_for_server_hello(conn)); EXPECT_SUCCESS(s2n_connection_set_config(conn, config)); - DEFER_CLEANUP(struct s2n_blob encrypted_extensions, s2n_free); + DEFER_CLEANUP(struct s2n_blob encrypted_extensions = { 0 }, s2n_free); EXPECT_OK(s2n_write_test_message(&encrypted_extensions, TLS_ENCRYPTED_EXTENSIONS)); EXPECT_SUCCESS(s2n_stuffer_write(&input_stuffer, &server_hello)); diff --git a/tests/unit/s2n_stuffer_text_test.c b/tests/unit/s2n_stuffer_text_test.c index 71016f3aef2..4bc156dfda6 100644 --- a/tests/unit/s2n_stuffer_text_test.c +++ b/tests/unit/s2n_stuffer_text_test.c @@ -25,7 +25,7 @@ int main(int argc, char **argv) char c = 0; uint32_t skipped = 0; struct s2n_stuffer stuffer, token; - struct s2n_blob pad_blob, token_blob; + struct s2n_blob pad_blob = { 0 }, token_blob = { 0 }; char text[] = " This is some text\r\n\tmore text"; char fields[] = "one,two,three"; uint8_t pad[1024]; diff --git a/tests/unit/s2n_tls13_hybrid_pq_shared_secret_test.c b/tests/unit/s2n_tls13_hybrid_pq_shared_secret_test.c index 0bdd8996a8f..b917e847356 100644 --- a/tests/unit/s2n_tls13_hybrid_pq_shared_secret_test.c +++ b/tests/unit/s2n_tls13_hybrid_pq_shared_secret_test.c @@ -486,7 +486,7 @@ static int set_up_conns(struct s2n_connection *client_conn, struct s2n_connectio /* Each peer sends its public ECC key to the other */ struct s2n_stuffer wire = { 0 }; - struct s2n_blob server_point_blob, client_point_blob; + struct s2n_blob server_point_blob = { 0 }, client_point_blob = { 0 }; uint16_t share_size = kem_group->curve->share_size; POSIX_GUARD(s2n_stuffer_growable_alloc(&wire, 1024)); diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index 4ed83a8ec20..cd5f22920bf 100644 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -1619,6 +1619,7 @@ int s2n_connection_get_peer_cert_chain(const struct s2n_connection *conn, struct struct s2n_cert *new_node = (struct s2n_cert *) (void *) mem.data; POSIX_ENSURE_REF(new_node); + *new_node = (struct s2n_cert){ 0 }; new_node->next = NULL; *insert = new_node; diff --git a/utils/s2n_mem.c b/utils/s2n_mem.c index 8ac175b0964..6263c432571 100644 --- a/utils/s2n_mem.c +++ b/utils/s2n_mem.c @@ -217,6 +217,7 @@ S2N_RESULT s2n_mem_get_callbacks(s2n_mem_init_callback *mem_init_callback, s2n_m /** * Allocate a new blob on the heap. * + * If the blob already holds an allocation, it is freed first. * The blob will be _growable_. * * This blob owns the underlying memory, which will be freed when `s2n_free` is @@ -226,6 +227,15 @@ int s2n_alloc(struct s2n_blob *b, uint32_t size) { POSIX_ENSURE(initialized, S2N_ERR_NOT_INITIALIZED); POSIX_ENSURE_REF(b); + + /* Free any pre-existing allocation to prevent memory leaks. + * Without this, zeroing the blob below would orphan the previous buffer + * since s2n_realloc only frees when b->allocated is non-zero. + */ + if (b->allocated) { + POSIX_GUARD(s2n_free(b)); + } + const struct s2n_blob temp = { 0 }; *b = temp; POSIX_GUARD(s2n_realloc(b, size)); diff --git a/utils/s2n_mem.h b/utils/s2n_mem.h index 3f04d6d6cf8..84ae00f5f61 100644 --- a/utils/s2n_mem.h +++ b/utils/s2n_mem.h @@ -25,9 +25,12 @@ uint32_t s2n_mem_get_page_size(void); int s2n_mem_cleanup(void); /* - * Generally, s2n_realloc is preferred over s2n_alloc. This is because calling - * s2n_alloc on a blob that already has memory allocated will leak memory. -*/ + * s2n_alloc allocates a new buffer of the requested size. If the blob + * already holds an allocation, it is freed first. + * + * s2n_realloc is preferred when growing an existing blob, as it may + * reuse the buffer without a new allocation. + */ int S2N_RESULT_MUST_USE s2n_alloc(struct s2n_blob *b, uint32_t size); int S2N_RESULT_MUST_USE s2n_realloc(struct s2n_blob *b, uint32_t size); int s2n_free(struct s2n_blob *b);