Hello,

for consistency, it might be good that mcd_free() is used instead of pkg_free when mcd_memory is set, because it is supposed to be the pair of mcd_malloc() which is used to allocate the memory. mcd_free() is a wrapper around pkg_free(), so works like current patch, but it is safer for future changes to mcd_*() functions.

These are remarks just looking at the current code, I am not familiar with memcached lib and api.

Cheers,
Daniel

On 10/4/13 9:34 AM, Federico Cabiddu wrote:
Hi Charles,
I just tried the patch running the test I used to reproduce the leak.
It's ok, the memory debug shows that the memory is correctly released.
I tested with memory parameter set to 0 and to 1.
Thank you.

Best regards,

Federico


On Fri, Oct 4, 2013 at 1:00 AM, Charles Chance <charles.chance@sipcentric.com> wrote:
Hi Federico/Dragos,

Thank you both for your input. I have placed the call to (pkg_)free into a separate function and called it where necessary from each of the other functions.

I haven't had chance to test yet - if you have time, please apply the attached diff and let me know if the leak is fixed and I will commit to master. Otherwise, I will test myself over the weekend.

Thanks again,

Charles



On 3 October 2013 21:30, Dragos Oancea <droancea@yahoo.com> wrote:
Hi Charles,

In the function where Daniel just made the fix for the memory leak (int pv_get_mcd_value() ) , just before existing it with 0  , we added  something like the following :

if (mcd_memory) {

       pkg_free(return_value); 

}  else {

free(return_value); 
}

It looks like it does not leak anymore, but please-double check if we are free-ing it in the right place. 


Regards,
Dragos


From: Charles Chance <charles.chance@sipcentric.com>
To: Daniel-Constantin Mierla <miconda@gmail.com>
Cc: sr-dev <sr-dev@lists.sip-router.org>
Sent: Thursday, October 3, 2013 7:27 PM
Subject: Re: [sr-dev] memory leak in memcached module

I can take a look this evening. Assuming nobody has already started?
Best,
Charles
On 2 Oct 2013 20:23, "Daniel-Constantin Mierla" <miconda@gmail.com> wrote:
Hello,

there is (still) a memory leak in memcached module, discovered on a report by Dragos Oancea.

The pkg usage logs are like:

0(24328) NOTICE: qm_status:    19010. N  address=0x7fb23683bc98 frag=0x7fb23683bc68 size=8 used=1
 0(24328) NOTICE: qm_status:           alloc'd from memcached: ../../parser/../ut.h: pkg_str_dup(733)
 0(24328) NOTICE: qm_status:          start check=f0f0f0f0, end check= c0c0c0c0, abcdefed
 0(24328) NOTICE: qm_status:    19011. N  address=0x7fb23683bd00 frag=0x7fb23683bcd0 size=48 used=1
 0(24328) NOTICE: qm_status:           alloc'd from memcached: memcached.c: mcd_malloc(127)
 0(24328) NOTICE: qm_status:          start check=f0f0f0f0, end check= c0c0c0c0, abcdefed
 0(24328) NOTICE: qm_status:    19012. N  address=0x7fb23683bd90 frag=0x7fb23683bd60 size=8 used=1
 0(24328) NOTICE: qm_status:           alloc'd from memcached: ../../parser/../ut.h: pkg_str_dup(733)
 0(24328) NOTICE: qm_status:          start check=f0f0f0f0, end check= c0c0c0c0, abcdefed
 0(24328) NOTICE: qm_status:    19013. N  address=0x7fb23683bdf8 frag=0x7fb23683bdc8 size=48 used=1
 0(24328) NOTICE: qm_status:           alloc'd from memcached: memcached.c: mcd_malloc(127)
 0(24328) NOTICE: qm_status:          start check=f0f0f0f0, end check= c0c0c0c0, abcdefed
 0(24328) NOTICE: qm_status:    19014. N  address=0x7fb23683be88 frag=0x7fb23683be58 size=8 used=1
 0(24328) NOTICE: qm_status:           alloc'd from memcached: memcached.c: mcd_malloc(127)
 0(24328) NOTICE: qm_status:          start check=f0f0f0f0, end check= c0c0c0c0, abcdefed
 0(24328) NOTICE: qm_status:    19015. N  address=0x7fb23683bef0 frag=0x7fb23683bec0 size=16 used=1
 0(24328) NOTICE: qm_status:           alloc'd from memcached: ../../parser/../ut.h: pkg_str_dup(733)
 0(24328) NOTICE: qm_status:          start check=f0f0f0f0, end check= c0c0c0c0, abcdefed
 0(24328) NOTICE: qm_status:    19016. N  address=0x7fb23683bf60 frag=0x7fb23683bf30 size=8 used=1
 0(24328) NOTICE: qm_status:           alloc'd from memcached: memcached.c: mcd_malloc(127)
 0(24328) NOTICE: qm_status:          start check=f0f0f0f0, end check= c0c0c0c0, abcdefed
 0(24328) NOTICE: qm_status:    19017. N  address=0x7fb23683bfc8 frag=0x7fb23683bf98 size=24 used=1
 0(24328) NOTICE: qm_status:           alloc'd from memcached: ../../parser/../ut.h: pkg_str_dup(733)
 0(24328) NOTICE: qm_status:          start check=f0f0f0f0, end check= c0c0c0c0, abcdefed

The one related to pkg_str_dup() should be fixed by the commit:

- http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=6faf12653c1db9f011b1826061824c831bda3f58

The other one is related to mcd_malloc(), which I guess it is due to the function that returns the value from memchache, memcached_get() used in  pv_get_mcd_value_helper() -- the returned object has to be freed by calling code, according to:

- http://docs.libmemcached.org/memcached_get.html

Since the libmemcached was initialized with wrappers around pkg malloc/free, I expect the respective free function has to be used to free the result.

Can any of memcached devs check my commit and investigate further the second leak?

Cheers,
Daniel

--
Daniel-Constantin Mierla - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Trainings - Berlin, Nov 25-28; Miami, Nov 18-20, 2013
  - more details about Kamailio trainings at http://www.asipto.com -


_______________________________________________
sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

www.sipcentric.com

Follow us on twitter @sipcentric

Sipcentric Ltd. Company registered in England & Wales no. 7365592. Registered office: Unit 10 iBIC, Birmingham Science Park, Holt Court South, Birmingham B7 4EJ.

_______________________________________________
sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev



_______________________________________________
sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev





www.sipcentric.com

Follow us on twitter @sipcentric

Sipcentric Ltd. Company registered in England & Wales no. 7365592. Registered office: Unit 10 iBIC, Birmingham Science Park, Holt Court South, Birmingham B7 4EJ.

_______________________________________________
sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev




_______________________________________________
sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- 
Daniel-Constantin Mierla - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Trainings - Berlin, Nov 25-28; Miami, Nov 18-20, 2013
  - more details about Kamailio trainings at http://www.asipto.com -