| Message ID | 20210512131511.1309914-5-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Miscellaneous cleanup patches/small fixes | expand |
Hi, On 12/05/2021 15:15, Arne Schwabe wrote: > This is is a small memory leak as this key is only leaked once > per server start. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> The leak can indeed be seen with valgrind when running the server with --auth-gen-token: ==23481== HEAP SUMMARY: ==23481== in use at exit: 536 bytes in 7 blocks ==23481== total heap usage: 7,460 allocs, 7,453 frees, 1,364,174 bytes allocated ==23481== ==23481== 536 (32 direct, 504 indirect) bytes in 1 blocks are definitely lost in loss record 7 of 7 ==23481== at 0x48397EF: malloc (vg_replace_malloc.c:307) ==23481== by 0x4AEB269: CRYPTO_zalloc (in /usr/lib64/libcrypto.so.1.1) ==23481== by 0x4AE4D67: HMAC_CTX_new (in /usr/lib64/libcrypto.so.1.1) ==23481== by 0x11F3A8: hmac_ctx_new (crypto_openssl.c:996) ==23481== by 0x119B84: init_key_ctx (crypto.c:847) ==23481== by 0x1135F6: auth_token_init_secret (auth_token.c:168) ==23481== by 0x12B117: do_init_auth_token_key (init.c:2685) ==23481== by 0x12B117: do_init_crypto_tls_c1 (init.c:2764) ==23481== by 0x12B117: do_init_crypto_tls (init.c:2802) ==23481== by 0x12FF07: do_init_crypto (init.c:3065) ==23481== by 0x12FF07: init_instance (init.c:4179) ==23481== by 0x130990: init_instance_handle_signals (init.c:3991) ==23481== by 0x140109: tunnel_server_udp (mudp.c:278) ==23481== by 0x14BD28: openvpn_main (openvpn.c:287) ==23481== by 0x4C69E39: (below main) (in /lib64/libc-2.32.so) ==23481== ==23481== LEAK SUMMARY: ==23481== definitely lost: 32 bytes in 1 blocks ==23481== indirectly lost: 504 bytes in 6 blocks ==23481== possibly lost: 0 bytes in 0 blocks ==23481== still reachable: 0 bytes in 0 blocks ==23481== suppressed: 0 bytes in 0 blocks And after applying the patch the leak is gone: ==27117== HEAP SUMMARY: ==27117== in use at exit: 0 bytes in 0 blocks ==27117== total heap usage: 7,447 allocs, 7,447 frees, 1,295,605 bytes allocated ==27117== ==27117== All heap blocks were freed -- no leaks are possible Acked-by: Antonio Quartulli <antonio@oppenvpn.net> Should this go to 2.5 too? Regards,
I won't claim to understand the lifetime of the various copies of
c1.ks.auth_token_key made by code in init.c (to "to.auth_token_key"
or "other contexts") - but it seems that these all are copying
c1.ks.ssl_ctx as well - and if that can be safely free()'ed, the
other one should be fine, too. I also checked that free_key_ctx()
is safe to be used should we have no auth_token_key at all.
Your patch has been applied to the master and release/2.5 branch.
release/2.4 does not have the offending code (no key-based tokens).
I have only compile-tested 2.5 and master ("it should be fine"),
but if not, the server-side test rig will find it later today...
commit fe39156a386bf0dbe79abe43717c84843830e3c0 (master)
commit 6471fd2ab1d07ad24c2c92e7fbda6bd645dd84c8 (release/2.5)
Author: Arne Schwabe
Date: Wed May 12 15:15:06 2021 +0200
Add missing free_key_ctx for auth_token
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20210512131511.1309914-5-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22345.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 1d77a9d42..49c742928 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2520,6 +2520,7 @@ key_schedule_free(struct key_schedule *ks, bool free_ssl_ctx) if (tls_ctx_initialised(&ks->ssl_ctx) && free_ssl_ctx) { tls_ctx_free(&ks->ssl_ctx); + free_key_ctx(&ks->auth_token_key); } CLEAR(*ks); }
This is is a small memory leak as this key is only leaked once per server start. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/init.c | 1 + 1 file changed, 1 insertion(+)