Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mbedtls: fix memory leak when destroying SSL connection data #626

Closed
wants to merge 1 commit into from
Closed

mbedtls: fix memory leak when destroying SSL connection data #626

wants to merge 1 commit into from

Conversation

raporrass
Copy link
Contributor

Using cURL 7.46 with mbedTLS 2.2.1 I've come across some memory leak according to Valgrind:

==93== 8,940 (1,080 direct, 7,860 indirect) bytes in 10 blocks are definitely lost in loss record 6 of 7
==93==    at 0x402B1E5: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
[...]
==93==    by 0x80C7856: rsa_alloc_wrap (pk_wrap.c:136)
==93==    by 0x80C7233: mbedtls_pk_setup (pk.c:106)
==93==    by 0x80C8CA6: mbedtls_pk_parse_key (pkparse.c:1087)
==93==    by 0x80C8F8D: mbedtls_pk_parse_keyfile (pkparse.c:134)
==93==    by 0x808AF31: mbedtls_connect_common
==93==    by 0x8089D0E: Curl_ssl_connect_nonblocking
==93==    by 0x808E16E: Curl_http_connect

To fix this, I've added some missing mbedtls_xxx_free to Curl_mbedtls_close. I've also substituted some memcpy with the proper mbedtls_xxx_init function when available.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @sasq64, @jay and @bagder to be potential reviewers

@bagder bagder added the TLS label Feb 1, 2016
@bagder
Copy link
Member

bagder commented Feb 9, 2016

Does curl even work with mbedtls atm without this patch? I wanted to try this patch but I get a segmentation fault using git master and mbedtls 2.1.2! Seems to be related to alpn...

$ ./src/curl https://curl.haxx.se/
Segmentation fault
$ gdb --args ./src/curl https://curl.haxx.se/
[snip]
(gdb) run
Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:106
106     ../sysdeps/x86_64/strlen.S: No such file or directory.
(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x000000000049c144 in ssl_write_alpn_ext ()
#2  0x000000000049ce1a in ssl_write_client_hello ()
#3  0x00000000004a1848 in mbedtls_ssl_handshake_client_step ()
#4  0x000000000049837e in mbedtls_ssl_handshake_step ()
#5  0x000000000049840b in mbedtls_ssl_handshake ()
#6  0x0000000000438ae8 in mbedtls_connect_step2 (conn=0x7c6848, sockindex=0)
    at vtls/mbedtls.c:432
#7  0x00000000004396c2 in mbedtls_connect_common (conn=0x7c6848, sockindex=0, 
    nonblocking=true, done=0x7fffffffd81c) at vtls/mbedtls.c:776

@jay
Copy link
Member

jay commented Feb 9, 2016

Does curl even work with mbedtls atm without this patch?

It works with mbedTLS, at least it does in Windows. I don't know about that version though. I'm using 2.2.1 because versions before that had some problems like Mbed-TLS/mbedtls#380. I've been using mbedTLS as my curl SSL backend for about 3 weeks now on my personal computers just to see how it fared. I can connect to https://curl.haxx.se/ without a problem.

curl 7.47.0-DEV (i686-w64-mingw32) libcurl/7.47.0-DEV mbedTLS/2.2.1 zlib/1.2.8 libidn/1.32 libssh2/1.6.0 nghttp2/1.6.0
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp scp sftp smtp smtps telnet tftp
Features: AsynchDNS Debug TrackMemory IDN IPv6 Largefile SSPI Kerberos SPNEGO NTLM SSL libz HTTP2

Do you get a crash on other sites?

@bagder
Copy link
Member

bagder commented Feb 9, 2016

Do you get a crash on other sites?

I do. But I should get an updated version and work out this problem separately since it isn't really related to this PR. I'll file a separate issue if it remains.

@bagder bagder closed this in c62d794 Feb 9, 2016
@bagder
Copy link
Member

bagder commented Feb 9, 2016

thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
TLS
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@raporrass @mention-bot @bagder @jay