diff mbox series

[for-9.2,5/9] tests/qtest/migration-test: Free QCRyptoTLSTestCertReq objects

Message ID 20240820144912.320744-6-peter.maydell@linaro.org
State New
Headers show
Series tests/qtest/migration-test: Fix various leaks | expand

Commit Message

Peter Maydell Aug. 20, 2024, 2:49 p.m. UTC
In the migration test we create several TLS certificates with
the TLS_* macros from crypto-tls-x509-helpers.h. These macros
create both a QCryptoTLSCertReq object which must be deinitialized
and also an on-disk certificate file. The migration test currently
removes the on-disk file in test_migrate_tls_x509_finish() but
never deinitializes the QCryptoTLSCertReq, which means that memory
allocated as part of it is leaked:

Indirect leak of 2 byte(s) in 1 object(s) allocated from:
    #0 0x5558ba33712e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f12e) (BuildId: 4c8618f663e538538cad19d35233124cea161491)
    #1 0x7f64afc131f4  (/lib/x86_64-linux-gnu/libtasn1.so.6+0x81f4) (BuildId: 2fde6ecb43c586fe4077118f771077aa1298e7ea)
    #2 0x7f64afc18d58 in asn1_write_value (/lib/x86_64-linux-gnu/libtasn1.so.6+0xdd58) (BuildId: 2fde6ecb43c586fe4077118f771077aa1298e7ea)
    #3 0x7f64af8fc678 in gnutls_x509_crt_set_version (/lib/x86_64-linux-gnu/libgnutls.so.30+0xe7678) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
    #4 0x5558ba470035 in test_tls_generate_cert tests/unit/crypto-tls-x509-helpers.c:234:5
    #5 0x5558ba464e4a in test_migrate_tls_x509_start_common tests/qtest/migration-test.c:1058:5
    #6 0x5558ba462c8a in test_migrate_tls_x509_start_default_host tests/qtest/migration-test.c:1123:12
    #7 0x5558ba45ab40 in test_precopy_common tests/qtest/migration-test.c:1786:21
    #8 0x5558ba450015 in test_precopy_unix_tls_x509_default_host tests/qtest/migration-test.c:2077:5
    #9 0x5558ba46d3c7 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5

(and similar reports).

The only function currently provided to deinit a QCryptoTLSCertReq is
test_tls_discard_cert(), which also removes the on-disk certificate
file.  For the migration tests we need to retain the on-disk files
until we've finished running the test, so the simplest fix is to
provide a new function test_tls_deinit_cert() which does only the
cleanup of the QCryptoTLSCertReq, and call it in the right places.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/unit/crypto-tls-x509-helpers.h |  6 ++++++
 tests/qtest/migration-test.c         |  3 +++
 tests/unit/crypto-tls-x509-helpers.c | 12 ++++++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

Comments

Fabiano Rosas Aug. 22, 2024, 12:43 p.m. UTC | #1
Peter Maydell <peter.maydell@linaro.org> writes:

> In the migration test we create several TLS certificates with
> the TLS_* macros from crypto-tls-x509-helpers.h. These macros
> create both a QCryptoTLSCertReq object which must be deinitialized
> and also an on-disk certificate file. The migration test currently
> removes the on-disk file in test_migrate_tls_x509_finish() but
> never deinitializes the QCryptoTLSCertReq, which means that memory
> allocated as part of it is leaked:
>
> Indirect leak of 2 byte(s) in 1 object(s) allocated from:
>     #0 0x5558ba33712e in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/tests/qtest/migration-test+0x22f12e) (BuildId: 4c8618f663e538538cad19d35233124cea161491)
>     #1 0x7f64afc131f4  (/lib/x86_64-linux-gnu/libtasn1.so.6+0x81f4) (BuildId: 2fde6ecb43c586fe4077118f771077aa1298e7ea)
>     #2 0x7f64afc18d58 in asn1_write_value (/lib/x86_64-linux-gnu/libtasn1.so.6+0xdd58) (BuildId: 2fde6ecb43c586fe4077118f771077aa1298e7ea)
>     #3 0x7f64af8fc678 in gnutls_x509_crt_set_version (/lib/x86_64-linux-gnu/libgnutls.so.30+0xe7678) (BuildId: 97b8f99f392f1fd37b969a7164bcea884e23649b)
>     #4 0x5558ba470035 in test_tls_generate_cert tests/unit/crypto-tls-x509-helpers.c:234:5
>     #5 0x5558ba464e4a in test_migrate_tls_x509_start_common tests/qtest/migration-test.c:1058:5
>     #6 0x5558ba462c8a in test_migrate_tls_x509_start_default_host tests/qtest/migration-test.c:1123:12
>     #7 0x5558ba45ab40 in test_precopy_common tests/qtest/migration-test.c:1786:21
>     #8 0x5558ba450015 in test_precopy_unix_tls_x509_default_host tests/qtest/migration-test.c:2077:5
>     #9 0x5558ba46d3c7 in migration_test_wrapper tests/qtest/migration-helpers.c:456:5
>
> (and similar reports).
>
> The only function currently provided to deinit a QCryptoTLSCertReq is
> test_tls_discard_cert(), which also removes the on-disk certificate
> file.  For the migration tests we need to retain the on-disk files
> until we've finished running the test, so the simplest fix is to
> provide a new function test_tls_deinit_cert() which does only the
> cleanup of the QCryptoTLSCertReq, and call it in the right places.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>
diff mbox series

