DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] member: fix memory leak on error
@ 2017-12-21 17:50 Anatoly Burakov
  2017-12-22  0:01 ` Wang, Yipeng1
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Anatoly Burakov @ 2017-12-21 17:50 UTC (permalink / raw)
  To: dev; +Cc: Yipeng Wang, Sameh Gobriel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] member: fix memory leak on error
  2017-12-21 17:50 [dpdk-dev] [PATCH] member: fix memory leak on error Anatoly Burakov
@ 2017-12-22  0:01 ` Wang, Yipeng1
  2017-12-22  9:20   ` Burakov, Anatoly
  2017-12-22  0:07 ` Wang, Yipeng1
  2018-01-12 17:23 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
  2 siblings, 1 reply; 10+ messages in thread
From: Wang, Yipeng1 @ 2017-12-22  0:01 UTC (permalink / raw)
  To: Burakov, Anatoly, dev; +Cc: Gobriel, Sameh

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] member: fix memory leak on error
  2017-12-21 17:50 [dpdk-dev] [PATCH] member: fix memory leak on error Anatoly Burakov
  2017-12-22  0:01 ` Wang, Yipeng1
@ 2017-12-22  0:07 ` Wang, Yipeng1
  2018-01-12 17:23 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
  2 siblings, 0 replies; 10+ messages in thread
From: Wang, Yipeng1 @ 2017-12-22  0:07 UTC (permalink / raw)
  To: Burakov, Anatoly, dev; +Cc: Gobriel, Sameh, De Lara Guarch, Pablo

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] member: fix memory leak on error
  2017-12-22  0:01 ` Wang, Yipeng1
@ 2017-12-22  9:20   ` Burakov, Anatoly
  2017-12-22 18:33     ` Wang, Yipeng1
  0 siblings, 1 reply; 10+ messages in thread
From: Burakov, Anatoly @ 2017-12-22  9:20 UTC (permalink / raw)
  To: Wang, Yipeng1, dev; +Cc: Gobriel, Sameh

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] member: fix memory leak on error
  2017-12-22  9:20   ` Burakov, Anatoly
@ 2017-12-22 18:33     ` Wang, Yipeng1
  2017-12-23 11:55       ` Burakov, Anatoly
  0 siblings, 1 reply; 10+ messages in thread
From: Wang, Yipeng1 @ 2017-12-22 18:33 UTC (permalink / raw)
  To: Burakov, Anatoly, dev; +Cc: Gobriel, Sameh

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] member: fix memory leak on error
  2017-12-22 18:33     ` Wang, Yipeng1
@ 2017-12-23 11:55       ` Burakov, Anatoly
  2017-12-26 17:23         ` Wang, Yipeng1
  0 siblings, 1 reply; 10+ messages in thread
From: Burakov, Anatoly @ 2017-12-23 11:55 UTC (permalink / raw)
  To: Wang, Yipeng1, dev; +Cc: Gobriel, Sameh

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] member: fix memory leak on error
  2017-12-23 11:55       ` Burakov, Anatoly
@ 2017-12-26 17:23         ` Wang, Yipeng1
  0 siblings, 0 replies; 10+ messages in thread
From: Wang, Yipeng1 @ 2017-12-26 17:23 UTC (permalink / raw)
  To: Burakov, Anatoly, dev; +Cc: Gobriel, Sameh

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [PATCH v2] member: fix memory leak on error
  2017-12-21 17:50 [dpdk-dev] [PATCH] member: fix memory leak on error Anatoly Burakov
  2017-12-22  0:01 ` Wang, Yipeng1
  2017-12-22  0:07 ` Wang, Yipeng1
@ 2018-01-12 17:23 ` Anatoly Burakov
  2018-01-16 17:14   ` Wang, Yipeng1
  2 siblings, 1 reply; 10+ messages in thread
From: Anatoly Burakov @ 2018-01-12 17:23 UTC (permalink / raw)
  To: dev; +Cc: Yipeng Wang, Sameh Gobriel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] member: fix memory leak on error
  2018-01-12 17:23 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
@ 2018-01-16 17:14   ` Wang, Yipeng1
  2018-01-18 23:40     ` Thomas Monjalon
  0 siblings, 1 reply; 10+ messages in thread
From: Wang, Yipeng1 @ 2018-01-16 17:14 UTC (permalink / raw)
  To: Burakov, Anatoly, dev; +Cc: Gobriel, Sameh

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] member: fix memory leak on error
  2018-01-16 17:14   ` Wang, Yipeng1
@ 2018-01-18 23:40     ` Thomas Monjalon
  0 siblings, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2018-01-18 23:40 UTC (permalink / raw)
  To: Burakov, Anatoly; +Cc: dev, Wang, Yipeng1, Gobriel, Sameh, stable

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-01-18 23:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-21 17:50 [dpdk-dev] [PATCH] member: fix memory leak on error Anatoly Burakov
2017-12-22  0:01 ` Wang, Yipeng1
2017-12-22  9:20   ` Burakov, Anatoly
2017-12-22 18:33     ` Wang, Yipeng1
2017-12-23 11:55       ` Burakov, Anatoly
2017-12-26 17:23         ` Wang, Yipeng1
2017-12-22  0:07 ` Wang, Yipeng1
2018-01-12 17:23 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
2018-01-16 17:14   ` Wang, Yipeng1
2018-01-18 23:40     ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).