[PATCH] member: fix memory leak on error

[PATCH] member: fix memory leak on error

[Anatoly Burakov]

rte_member may have allocated a tailq entry before failure, so
free it.

Fixes: 857ed6c68cf2 ("member: implement main API")
Cc: yipeng1.wang@intel.com
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_member/rte_member.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_member/rte_member.c b/lib/librte_member/rte_member.c
index cc9ea84..569fff9 100644
--- a/lib/librte_member/rte_member.c
+++ b/lib/librte_member/rte_member.c
@@ -191,6 +191,7 @@ rte_member_create(const struct rte_member_parameters *params)
 	return setsum;
 
 error_unlock_exit:
+	rte_free(te);
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 	rte_member_free(setsum);
 	return NULL;
-- 
2.7.4

Re: [PATCH] member: fix memory leak on error

[Wang, Yipeng1]

Thank you Anatoly for finding this issue. In the code I tried to reuse the rte_member_free function to free memory but it may not be executed through. 

Because of this, I may not properly release setsum struct neither. I will post a fix for both soon.

Thanks

>-----Original Message-----
>From: Burakov, Anatoly
>Sent: Thursday, December 21, 2017 9:51 AM
>To: dev@dpdk.org
>Cc: Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh
><sameh.gobriel@intel.com>
>Subject: [PATCH] member: fix memory leak on error
>
>rte_member may have allocated a tailq entry before failure, so
>free it.
>
>Fixes: 857ed6c68cf2 ("member: implement main API")
>Cc: yipeng1.wang@intel.com
>Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>---
> lib/librte_member/rte_member.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/lib/librte_member/rte_member.c
>b/lib/librte_member/rte_member.c
>index cc9ea84..569fff9 100644
>--- a/lib/librte_member/rte_member.c
>+++ b/lib/librte_member/rte_member.c
>@@ -191,6 +191,7 @@ rte_member_create(const struct
>rte_member_parameters *params)
> 	return setsum;
>
> error_unlock_exit:
>+	rte_free(te);
> 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> 	rte_member_free(setsum);
> 	return NULL;
>--
>2.7.4

Re: [PATCH] member: fix memory leak on error

[Wang, Yipeng1]

Btw, Pablo, since I remember I refer to the EFD library on my membership implementation, does EFD have similar memory leakage issue that not releasing te when failure?

Thanks

>-----Original Message-----
>From: Wang, Yipeng1
>Sent: Thursday, December 21, 2017 4:01 PM
>To: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org
>Cc: Gobriel, Sameh <sameh.gobriel@intel.com>
>Subject: RE: [PATCH] member: fix memory leak on error
>
>Thank you Anatoly for finding this issue. In the code I tried to reuse the
>rte_member_free function to free memory but it may not be executed
>through.
>
>Because of this, I may not properly release setsum struct neither. I will post a
>fix for both soon.
>
>Thanks
>
>>-----Original Message-----
>>From: Burakov, Anatoly
>>Sent: Thursday, December 21, 2017 9:51 AM
>>To: dev@dpdk.org
>>Cc: Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh
>><sameh.gobriel@intel.com>
>>Subject: [PATCH] member: fix memory leak on error
>>
>>rte_member may have allocated a tailq entry before failure, so
>>free it.
>>
>>Fixes: 857ed6c68cf2 ("member: implement main API")
>>Cc: yipeng1.wang@intel.com
>>Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>---
>> lib/librte_member/rte_member.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>>diff --git a/lib/librte_member/rte_member.c
>>b/lib/librte_member/rte_member.c
>>index cc9ea84..569fff9 100644
>>--- a/lib/librte_member/rte_member.c
>>+++ b/lib/librte_member/rte_member.c
>>@@ -191,6 +191,7 @@ rte_member_create(const struct
>>rte_member_parameters *params)
>> 	return setsum;
>>
>> error_unlock_exit:
>>+	rte_free(te);
>> 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
>> 	rte_member_free(setsum);
>> 	return NULL;
>>--
>>2.7.4

Re: [PATCH] member: fix memory leak on error

[Burakov, Anatoly]

On 22-Dec-17 12:01 AM, Wang, Yipeng1 wrote:
> Thank you Anatoly for finding this issue. In the code I tried to reuse the rte_member_free function to free memory but it may not be executed through.
> 
> Because of this, I may not properly release setsum struct neither. I will post a fix for both soon.
> 
> Thanks

Yep, i can see that now. Didn't think to look inside rte_member_free() 
:/ However, you're creating a race condition there - you're unlocking a 
tailq, and then locking (and unlocking) it again inside 
rte_member_free() - it probably needs _thread_unsafe() functions that 
you can call from behind the lock.

-- 
Thanks,
Anatoly

Re: [PATCH] member: fix memory leak on error

[Wang, Yipeng1]

>-----Original Message-----
>From: Burakov, Anatoly
>Yep, i can see that now. Didn't think to look inside rte_member_free()
>:/ However, you're creating a race condition there - you're unlocking a
>tailq, and then locking (and unlocking) it again inside
>rte_member_free() - it probably needs _thread_unsafe() functions that
>you can call from behind the lock.
>
>--

