[Openvpn-devel] Fix memory leak in buffer unit tests

Message ID 1512724338-22197-1-git-send-email-steffan@karger.me
State Accepted
Headers show
Series
  • [Openvpn-devel] Fix memory leak in buffer unit tests
Related show

Commit Message

Steffan Karger Dec. 8, 2017, 9:12 a.m.
buffer_list_push_data does not take ownership of the memory, so just feed
it stack data to plug the leak.

Signed-off-by: Steffan Karger <steffan@karger.me>
---
 tests/unit_tests/openvpn/test_buffer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Antonio Quartulli Dec. 29, 2017, 9:39 a.m. | #1
Hi,

On 08/12/17 17:12, Steffan Karger wrote:
> buffer_list_push_data does not take ownership of the memory, so just feed
> it stack data to plug the leak.
> 
> Signed-off-by: Steffan Karger <steffan@karger.me>

as reported by Steffan, buffer_list_push_data() takes the content of the
buffer passed as argument and copies it in a new buffer. Thus the
ownership of that argument still belongs to the caller which is
responsible for releasing it.

Using stack data just fixes the issue.

Acked-by: Antonio Quartulli <a@unstable.cc>

Aside from this patch, I just noted that buffer_list_push_data() suffers
from the same problem as buffer_list_aggregate_separator().
The caller could pass size=0 as argument, thus leading to a 0-byte
malloc. This should probably be fixed with an additional patch.

Cheers,

> ---
>  tests/unit_tests/openvpn/test_buffer.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c
> index ba5aa67..c2b7f30 100644
> --- a/tests/unit_tests/openvpn/test_buffer.c
> +++ b/tests/unit_tests/openvpn/test_buffer.c
> @@ -77,10 +77,9 @@ static int test_buffer_list_setup(void **state)
>      buffer_list_push(ctx->zero_length_strings, "");
>  
>      ctx->empty_buffers = buffer_list_new(2);
> -    uint8_t *data1 = malloc(1);
> -    uint8_t *data2 = malloc(1);
> -    buffer_list_push_data(ctx->empty_buffers, data1, 0);
> -    buffer_list_push_data(ctx->empty_buffers, data2, 0);
> +    uint8_t data = 0;
> +    buffer_list_push_data(ctx->empty_buffers, &data, 0);
> +    buffer_list_push_data(ctx->empty_buffers, &data, 0);
>  
>      *state = ctx;
>      return 0;
>
Gert Doering Jan. 9, 2018, 2:13 p.m. | #2
Your patch has been applied to the master and release/2.4 branch.

commit 2c7c760dfbddbc9cf348bce06fa922c1217a2039 (master)
commit 888204a1e9a4278a62e9a1cb0ea1c0cdff95473e (release/2.4)
Author: Steffan Karger
Date:   Fri Dec 8 10:12:18 2017 +0100

     Fix memory leak in buffer unit tests

     Signed-off-by: Steffan Karger <steffan@karger.me>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <1512724338-22197-1-git-send-email-steffan@karger.me>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16055.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c
index ba5aa67..c2b7f30 100644
--- a/tests/unit_tests/openvpn/test_buffer.c
+++ b/tests/unit_tests/openvpn/test_buffer.c
@@ -77,10 +77,9 @@  static int test_buffer_list_setup(void **state)
     buffer_list_push(ctx->zero_length_strings, "");
 
     ctx->empty_buffers = buffer_list_new(2);
-    uint8_t *data1 = malloc(1);
-    uint8_t *data2 = malloc(1);
-    buffer_list_push_data(ctx->empty_buffers, data1, 0);
-    buffer_list_push_data(ctx->empty_buffers, data2, 0);
+    uint8_t data = 0;
+    buffer_list_push_data(ctx->empty_buffers, &data, 0);
+    buffer_list_push_data(ctx->empty_buffers, &data, 0);
 
     *state = ctx;
     return 0;