[Tarantool-patches] [PATCH v1 1/1] netbox: fix memleak in lbox_tuple_format_new()

[Tarantool-patches] [PATCH v1 1/1] netbox: fix memleak in lbox_tuple_format_new()

[imeevma]

Memleak appeared due to the fact that when creating a dictionary
its reference count is 1 from the beginning. Later, when the
dictionary is used to create tuple_format, its reference counter
increased by 1 and it became equal to 2. After removing
tuple_format, the reference counter for dict decreased by one, so
it became equal to 1. Since ref counter is not equal to 0, dict
not deleted, causing a memory leak.

Closes #4588
---
 src/box/lua/misc.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index 7b8b9dc..102194e 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -233,6 +233,7 @@ lbox_tuple_format_new(struct lua_State *L)
 				 NULL, 0, 0, dict, false, false);
 	if (format == NULL)
 		return luaT_error(L);
+	tuple_dictionary_unref(dict);
 	return lbox_push_tuple_format(L, format);
 }
 
-- 
2.7.4

Re: [Tarantool-patches] [PATCH v1 1/1] netbox: fix memleak in lbox_tuple_format_new()

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 30/10/2019 11:58, imeevma@tarantool.org wrote:
> Memleak appeared due to the fact that when creating a dictionary
> its reference count is 1 from the beginning. Later, when the
> dictionary is used to create tuple_format, its reference counter
> increased by 1 and it became equal to 2. After removing
> tuple_format, the reference counter for dict decreased by one, so
> it became equal to 1. Since ref counter is not equal to 0, dict
> not deleted, causing a memory leak.
> 
> Closes #4588
> ---
>  src/box/lua/misc.cc | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
> index 7b8b9dc..102194e 100644
> --- a/src/box/lua/misc.cc
> +++ b/src/box/lua/misc.cc
> @@ -233,6 +233,7 @@ lbox_tuple_format_new(struct lua_State *L)
>  				 NULL, 0, 0, dict, false, false);
>  	if (format == NULL)
>  		return luaT_error(L);

The dict should be unreferenced regardless of tuple_format_new()
result. Now there is a leak, when format == NULL.

> +	tuple_dictionary_unref(dict);
>  	return lbox_push_tuple_format(L, format);
>  }
>  

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v1 1/1] netbox: fix memleak in lbox_tuple_format_new()

[Mergen Imeev]

Thank you for review. I fixed the error and reworked the
patch a bit. New patch below.

On Wed, Oct 30, 2019 at 11:13:54PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 30/10/2019 11:58, imeevma@tarantool.org wrote:
> > Memleak appeared due to the fact that when creating a dictionary
> > its reference count is 1 from the beginning. Later, when the
> > dictionary is used to create tuple_format, its reference counter
> > increased by 1 and it became equal to 2. After removing
> > tuple_format, the reference counter for dict decreased by one, so
> > it became equal to 1. Since ref counter is not equal to 0, dict
> > not deleted, causing a memory leak.
> > 
> > Closes #4588
> > ---
> >  src/box/lua/misc.cc | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
> > index 7b8b9dc..102194e 100644
> > --- a/src/box/lua/misc.cc
> > +++ b/src/box/lua/misc.cc
> > @@ -233,6 +233,7 @@ lbox_tuple_format_new(struct lua_State *L)
> >  				 NULL, 0, 0, dict, false, false);
> >  	if (format == NULL)
> >  		return luaT_error(L);
> 
> The dict should be unreferenced regardless of tuple_format_new()
> result. Now there is a leak, when format == NULL.
> 
Fixed.

> > +	tuple_dictionary_unref(dict);
> >  	return lbox_push_tuple_format(L, format);
> >  }
> >  
> 

New patch:

From 0d5bbc6e95394cc772ab05d6af03bd1cee6c6536 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Wed, 30 Oct 2019 13:39:14 +0300
Subject: [PATCH] netbox: fix memory leak in connect()

This patch fixes memory leak in lbox_tuple_format_new().

Closes #4588

diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index 7b8b9dc..79b6cfe 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -231,6 +231,12 @@ lbox_tuple_format_new(struct lua_State *L)
 	struct tuple_format *format =
 		tuple_format_new(&tuple_format_runtime->vtab, NULL, NULL, 0,
 				 NULL, 0, 0, dict, false, false);