Patch

diff --git a/tests/unit/crypto-tls-x509-helpers.h b/tests/unit/crypto-tls-x509-helpers.h
index 562c1606535..2a0f7c04fd8 100644
--- a/tests/unit/crypto-tls-x509-helpers.h
+++ b/tests/unit/crypto-tls-x509-helpers.h
@@ -73,6 +73,12 @@  void test_tls_generate_cert(QCryptoTLSTestCertReq *req,
 void test_tls_write_cert_chain(const char *filename,
                                gnutls_x509_crt_t *certs,
                                size_t ncerts);
+/*
+ * Deinitialize the QCryptoTLSTestCertReq, but don't delete the certificate
+ * file on disk. (The caller is then responsible for doing that themselves.
+ */
+void test_tls_deinit_cert(QCryptoTLSTestCertReq *req);
+/* Deinit the QCryptoTLSTestCertReq, and delete the certificate file */
 void test_tls_discard_cert(QCryptoTLSTestCertReq *req);
 
 void test_tls_init(const char *keyfile);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 9811047eeb4..a659609ccb0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1062,12 +1062,15 @@  test_migrate_tls_x509_start_common(QTestState *from,
                                    QCRYPTO_TLS_TEST_CLIENT_HOSTILE_NAME :
                                    QCRYPTO_TLS_TEST_CLIENT_NAME,
                                    data->clientcert);
+        test_tls_deinit_cert(&servercertreq);
     }
 
     TLS_CERT_REQ_SIMPLE_SERVER(clientcertreq, cacertreq,
                                data->servercert,
                                args->certhostname,
                                args->certipaddr);
+    test_tls_deinit_cert(&clientcertreq);
+    test_tls_deinit_cert(&cacertreq);
 
     qtest_qmp_assert_success(from,
                              "{ 'execute': 'object-add',"
diff --git a/tests/unit/crypto-tls-x509-helpers.c b/tests/unit/crypto-tls-x509-helpers.c
index 3e74ec5b5d4..b316155d6a6 100644
--- a/tests/unit/crypto-tls-x509-helpers.c
+++ b/tests/unit/crypto-tls-x509-helpers.c
@@ -502,8 +502,7 @@  void test_tls_write_cert_chain(const char *filename,
     g_free(buffer);
 }
 
-
-void test_tls_discard_cert(QCryptoTLSTestCertReq *req)
+void test_tls_deinit_cert(QCryptoTLSTestCertReq *req)
 {
     if (!req->crt) {
         return;
@@ -511,6 +510,15 @@  void test_tls_discard_cert(QCryptoTLSTestCertReq *req)
 
     gnutls_x509_crt_deinit(req->crt);
     req->crt = NULL;
+}
+
+void test_tls_discard_cert(QCryptoTLSTestCertReq *req)
+{
+    if (!req->crt) {
+        return;
+    }
+
+    test_tls_deinit_cert(req);
 
     if (getenv("QEMU_TEST_DEBUG_CERTS") == NULL) {
         unlink(req->filename);