Hello Henning,

Sorry hit reply only, first time.

Would you like me still to update to use mcd_free or leave it with you?

Best,

Charles


On 4 October 2013 11:40, Henning Westerholt <henning.westerholt@1und1.de> wrote:
Am Freitag, 4. Oktober 2013, 09:40:17 schrieb Charles Chance:
> Yes, agreed - I'll update it today. Henning Westerholt was original author
> if he'd like to review before I commit?

Hello Charles,

I'll take a look today to it as well, thank you.

Regards,

Henning

> On 4 Oct 2013 09:12, "Daniel-Constantin Mierla" <miconda@gmail.com> wrote:
> >  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=6faf
> >>> 12653c1db9f011b1826061824c831bda3f58
> >>>
> >>> 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 <http://twitter.com/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 <http://twitter.com/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
> > listsr-dev@lists.sip-router.orghttp://lists.sip-router.org/cgi-bin/mailma
> > n/listinfo/sr-dev
> >
> >
> > --
> > Daniel-Constantin Mierla -
> > http://www.asipto.comhttp://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




--
Charles Chance
Managing Director

t. 0121 285 4400    m. 07932 063 891

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.