+	/*
+	 * Since dictionary reference counter is 1 from the
+	 * beginning and after creation of the tuple_format
+	 * increases by one, we must decrease it once.
+	 */
+	tuple_dictionary_unref(dict);
 	if (format == NULL)
 		return luaT_error(L);
 	return lbox_push_tuple_format(L, format);

Re: [Tarantool-patches] [PATCH v1 1/1] netbox: fix memleak in lbox_tuple_format_new()

[Konstantin Osipov]

* imeevma@tarantool.org <imeevma@tarantool.org> [19/10/30 14:03]:
> Memleak appeared due to the fact that when creating a dictionary
> its reference count is 1 from the beginning. Later, when the
> dictionary is used to create tuple_format, its reference counter
> increased by 1 and it became equal to 2. After removing
> tuple_format, the reference counter for dict decreased by one, so
> it became equal to 1. Since ref counter is not equal to 0, dict
> not deleted, causing a memory leak.
> 
> Closes #4588

I wonder if you could come up with a test case? Is memory used by
dict accounted in any stats?


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v1 1/1] netbox: fix memleak in lbox_tuple_format_new()

[Imeev Mergen]

On 10/31/19 6:23 PM, Konstantin Osipov wrote:
> * imeevma@tarantool.org <imeevma@tarantool.org> [19/10/30 14:03]:
>> Memleak appeared due to the fact that when creating a dictionary
>> its reference count is 1 from the beginning. Later, when the
>> dictionary is used to create tuple_format, its reference counter
>> increased by 1 and it became equal to 2. After removing
>> tuple_format, the reference counter for dict decreased by one, so
>> it became equal to 1. Since ref counter is not equal to 0, dict
>> not deleted, causing a memory leak.
>>
>> Closes #4588
> I wonder if you could come up with a test case? Is memory used by
> dict accounted in any stats?
>
I did not find anything in box, and I did not see anything about
the statistics in the tuple_dictionary_new() function. Doesn't
look like we accounting this. At the moment, I can’t come up with
a test.

You think we should add this in stats?

Re: [Tarantool-patches] [PATCH v1 1/1] netbox: fix memleak in lbox_tuple_format_new()

[Vladislav Shpilevoy]

Hi! Thanks for the fix!

LGTM.

On 31/10/2019 17:43, Imeev Mergen wrote:
> 
> On 10/31/19 6:23 PM, Konstantin Osipov wrote:
>> * imeevma@tarantool.org <imeevma@tarantool.org> [19/10/30 14:03]:
>>> Memleak appeared due to the fact that when creating a dictionary
>>> its reference count is 1 from the beginning. Later, when the
>>> dictionary is used to create tuple_format, its reference counter
>>> increased by 1 and it became equal to 2. After removing
>>> tuple_format, the reference counter for dict decreased by one, so
>>> it became equal to 1. Since ref counter is not equal to 0, dict
>>> not deleted, causing a memory leak.
>>>
>>> Closes #4588
>> I wonder if you could come up with a test case? Is memory used by
>> dict accounted in any stats?
>>
> I did not find anything in box, and I did not see anything about
> the statistics in the tuple_dictionary_new() function. Doesn't
> look like we accounting this. At the moment, I can’t come up with
> a test.
> 
> You think we should add this in stats?
> 

There is no accounting for the heap memory. We tried to add it via
symbol replacement, but it didn't work properly. I think we can
omit a test here.

Re: [Tarantool-patches] [tarantool-patches] [PATCH v1 1/1] netbox: fix memleak in lbox_tuple_format_new()

[Kirill Yukhin]

Hello,

On 30 окт 13:58, imeevma@tarantool.org wrote:
> Memleak appeared due to the fact that when creating a dictionary
> its reference count is 1 from the beginning. Later, when the
> dictionary is used to create tuple_format, its reference counter
> increased by 1 and it became equal to 2. After removing
> tuple_format, the reference counter for dict decreased by one, so
> it became equal to 1. Since ref counter is not equal to 0, dict
> not deleted, causing a memory leak.
> 
> Closes #4588

I've checked your patch into 2.2 and master.

--
Regards, Kirill Yukhin