Thank you Anatoly,

I realize that rte_member_free does not do anything good here. As a fix, I think the following should work. Is there any other concern?

diff --git a/lib/librte_member/rte_member.c b/lib/librte_member/rte_member.c
index cc9ea84..25934e8 100644
--- a/lib/librte_member/rte_member.c
+++ b/lib/librte_member/rte_member.c
@@ -192,7 +192,8 @@ rte_member_create(const struct rte_member_parameters *params)

 error_unlock_exit:
        rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
-       rte_member_free(setsum);
+       rte_free(te);
+       rte_free(setsum);
        return NULL;
 }

Thank you!

Re: [PATCH] member: fix memory leak on error

[Burakov, Anatoly]

On 22-Dec-17 6:33 PM, Wang, Yipeng1 wrote:
>> -----Original Message-----
>> From: Burakov, Anatoly
>> Yep, i can see that now. Didn't think to look inside rte_member_free()
>> :/ However, you're creating a race condition there - you're unlocking a
>> tailq, and then locking (and unlocking) it again inside
>> rte_member_free() - it probably needs _thread_unsafe() functions that
>> you can call from behind the lock.
>>
>> --
> 
> Thank you Anatoly,
> 
> I realize that rte_member_free does not do anything good here. As a fix, I think the following should work. Is there any other concern?
> 

Yes, that should work. Table creation is the last step that can cause an 
error, so if we're there, we already know that we couldn't have 
allocated it, so there's no need to deallocate those, and simple 
rte_free(setsum) should do. I'll submit a v2?

-- 
Thanks,
Anatoly

Re: [PATCH] member: fix memory leak on error

[Wang, Yipeng1]

Sure. Thanks Anatoly,

Please go ahead and submit V2. 

Thanks
Yipeng

>-----Original Message-----
>From: Burakov, Anatoly
>Sent: Saturday, December 23, 2017 3:55 AM
>To: Wang, Yipeng1 <yipeng1.wang@intel.com>; dev@dpdk.org
>Cc: Gobriel, Sameh <sameh.gobriel@intel.com>
>Subject: Re: [PATCH] member: fix memory leak on error
>
>On 22-Dec-17 6:33 PM, Wang, Yipeng1 wrote:
>>> -----Original Message-----
>>> From: Burakov, Anatoly
>>> Yep, i can see that now. Didn't think to look inside rte_member_free()
>>> :/ However, you're creating a race condition there - you're unlocking a
>>> tailq, and then locking (and unlocking) it again inside
>>> rte_member_free() - it probably needs _thread_unsafe() functions that
>>> you can call from behind the lock.
>>>
>>> --
>>
>> Thank you Anatoly,
>>
>> I realize that rte_member_free does not do anything good here. As a fix, I
>think the following should work. Is there any other concern?
>>
>
>Yes, that should work. Table creation is the last step that can cause an
>error, so if we're there, we already know that we couldn't have
>allocated it, so there's no need to deallocate those, and simple
>rte_free(setsum) should do. I'll submit a v2?
>
>--
>Thanks,
>Anatoly

[PATCH v2] member: fix memory leak on error

[Anatoly Burakov]

rte_member may have allocated a tailq entry or setum before failure,
so free them.

Fixes: 857ed6c68cf2 ("member: implement main API")
Cc: yipeng1.wang@intel.com
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    v2: free setsum as well

 lib/librte_member/rte_member.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_member/rte_member.c b/lib/librte_member/rte_member.c
index 0c4c144..bc4cef6 100644
--- a/lib/librte_member/rte_member.c
+++ b/lib/librte_member/rte_member.c
@@ -162,8 +162,9 @@ rte_member_create(const struct rte_member_parameters *params)
 	return setsum;
 
 error_unlock_exit:
+	rte_free(te);
+	rte_free(setsum);
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
-	rte_member_free(setsum);
 	return NULL;
 }
 
-- 
2.7.4

Re: [PATCH v2] member: fix memory leak on error

[Wang, Yipeng1]

>-----Original Message-----
>From: Burakov, Anatoly
>Sent: Friday, January 12, 2018 9:23 AM
>To: dev@dpdk.org
>Cc: Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh <sameh.gobriel@intel.com>
>Subject: [PATCH v2] member: fix memory leak on error
>
>rte_member may have allocated a tailq entry or setum before failure,
>so free them.
>
>Fixes: 857ed6c68cf2 ("member: implement main API")
>Cc: yipeng1.wang@intel.com
>Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>---

Acked-by: Yipeng Wang <yipeng1.wang@intel.com>

Thanks

Re: [PATCH v2] member: fix memory leak on error

[Thomas Monjalon]

> >rte_member may have allocated a tailq entry or setum before failure,
> >so free them.
> >
> >Fixes: 857ed6c68cf2 ("member: implement main API")

Cc: stable@dpdk.org

> >Cc: yipeng1.wang@intel.com
> >Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Acked-by: Yipeng Wang <yipeng1.wang@intel.com>

Applied, thanks