diff mbox series

[ustream-ssl,05/12] ustream-openssl: fix BIO_method memory leak

Message ID 20201210154134.794-6-ynezz@true.cz
State Accepted
Delegated to: Petr Štetiar
Headers show
Series fixes, improvements and CI | expand

Commit Message

Petr Štetiar Dec. 10, 2020, 3:41 p.m. UTC
Fixes following issues as reported by clang-12 LeakSanitizer:

 $ uclient-fetch-san -q -O /dev/null 'https://expired.badssl.com/'
  Direct leak of 96 byte(s) in 1 object(s) allocated from:
      #0 0x49716d in malloc (uclient-fetch-san+0x49716d)
      #1 0x7f551cbabe58 in CRYPTO_zalloc (/usr/lib/x86_64-linux-gnu/libcrypto.so.1.1+0x17ae58)

  Indirect leak of 8 byte(s) in 1 object(s) allocated from:
      #0 0x49716d in malloc (uclient-fetch-san+0x49716d)
      #1 0x7f551cbb51c5 in CRYPTO_strdup (/usr/lib/x86_64-linux-gnu/libcrypto.so.1.1+0x1841c5)

  SUMMARY: AddressSanitizer: 104 byte(s) leaked in 2 allocation(s).

and Valgrind:

  $ valgrind --quiet --leak-check=full uclient-fetch -q -O /dev/null 'https://expired.badssl.com/'
  ==1966== 104 (96 direct, 8 indirect) bytes in 1 blocks are definitely lost in loss record 4 of 9
  ==1966==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==1966==    by 0x5FC4E58: CRYPTO_zalloc (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
  ==1966==    by 0x5EF712F: BIO_meth_new (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
  ==1966==    by 0x5C48039: ustream_bio_new (ustream-io-openssl.c:125)
  ==1966==    by 0x5C48039: ustream_set_io (ustream-io-openssl.c:141)
  ==1966==    by 0x5C47CB0: _ustream_ssl_init (ustream-ssl.c:210)
  ==1966==    by 0x4E4117A: uclient_setup_https (uclient-http.c:914)
  ==1966==    by 0x4E4117A: uclient_http_connect (uclient-http.c:936)
  ==1966==    by 0x401FD9: init_request (uclient-fetch.c:333)
  ==1966==    by 0x401E08: main (uclient-fetch.c:745)

Suggested-by: Pan Chen <serial115200@outlook.com>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 ustream-io-openssl.c | 47 ++++++++++++++++++++++----------------------
 ustream-openssl.c    |  7 +++++++
 ustream-openssl.h    |  5 +++++
 3 files changed, 36 insertions(+), 23 deletions(-)
diff mbox series

Patch

diff --git a/ustream-io-openssl.c b/ustream-io-openssl.c
index 606ed4a36f40..7045bb660a36 100644
--- a/ustream-io-openssl.c
+++ b/ustream-io-openssl.c
@@ -48,18 +48,18 @@  s_ustream_free(BIO *b)
 static int
 s_ustream_read(BIO *b, char *buf, int len)
 {
-	struct ustream *s;
+	struct bio_ctx *ctx;
 	char *sbuf;
 	int slen;
 
 	if (!buf || len <= 0)
 		return 0;
 
-	s = (struct ustream *)BIO_get_data(b);
-	if (!s)
+	ctx = (struct bio_ctx *)BIO_get_data(b);
+	if (!ctx || !ctx->stream)
 		return 0;
 
-	sbuf = ustream_get_read_buf(s, &slen);
+	sbuf = ustream_get_read_buf(ctx->stream, &slen);
 
 	BIO_clear_retry_flags(b);
 	if (!slen) {
@@ -71,7 +71,7 @@  s_ustream_read(BIO *b, char *buf, int len)
 		slen = len;
 
 	memcpy(buf, sbuf, slen);
-	ustream_consume(s, slen);
+	ustream_consume(ctx->stream, slen);
 
 	return slen;
 }
@@ -79,19 +79,19 @@  s_ustream_read(BIO *b, char *buf, int len)
 static int
 s_ustream_write(BIO *b, const char *buf, int len)
 {
-	struct ustream *s;
+	struct bio_ctx *ctx;
 
 	if (!buf || len <= 0)
 		return 0;
 
-	s = (struct ustream *)BIO_get_data(b);
-	if (!s)
+	ctx = (struct bio_ctx *)BIO_get_data(b);
+	if (!ctx || !ctx->stream)
 		return 0;
 
-	if (s->write_error)
+	if (ctx->stream->write_error)
 		return len;
 
-	return ustream_write(s, buf, len, false);
+	return ustream_write(ctx->stream, buf, len, false);
 }
 
 static int
@@ -119,19 +119,20 @@  static long s_ustream_ctrl(BIO *b, int cmd, long num, void *ptr)
 static BIO *ustream_bio_new(struct ustream *s)
 {
 	BIO *bio;
-
-	BIO_METHOD *methods_ustream;
-
-	methods_ustream = BIO_meth_new(100 | BIO_TYPE_SOURCE_SINK, "ustream");
-	BIO_meth_set_write(methods_ustream, s_ustream_write);
-	BIO_meth_set_read(methods_ustream, s_ustream_read);
-	BIO_meth_set_puts(methods_ustream, s_ustream_puts);
-	BIO_meth_set_gets(methods_ustream, s_ustream_gets);
-	BIO_meth_set_ctrl(methods_ustream, s_ustream_ctrl);
-	BIO_meth_set_create(methods_ustream, s_ustream_new);
-	BIO_meth_set_destroy(methods_ustream, s_ustream_free);
-	bio = BIO_new(methods_ustream);
-	BIO_set_data(bio, s);
+	struct bio_ctx *ctx = calloc(1, sizeof(struct bio_ctx));
+
+	ctx->stream = s;
+	ctx->meth = BIO_meth_new(100 | BIO_TYPE_SOURCE_SINK, "ustream");
+
+	BIO_meth_set_write(ctx->meth, s_ustream_write);
+	BIO_meth_set_read(ctx->meth, s_ustream_read);
+	BIO_meth_set_puts(ctx->meth, s_ustream_puts);
+	BIO_meth_set_gets(ctx->meth, s_ustream_gets);
+	BIO_meth_set_ctrl(ctx->meth, s_ustream_ctrl);
+	BIO_meth_set_create(ctx->meth, s_ustream_new);
+	BIO_meth_set_destroy(ctx->meth, s_ustream_free);
+	bio = BIO_new(ctx->meth);
+	BIO_set_data(bio, ctx);
 
 	return bio;
 }
diff --git a/ustream-openssl.c b/ustream-openssl.c
index dec2b9f7816d..ad77e721534c 100644
--- a/ustream-openssl.c
+++ b/ustream-openssl.c
@@ -210,8 +210,15 @@  __hidden void __ustream_ssl_context_free(struct ustream_ssl_ctx *ctx)
 
 void __ustream_ssl_session_free(void *ssl)
 {
+	BIO *bio = SSL_get_wbio(ssl);
+	struct bio_ctx *ctx = BIO_get_data(bio);
+
 	SSL_shutdown(ssl);
 	SSL_free(ssl);
+	if (ctx) {
+		BIO_meth_free(ctx->meth);
+		free(ctx);
+	}
 }
 
 static void ustream_ssl_error(struct ustream_ssl *us, int ret)
diff --git a/ustream-openssl.h b/ustream-openssl.h
index 9663d21ffd70..90acc864ddfa 100644
--- a/ustream-openssl.h
+++ b/ustream-openssl.h
@@ -31,6 +31,11 @@ 
 
 void __ustream_ssl_session_free(void *ssl);
 
+struct bio_ctx {
+	BIO_METHOD *meth;
+	struct ustream *stream;
+};
+
 static inline void *__ustream_ssl_session_new(void *ctx)
 {
 	return SSL_new(ctx);