Refactor Callback Tests for Improved Memory Management

Refactor the callback test code to replace global variables with local structures, enhancing memory management and reducing reliance on redundant cleanup logic.

Using a local struct containing a magic number and result flag to ensure the correct handling of user data and to verify that the callback function is invoked at least once during the test.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/25330)

diff --git a/test/bio_pw_callback_test.c b/test/bio_pw_callback_test.c
index 56c0ceab713017ae11ebeeeccc293399ab08e613..917021bd48d9efa1b3107e793819214518a7d701 100644 (file)
--- a/test/bio_pw_callback_test.c
+++ b/test/bio_pw_callback_test.c
@@ -14,7 +14,8 @@
 
 /* dummy data that needs to be passed to the callback */
 typedef struct CallbackData {
-    int dummy;
+    char magic;
+    int result;
 } CALLBACK_DATA;
 
 /* constants */
@@ -24,16 +25,11 @@ static char *a0a_password = "aaaaaaaa\0aaaaaaaa";
 static int a0a_password_len = 17;
 static char *a0b_password = "aaaaaaaa\0bbbbbbbb";
 static int a0b_password_len = 17;
+static char cb_magic = 'p';
 
 /* shared working data for all tests */
 static char *key_file = NULL;
 static EVP_PKEY *original_pkey = NULL;
-static BUF_MEM *encrypted_key_data = NULL;
-static int encrypted_key_data_size = 0;
-static BIO *bio = NULL;
-static EVP_PKEY *pkey = NULL;
-static CALLBACK_DATA *callback_data = NULL;
-static int callback_ret = 0;
 
 /* the test performed by the callback */
 typedef enum CallbackTest {
@@ -76,17 +72,6 @@ const OPTIONS *test_get_options(void)
     return test_options;
 }
 
-static void cleanup_after_test(void)
-{
-    free(encrypted_key_data);
-    encrypted_key_data = NULL;
-    encrypted_key_data_size = 0;
-    BIO_free(bio);
-    bio = NULL;
-    EVP_PKEY_free(pkey);
-    pkey = NULL;
-}
-
 static int callback_copy_password(char *buf, int size)
 {
     int ret = -1;
@@ -127,10 +112,13 @@ static int callback_copy_password(char *buf, int size)
 
 static int read_callback(char *buf, int size, int rwflag, void *u)
 {
+    CALLBACK_DATA *cb_data = (CALLBACK_DATA *)u;
     int ret = -1;
 
     /* basic verification of the received data */
-    if (!TEST_ptr_eq(u, callback_data))
+    if (!TEST_ptr(cb_data))
+        goto err;
+    if (!TEST_char_eq(cb_data->magic, cb_magic))
         goto err;
     if (!TEST_ptr(buf))
         goto err;
@@ -139,17 +127,20 @@ static int read_callback(char *buf, int size, int rwflag, void *u)
     if (!TEST_int_eq(rwflag, 0))
         goto err;
     ret = callback_copy_password(buf, size);
-    callback_ret = 1;
+    cb_data->result = 1;
 err:
     return ret;
 }
 
 static int write_callback(char *buf, int size, int rwflag, void *u)
 {
+    CALLBACK_DATA *cb_data = (CALLBACK_DATA *)u;
     int ret = -1;
 
     /* basic verification of the received data */
-    if (!TEST_ptr_eq(u, callback_data))
+    if (!TEST_ptr(cb_data))
+        goto err;
+    if (!TEST_char_eq(cb_data->magic, cb_magic))
         goto err;
     if (!TEST_ptr(buf))
         goto err;
@@ -158,41 +149,45 @@ static int write_callback(char *buf, int size, int rwflag, void *u)
     if (!TEST_int_eq(rwflag, 1))
         goto err;
     ret = callback_copy_password(buf, size);
-    callback_ret = 1;
+    cb_data->result = 1;
 err:
     return ret;
 }
 
-static int re_encrypt_key(KEY_ENCODING key_encoding)
+static int re_encrypt_key(char **enc_data, int *enc_data_size,
+                          KEY_ENCODING key_encoding)
 {
+    CALLBACK_DATA cb_data;
     int w_ret = 0;
-    int ret = 0;
     BUF_MEM *bptr = NULL;
+    BIO *bio = NULL;
+    int ret = 0;
 
-    free(encrypted_key_data);
-    encrypted_key_data = NULL;
-    encrypted_key_data_size = 0;
+    if (!TEST_ptr(enc_data))
+        goto err;
+    if (!TEST_ptr(enc_data_size))
+        goto err;
     if (!TEST_ptr(bio = BIO_new(BIO_s_mem())))
         goto err;
-    callback_ret = 0;
+    cb_data.magic = cb_magic;
+    cb_data.result = 0;
     switch (key_encoding) {
     case KE_PEM:
-        w_ret = PEM_write_bio_PrivateKey(bio, original_pkey,
-                                         EVP_aes_256_cbc(),
-                                         NULL, 0, write_callback,
-                                         callback_data);
+        w_ret = PEM_write_bio_PrivateKey(bio, original_pkey, EVP_aes_256_cbc(),
+                                         NULL, 0, write_callback, &cb_data);
         break;
     case KE_PKCS8:
         w_ret = i2d_PKCS8PrivateKey_bio(bio, original_pkey, EVP_aes_256_cbc(),
-                                        NULL, 0, write_callback,
-                                        callback_data);
+                                        NULL, 0, write_callback, &cb_data);
         break;
     }
     if (!TEST_int_ne(w_ret, 0))
         goto err;
-    if (!TEST_int_eq(callback_ret, 1))
+    if (!TEST_char_eq(cb_data.magic, cb_magic))
         goto err;
-    encrypted_key_data_size = BIO_get_mem_data(bio, &encrypted_key_data);
+    if (!TEST_int_eq(cb_data.result, 1))
+        goto err;
+    *enc_data_size = BIO_get_mem_data(bio, enc_data);
     BIO_get_mem_ptr(bio, &bptr);
     if (!BIO_set_close(bio, BIO_NOCLOSE))
         goto err;
@@ -201,30 +196,29 @@ static int re_encrypt_key(KEY_ENCODING key_encoding)
 err:
     BUF_MEM_free(bptr);
     BIO_free(bio);
-    bio = NULL;
     return ret;
 }
 
