search

signalwire / libstirshaken

C library implementing STIR-shaken STI-SP AS/VS, STI-CA
MIT License
31 stars 23 forks source link

Memory leak in stir_shaken_deinit #135

Open a-rose opened 1 year ago

a-rose commented 1 year ago

Hello,

Using master (5e06ce95965658f3e51f4820938347b1c593cf39), I noticed a memory leak when using stir_shaken_init + stir_shaken_deinit. Valgrind shows the following:

==27431==
==27431== FILE DESCRIPTORS: 3 open at exit.
==27431== Open file descriptor 2: /dev/pts/3
==27431==    <inherited from parent>
==27431==
==27431== Open file descriptor 1: /dev/pts/3
==27431==    <inherited from parent>
==27431==
==27431== Open file descriptor 0: /dev/pts/3
==27431==    <inherited from parent>
==27431==
==27431==
==27431== HEAP SUMMARY:
==27431==     in use at exit: 168 bytes in 3 blocks
==27431==   total heap usage: 7,469 allocs, 7,466 frees, 487,438 bytes allocated
==27431==
==27431== 32 bytes in 1 blocks are still reachable in loss record 1 of 3
==27431==    at 0x483577F: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==27431==    by 0x4C6C8D8: CRYPTO_zalloc (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==27431==    by 0x4CCE96E: OPENSSL_sk_new_reserve (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==27431==    by 0x4CFA6CB: X509V3_EXT_add (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==27431==    by 0x486426B: stir_shaken_register_tnauthlist_extension (stir_shaken_ssl.c:1597)
==27431==    by 0x4867AF9: stir_shaken_init_ssl (stir_shaken_ssl.c:3093)
==27431==    by 0x485B205: stir_shaken_init_basic_and_ssl (stir_shaken.c:55)
==27431==    by 0x487A4F1: ... my program ...
==27431==    by 0x48832AA: ... my program ...
==27431==    by 0x4883454: ... my program ...
==27431==    by 0x109142: main (in ... my program ... )
==27431==
==27431== 32 bytes in 1 blocks are still reachable in loss record 2 of 3
==27431==    at 0x483577F: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==27431==    by 0x4C6C8D8: CRYPTO_zalloc (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==27431==    by 0x4CCE217: ??? (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==27431==    by 0x4CCE3EB: OPENSSL_sk_insert (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==27431==    by 0x4CFA6AB: X509V3_EXT_add (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==27431==    by 0x486426B: stir_shaken_register_tnauthlist_extension (stir_shaken_ssl.c:1597)
==27431==    by 0x4867AF9: stir_shaken_init_ssl (stir_shaken_ssl.c:3093)
==27431==    by 0x485B205: stir_shaken_init_basic_and_ssl (stir_shaken.c:55)
==27431==    by 0x487A4F1: ... my program ...
==27431==    by 0x48832AA: ... my program ...
==27431==    by 0x4883454: ... my program ...
==27431==    by 0x109142: main (in /... my program ...)
==27431==
==27431== 104 bytes in 1 blocks are still reachable in loss record 3 of 3
==27431==    at 0x483577F: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==27431==    by 0x4CFA87C: X509V3_EXT_add_alias (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==27431==    by 0x486426B: stir_shaken_register_tnauthlist_extension (stir_shaken_ssl.c:1597)
==27431==    by 0x4867AF9: stir_shaken_init_ssl (stir_shaken_ssl.c:3093)
==27431==    by 0x485B205: stir_shaken_init_basic_and_ssl (stir_shaken.c:55)
==27431==    by 0x487A4F1: ... my program ...
==27431==    by 0x48832AA: ... my program ...
==27431==    by 0x4883454: ... my program ...
==27431==    by 0x109142: main (in /... my program ...)
==27431==
==27431== LEAK SUMMARY:
==27431==    definitely lost: 0 bytes in 0 blocks
==27431==    indirectly lost: 0 bytes in 0 blocks
==27431==      possibly lost: 0 bytes in 0 blocks
==27431==    still reachable: 168 bytes in 3 blocks
==27431==         suppressed: 0 bytes in 0 blocks
==27431==
==27431== For counts of detected and suppressed errors, rerun with: -v
==27431== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

stir_shaken_init indirectly calls stir_shaken_register_tnauthlist_extension, which uses X509V3_EXT_add_alias. So it seems like X509V3_EXT_cleanup should be called in stir_shaken_deinit. If I add X509V3_EXT_cleanup at the start of stir_shaken_deinit_ssl, valgrind doesn't find leaks anymore.

It seems like a simple change but I'm not sure what the implications are, so I thought I'd open an issue before submitting a PR.