fix: Free pre-existing allocation in s2n_alloc to prevent memory leak…

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

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
 

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 +=
 

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))
 

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;
 

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));
 

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();
 }

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));

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];

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));

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;

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));

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);