From c6048af23c577bcf85f15122dd03b65f959c9ecb Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Mon, 21 Jan 2019 17:47:02 +1000 Subject: [PATCH] Fix a memory leak in the mem bio If you use a BIO and set up your own buffer that is not freed, the memory bio will leak the BIO_BUF_MEM object it allocates. The trouble is that the BIO_BUF_MEM is allocated and kept around, but it is not freed if BIO_NOCLOSE is set. The freeing of BIO_BUF_MEM was fairly confusing, simplify things so mem_buf_free only frees the memory buffer and free the BIO_BUF_MEM in mem_free(), where it should be done. Alse add a test for a leak in the memory bio Setting a memory buffer caused a leak. Signed-off-by: Corey Minyard Reviewed-by: Bernd Edlinger Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/8051) --- crypto/bio/bss_mem.c | 24 +++++++------ test/bio_memleak_test.c | 54 ++++++++++++++++++++++++++++++ test/build.info | 6 +++- test/recipes/90-test_bio_memleak.t | 12 +++++++ 4 files changed, 85 insertions(+), 11 deletions(-) create mode 100644 test/bio_memleak_test.c create mode 100644 test/recipes/90-test_bio_memleak.t diff --git a/crypto/bio/bss_mem.c b/crypto/bio/bss_mem.c index ee9ea917a0..89c54b2d53 100644 --- a/crypto/bio/bss_mem.c +++ b/crypto/bio/bss_mem.c @@ -20,7 +20,7 @@ static long mem_ctrl(BIO *h, int cmd, long arg1, void *arg2); static int mem_new(BIO *h); static int secmem_new(BIO *h); static int mem_free(BIO *data); -static int mem_buf_free(BIO *data, int free_all); +static int mem_buf_free(BIO *data); static int mem_buf_sync(BIO *h); static const BIO_METHOD mem_method = { @@ -140,10 +140,20 @@ static int secmem_new(BIO *bi) static int mem_free(BIO *a) { - return mem_buf_free(a, 1); + BIO_BUF_MEM *bb; + + if (a == NULL) + return 0; + + bb = (BIO_BUF_MEM *)a->ptr; + if (!mem_buf_free(a)) + return 0; + OPENSSL_free(bb->readp); + OPENSSL_free(bb); + return 1; } -static int mem_buf_free(BIO *a, int free_all) +static int mem_buf_free(BIO *a) { if (a == NULL) return 0; @@ -155,11 +165,6 @@ static int mem_buf_free(BIO *a, int free_all) if (a->flags & BIO_FLAGS_MEM_RDONLY) b->data = NULL; BUF_MEM_free(b); - if (free_all) { - OPENSSL_free(bb->readp); - OPENSSL_free(bb); - } - a->ptr = NULL; } return 1; } @@ -266,11 +271,10 @@ static long mem_ctrl(BIO *b, int cmd, long num, void *ptr) } break; case BIO_C_SET_BUF_MEM: - mem_buf_free(b, 0); + mem_buf_free(b); b->shutdown = (int)num; bbm->buf = ptr; *bbm->readp = *bbm->buf; - b->ptr = bbm; break; case BIO_C_GET_BUF_MEM_PTR: if (ptr != NULL) { diff --git a/test/bio_memleak_test.c b/test/bio_memleak_test.c new file mode 100644 index 0000000000..36680e30a8 --- /dev/null +++ b/test/bio_memleak_test.c @@ -0,0 +1,54 @@ +/* + * Copyright 2018 The OpenSSL Project Authors. All Rights Reserved. + * + * Licensed under the OpenSSL license (the "License"). You may not use + * this file except in compliance with the License. You can obtain a copy + * in the file LICENSE in the source distribution or at + * https://www.openssl.org/source/license.html + */ +#include +#include +#include +#include + +#include "testutil.h" + +static int test_bio_memleak(void) +{ + int ok = 0; + BIO *bio; + BUF_MEM bufmem; + const char *str = "BIO test\n"; + char buf[100]; + + bio = BIO_new(BIO_s_mem()); + if (bio == NULL) + goto finish; + bufmem.length = strlen(str) + 1; + bufmem.data = (char *) str; + bufmem.max = bufmem.length; + BIO_set_mem_buf(bio, &bufmem, BIO_NOCLOSE); + BIO_set_flags(bio, BIO_FLAGS_MEM_RDONLY); + + if (BIO_read(bio, buf, sizeof(buf)) <= 0) + goto finish; + + ok = strcmp(buf, str) == 0; + +finish: + BIO_free(bio); + return ok; +} + +int global_init(void) +{ + CRYPTO_set_mem_debug(1); + CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ON); + return 1; +} + +int setup_tests(void) +{ + ADD_TEST(test_bio_memleak); + return 1; +} diff --git a/test/build.info b/test/build.info index 962af11eef..2e17a5fb8c 100644 --- a/test/build.info +++ b/test/build.info @@ -42,7 +42,7 @@ INCLUDE_MAIN___test_libtestutil_OLB = /INCLUDE=main packettest asynctest secmemtest srptest memleaktest stack_test \ dtlsv1listentest ct_test threadstest afalgtest d2i_test \ ssl_test_ctx_test ssl_test x509aux cipherlist_test asynciotest \ - bio_callback_test \ + bio_callback_test bio_memleak_test \ bioprinttest sslapitest dtlstest sslcorrupttest bio_enc_test \ pkey_meth_test pkey_meth_kdf_test uitest cipherbytes_test \ asn1_encode_test asn1_decode_test asn1_string_table_test \ @@ -300,6 +300,10 @@ INCLUDE_MAIN___test_libtestutil_OLB = /INCLUDE=main INCLUDE[bio_callback_test]=../include DEPEND[bio_callback_test]=../libcrypto libtestutil.a + SOURCE[bio_memleak_test]=bio_memleak_test.c + INCLUDE[bio_memleak_test]=../include + DEPEND[bio_memleak_test]=../libcrypto libtestutil.a + SOURCE[bioprinttest]=bioprinttest.c INCLUDE[bioprinttest]=../include DEPEND[bioprinttest]=../libcrypto libtestutil.a diff --git a/test/recipes/90-test_bio_memleak.t b/test/recipes/90-test_bio_memleak.t new file mode 100644 index 0000000000..93f7f928a7 --- /dev/null +++ b/test/recipes/90-test_bio_memleak.t @@ -0,0 +1,12 @@ +#! /usr/bin/env perl +# Copyright 2018 The OpenSSL Project Authors. All Rights Reserved. +# +# Licensed under the OpenSSL license (the "License"). You may not use +# this file except in compliance with the License. You can obtain a copy +# in the file LICENSE in the source distribution or at +# https://www.openssl.org/source/license.html + + +use OpenSSL::Test::Simple; + +simple_test("test_bio_memleak", "bio_memleak_test"); -- 2.25.1