From 5f4843191b1766de71c908c304c123b14acd5c9d Mon Sep 17 00:00:00 2001 From: Armin Novak Date: Thu, 8 Nov 2018 12:09:49 +0100 Subject: [PATCH] Replaced BIO_free with BIO_free_all There is no point in using BIO_free with a custom recursion to free up stacked BIOs if there is already BIO_free_all. Using it consistently avoids memory leaks due to stacked BIOs not being recursively freed. --- libfreerdp/core/certificate.c | 2 +- libfreerdp/core/gateway/rdg.c | 29 ++++--------------- libfreerdp/core/tcp.c | 11 ++++--- libfreerdp/core/transport.c | 3 ++ libfreerdp/crypto/crypto.c | 2 +- libfreerdp/crypto/tls.c | 24 ++++++--------- .../libwinpr/sspi/Schannel/schannel_openssl.c | 9 +++--- winpr/tools/makecert/makecert.c | 18 +++++------- 8 files changed, 35 insertions(+), 63 deletions(-) diff --git a/libfreerdp/core/certificate.c b/libfreerdp/core/certificate.c index 0f3b6f0b7..65b5e0707 100644 --- a/libfreerdp/core/certificate.c +++ b/libfreerdp/core/certificate.c @@ -716,7 +716,7 @@ rdpRsaKey* key_new_from_content(const char* keycontent, const char* keyfile) goto out_free; rsa = PEM_read_bio_RSAPrivateKey(bio, NULL, NULL, NULL); - BIO_free(bio); + BIO_free_all(bio); if (!rsa) { diff --git a/libfreerdp/core/gateway/rdg.c b/libfreerdp/core/gateway/rdg.c index ca678cebc..51cb1e228 100644 --- a/libfreerdp/core/gateway/rdg.c +++ b/libfreerdp/core/gateway/rdg.c @@ -1346,30 +1346,11 @@ void rdg_free(rdpRdg* rdg) if (!rdg) return; - if (rdg->tlsOut) - { - tls_free(rdg->tlsOut); - rdg->tlsOut = NULL; - } - - if (rdg->tlsIn) - { - tls_free(rdg->tlsIn); - rdg->tlsIn = NULL; - } - - if (rdg->http) - { - http_context_free(rdg->http); - rdg->http = NULL; - } - - if (rdg->ntlm) - { - ntlm_free(rdg->ntlm); - rdg->ntlm = NULL; - } - + tls_free(rdg->tlsOut); + tls_free(rdg->tlsIn); + http_context_free(rdg->http); + ntlm_free(rdg->ntlm); + BIO_free_all(rdg->frontBio); DeleteCriticalSection(&rdg->writeSection); free(rdg); } diff --git a/libfreerdp/core/tcp.c b/libfreerdp/core/tcp.c index e25c543e8..41ef46915 100644 --- a/libfreerdp/core/tcp.c +++ b/libfreerdp/core/tcp.c @@ -627,16 +627,15 @@ static int transport_bio_buffered_new(BIO* bio) return 1; } +/* Free the buffered BIO. + * Do not free other elements in the BIO stack, + * let BIO_free_all handle that. */ static int transport_bio_buffered_free(BIO* bio) { WINPR_BIO_BUFFERED_SOCKET* ptr = (WINPR_BIO_BUFFERED_SOCKET*) BIO_get_data(bio); - BIO* next_bio = BIO_next(bio); - if (next_bio) - { - BIO_free(next_bio); - BIO_set_next(bio, NULL); - } + if (!ptr) + return 0; ringbuffer_destroy(&ptr->xmitBuffer); free(ptr); diff --git a/libfreerdp/core/transport.c b/libfreerdp/core/transport.c index 9b186d654..a78d32976 100644 --- a/libfreerdp/core/transport.c +++ b/libfreerdp/core/transport.c @@ -1093,6 +1093,9 @@ BOOL transport_disconnect(rdpTransport* transport) if (!transport) return FALSE; + if (transport->rdg && (transport->rdg->frontBio == transport->frontBio)) + transport->frontBio = NULL; + if (transport->tls) { tls_free(transport->tls); diff --git a/libfreerdp/crypto/crypto.c b/libfreerdp/crypto/crypto.c index 73dc91dfa..39875f74d 100644 --- a/libfreerdp/crypto/crypto.c +++ b/libfreerdp/crypto/crypto.c @@ -251,7 +251,7 @@ char* crypto_print_name(X509_NAME* name) BIO_read(outBIO, buffer, size); } - BIO_free(outBIO); + BIO_free_all(outBIO); return buffer; } diff --git a/libfreerdp/crypto/tls.c b/libfreerdp/crypto/tls.c index 8e563b976..e312c42f7 100644 --- a/libfreerdp/crypto/tls.c +++ b/libfreerdp/crypto/tls.c @@ -538,7 +538,7 @@ static BIO* BIO_new_rdp_tls(SSL_CTX* ctx, int client) if (!ssl) { - BIO_free(bio); + BIO_free_all(bio); return NULL; } @@ -935,7 +935,7 @@ BOOL tls_accept(rdpTls* tls, BIO* underlying, rdpSettings* settings) } rsa = PEM_read_bio_RSAPrivateKey(bio, NULL, NULL, NULL); - BIO_free(bio); + BIO_free_all(bio); if (!rsa) { @@ -979,7 +979,7 @@ BOOL tls_accept(rdpTls* tls, BIO* underlying, rdpSettings* settings) } x509 = PEM_read_bio_X509(bio, NULL, NULL, 0); - BIO_free(bio); + BIO_free_all(bio); if (!x509) { @@ -1282,7 +1282,7 @@ fail: if (!rc) free(pemCert); - BIO_free(bio); + BIO_free_all(bio); return rc; } @@ -1612,17 +1612,11 @@ void tls_free(rdpTls* tls) tls->ctx = NULL; } - if (tls->bio) - { - BIO_free(tls->bio); - tls->bio = NULL; - } - - if (tls->underlying) - { - BIO_free(tls->underlying); - tls->underlying = NULL; - } + /* tls->underlying is a stacked BIO under tls->bio. + * BIO_free_all will free recursivly. */ + BIO_free_all(tls->bio); + tls->bio = NULL; + tls->underlying = NULL; if (tls->PublicKey) { diff --git a/winpr/libwinpr/sspi/Schannel/schannel_openssl.c b/winpr/libwinpr/sspi/Schannel/schannel_openssl.c index e208b3255..f54a2222f 100644 --- a/winpr/libwinpr/sspi/Schannel/schannel_openssl.c +++ b/winpr/libwinpr/sspi/Schannel/schannel_openssl.c @@ -125,7 +125,6 @@ int schannel_openssl_client_init(SCHANNEL_OPENSSL* context) { WLog_ERR(TAG, "BIO_new failed"); goto out_bio_read_failed; - return -1; } status = BIO_set_write_buf_size(context->bioRead, SCHANNEL_CB_MAX_TOKEN); @@ -183,10 +182,10 @@ out_write_alloc: out_read_alloc: out_bio_pair: out_set_write_buf_write: - BIO_free(context->bioWrite); + BIO_free_all(context->bioWrite); out_bio_write_failed: out_set_write_buf_read: - BIO_free(context->bioRead); + BIO_free_all(context->bioRead); out_bio_read_failed: SSL_free(context->ssl); out_ssl_new_failed: @@ -324,10 +323,10 @@ out_write_buffer: out_read_buffer: out_bio_pair: out_set_write_buf_write: - BIO_free(context->bioWrite); + BIO_free_all(context->bioWrite); out_bio_write: out_set_write_buf_read: - BIO_free(context->bioRead); + BIO_free_all(context->bioRead); out_bio_read: out_use_certificate: SSL_free(context->ssl); diff --git a/winpr/tools/makecert/makecert.c b/winpr/tools/makecert/makecert.c index 564ba0c9e..d356209b6 100644 --- a/winpr/tools/makecert/makecert.c +++ b/winpr/tools/makecert/makecert.c @@ -767,7 +767,7 @@ int makecert_context_output_certificate_file(MAKECERT_CONTEXT* context, char* pa free(x509_str); x509_str = NULL; - BIO_free(bio); + BIO_free_all(bio); bio = NULL; if (context->pemFormat) @@ -831,9 +831,7 @@ int makecert_context_output_certificate_file(MAKECERT_CONTEXT* context, char* pa ret = 1; out_fail: - - if (bio) - BIO_free(bio); + BIO_free_all(bio); if (fp) fclose(fp); @@ -954,9 +952,7 @@ out_fail: if (fp) fclose(fp); - if (bio) - BIO_free(bio); - + BIO_free_all(bio); free(x509_str); free(filename); free(fullpath); @@ -1176,7 +1172,7 @@ int makecert_context_process(MAKECERT_CONTEXT* context, int argc, char** argv) if (status < 0) { - BIO_free(bio); + BIO_free_all(bio); return -1; } @@ -1185,7 +1181,7 @@ int makecert_context_process(MAKECERT_CONTEXT* context, int argc, char** argv) if (!(x509_str = (BYTE*) malloc(length + 1))) { - BIO_free(bio); + BIO_free_all(bio); return -1; } @@ -1193,7 +1189,7 @@ int makecert_context_process(MAKECERT_CONTEXT* context, int argc, char** argv) if (status < 0) { - BIO_free(bio); + BIO_free_all(bio); free(x509_str); return -1; } @@ -1230,7 +1226,7 @@ int makecert_context_process(MAKECERT_CONTEXT* context, int argc, char** argv) x509_str[length] = '\0'; printf("%s", x509_str); free(x509_str); - BIO_free(bio); + BIO_free_all(bio); } /**