Hi,
Since e3c07527f3 (crypto/hash: Implement and use new hash API,
2024-10-08), we've been seeing a memory leak in two check-unit tests:
test-crypto-hash and test-crypto-ivgen. Looking a bit further to try and
plug the leak, I got a bit confused regarding how the result crypto
buffer is handled. Looks like we are allocating different sizes at two
different places, and I'm unsure if these places follow the same
convention or could be breaking expectations from one another...
So this is the report from LeakSanitizer:
qemu:unit / test-crypto-hash ERROR
Direct leak of 180 byte(s) in 5 object(s) allocated from:
#0 0x7fdb016e0a06 in __interceptor_calloc
../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
#1 0x555bdb5892f7 in g_malloc0 ../subprojects/glib/glib/gmem.c:155
#2 0x555bdb5895d6 in g_malloc0_n ../subprojects/glib/glib/gmem.c:387
#3 0x555bdb3fede2 in test_hash_prealloc ../tests/unit/test-crypto-hash.c:136
#4 0x555bdb59fb96 in test_case_run
../subprojects/glib/glib/gtestutils.c:2930
#5 0x555bdb59ff81 in g_test_run_suite_internal
../subprojects/glib/glib/gtestutils.c:3018
#6 0x555bdb5a00a9 in g_test_run_suite_internal
../subprojects/glib/glib/gtestutils.c:3035
#7 0x555bdb5a00a9 in g_test_run_suite_internal
../subprojects/glib/glib/gtestutils.c:3035
#8 0x555bdb5a02c8 in g_test_run_suite
../subprojects/glib/glib/gtestutils.c:3112
#9 0x555bdb59e9e2 in g_test_run ../subprojects/glib/glib/gtestutils.c:2231
#10 0x555bdb401b27 in main ../tests/unit/test-crypto-hash.c:301
#11 0x7fdb00a75082 in __libc_start_main ../csu/libc-start.c:308
qemu:unit / test-crypto-ivgen ERROR
Direct leak of 96 byte(s) in 3 object(s) allocated from:
#0 0x7fd572092a06 in __interceptor_calloc
../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
#1 0x565440094e7a in g_malloc0 ../subprojects/glib/glib/gmem.c:155
#2 0x565440095159 in g_malloc0_n ../subprojects/glib/glib/gmem.c:387
#3 0x56543fede89e in qcrypto_ivgen_essiv_init ../crypto/ivgen-essiv.c:44
#4 0x56543fedfc8a in qcrypto_ivgen_new ../crypto/ivgen.c:58
#5 0x56543feefdd3 in test_ivgen ../tests/unit/test-crypto-ivgen.c:147
#6 0x5654400ab719 in test_case_run
../subprojects/glib/glib/gtestutils.c:2930
#7 0x5654400abb04 in g_test_run_suite_internal
../subprojects/glib/glib/gtestutils.c:3018
#8 0x5654400abc2c in g_test_run_suite_internal
../subprojects/glib/glib/gtestutils.c:3035
#9 0x5654400abc2c in g_test_run_suite_internal
../subprojects/glib/glib/gtestutils.c:3035
#10 0x5654400abc2c in g_test_run_suite_internal
../subprojects/glib/glib/gtestutils.c:3035
#11 0x5654400abe4b in g_test_run_suite
../subprojects/glib/glib/gtestutils.c:3112
#12 0x5654400aa565 in g_test_run ../subprojects/glib/glib/gtestutils.c:2231
#13 0x56543fef0654 in main ../tests/unit/test-crypto-ivgen.c:177
#14 0x7fd571427082 in __libc_start_main ../csu/libc-start.c:308
At qcrypto_ivgen_essiv_init(), we allocate 'salt' with:
/* Salt must be larger of hash size or key size */
salt = g_new0(uint8_t, MAX(nhash, nsalt));
The address of 'salt' is passed down the stack as 'result':
qcrypto_ivgen_essiv_init()
|> qcrypto_hash_bytes()
|> qcrypto_hash_bytesv()
|> qcrypto_hash_finalize_bytes()
|> qcrypto_glib_hash_finalize()
Which is reassigned to a new memory (of potentially different size) at
qcrypto_glib_hash_finalize():
*result_len = ret;
*result = g_new(uint8_t, *result_len);
The reference to the original address gets lost and never free'd, so we
get the mem leak error. I'm confused as to why we start with one size
and then realloc later here. Also, how should this mem leak be fixed?
Would it be sufficient to avoid the g_checksum_type_get_length() and
rellocation here if 'result' is already set?
As mentioned before, the mem leak bisects down to e3c07527f3.
Thanks,
Matheus