-static int decrypt_key(KEY_ENCODING key_encoding,
+static int decrypt_key(char *enc_data, int enc_data_size,
+                       KEY_ENCODING key_encoding,
                        EXPECTED_RESULT expected_result)
 {
+    CALLBACK_DATA cb_data;
     EVP_PKEY *r_ret = NULL;
+    BIO *bio = NULL;
+    EVP_PKEY *pkey = NULL;
     int ret = 0;
 
-    if (!TEST_ptr(bio = BIO_new_mem_buf(encrypted_key_data,
-                                        encrypted_key_data_size)))
+    if (!TEST_ptr(bio = BIO_new_mem_buf(enc_data, enc_data_size)))
         goto err;
-    EVP_PKEY_free(pkey);
-    pkey = NULL;
-    callback_ret = 0;
+    cb_data.magic = cb_magic;
+    cb_data.result = 0;
     switch (key_encoding) {
     case KE_PEM:
-        r_ret = PEM_read_bio_PrivateKey(bio, &pkey, read_callback,
-                                        callback_data);
+        r_ret = PEM_read_bio_PrivateKey(bio, &pkey, read_callback, &cb_data);
         break;
     case KE_PKCS8:
-        r_ret = d2i_PKCS8PrivateKey_bio(bio, &pkey, read_callback,
-                                        callback_data);
+        r_ret = d2i_PKCS8PrivateKey_bio(bio, &pkey, read_callback, &cb_data);
         break;
     }
     if (expected_result == ER_SUCCESS) {
@@ -234,14 +228,14 @@ static int decrypt_key(KEY_ENCODING key_encoding,
         if (!TEST_ptr_null(r_ret))
             goto err;
     }
-    if (!TEST_int_eq(callback_ret, 1))
+    if (!TEST_char_eq(cb_data.magic, cb_magic))
+        goto err;
+    if (!TEST_int_eq(cb_data.result, 1))
         goto err;
     ret = 1;
 err:
     EVP_PKEY_free(pkey);
-    pkey = NULL;
     BIO_free(bio);
-    bio = NULL;
     return ret;
 }
 
@@ -249,22 +243,20 @@ static int full_cycle_test(KEY_ENCODING key_encoding, CALLBACK_TEST write_test,
                            CALLBACK_TEST read_test,
                            EXPECTED_RESULT expected_read_result)
 {
+    char *enc_data = NULL;
+    int enc_data_size = 0;
     int ret = 0;
 
     callback_test = write_test;
-    callback_ret = 0;
-    if (!re_encrypt_key(key_encoding))
-        goto err;
-    if (!TEST_int_eq(callback_ret, 1))
+    if (!re_encrypt_key(&enc_data, &enc_data_size, key_encoding))
         goto err;
     callback_test = read_test;
-    if (!decrypt_key(key_encoding, expected_read_result))
-        goto err;
-    if (!TEST_int_eq(callback_ret, 1))
+    if (!decrypt_key(enc_data, enc_data_size, key_encoding,
+                     expected_read_result))
         goto err;
     ret = 1;
 err:
-    cleanup_after_test();
+    OPENSSL_free(enc_data);
     return ret;
 }
 
@@ -364,6 +356,7 @@ static int callback_original_pw(char *buf, int size, int rwflag, void *u)
 int setup_tests(void)
 {
     OPTION_CHOICE o;
+    BIO *bio = NULL;
 
     while ((o = opt_next()) != OPT_EOF) {
         switch (o) {
@@ -378,10 +371,6 @@ int setup_tests(void)
         }
     }
 
-    /* create dummy callback data for verification */
-    callback_data = OPENSSL_malloc(sizeof(CALLBACK_DATA));
-    memset(callback_data, 0, sizeof(CALLBACK_DATA));
-
     /* read the original key */
     if (!TEST_ptr(bio = BIO_new_file(key_file, "r")))
         return 0;
@@ -389,7 +378,6 @@ int setup_tests(void)
                                           callback_original_pw, NULL)))
         return 0;
     BIO_free(bio);
-    bio = NULL;
 
     /* add all tests */
     ADD_TEST(test_pem_negative);
@@ -413,7 +401,5 @@ int setup_tests(void)
 
 void cleanup_tests(void)
 {
-    BUF_MEM_free(encrypted_key_data);
-    OPENSSL_free(callback_data);
     EVP_PKEY_free(original_pkey);
 }