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

[Antonio Q. <a...@un...>]

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 <st...@ka...>

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...@un...>

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;
> 

-- 
Antonio Quartulli