Bug #23717 for Sendmail-Milter: memory anti-leak

Thu Nov 30 05:37:24 2006 CDRAKE [...] cpan.org - Ticket created

Subject:

memory anti-leak

The .xs funtion smfi_setpriv makes this call:- RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, (void *)newSVsv(data))); which causes sendmail to "remember" the pointer to this new SV, however, you have not told perl that sendmail is remembering this pointer, so as soon as the perl calling subroutine is done with it's setpriv() call, perl will "free" this new SV, because its reference count returns to 0. Additionally, the smfi_getpriv call grabs the pointer to this data, and returns it to perl (thereby incrimenting the number of things pointing to this scalar) without incrimenting the reference count first, so as soon as whatever has used this value is finished with it, perl will attempt to free this variable again (which will result in a double-free, since setpriv probably already free it earlier). this seems to be the cause of getpriv not working as expected Eventually, this behaviour causes a segfault, probably whenever the memory freed immediately after the setpriv gets re-used I think the reason things are mostly working right now, is because folks are blindly re-calling setpriv over and over, which slightly lessens the fact that later calls to getpriv (which are returning pointers to things that already got freed!) tend to seem to work, since the stale memory they're using still (by luck) usually contains a pointer to whatever they wanted. If you agree that my interpretation is correct, I can send you the patch which seems to fix this.

Wed Mar 10 14:47:07 2010 bart [...] crossbar.net - Correspondence added

From:

bart

On Thu Nov 30 05:37:24 2006, CDRAKE wrote: Show quoted text

> > If you agree that my interpretation is correct, I can send you the > patch which seems to fix this.

I would be interested in seeing this patch. I've some issues using Sendmail::Milter with Sendmail 8.14.x and I suspect this could be the cause.

Wed Mar 10 14:47:11 2010 The RT System itself - Status changed from 'new' to 'open'

Tue May 13 22:52:49 2014 CDRAKE [...] cpan.org - Correspondence added

On Wed Mar 10 14:47:07 2010, bartd wrote: Show quoted text

> On Thu Nov 30 05:37:24 2006, CDRAKE wrote:

> > > > If you agree that my interpretation is correct, I can send you the > > patch which seems to fix this.

> > I would be interested in seeing this patch. I've some issues using > Sendmail::Milter with Sendmail 8.14.x and I suspect this could be the cause. >

Cor Fredrik - talk about testing *my* memory! Here's what I find I added to the .xs file (attached: cnd_patch_xs.txt and below). I can't remember if I changed anything else, but the only backup I could find has 2006 dates on only that one source file, so this is probably it: [root@nwt Sendmail-Milter-0.18]# diffc ori_Milter.xs Milter.xs *** ori_Milter.xs 2014-05-14 02:47:02.886840245 +0000 --- Milter.xs 2006-11-30 10:54:03.000000000 +0000 *************** *** 467,468 **** --- 467,504 ---- OUTPUT: RETVAL + + + bool + smfi_setrefpriv(ctx, data) + Sendmail_Milter_Context ctx; + SV* data; + CODE: + SV* myref; + + /* First - check to see if they've already got a reference set... */ + myref=(SV *) smfi_getpriv(ctx); + if(myref!=0) { /* Yes */ + SvREFCNT_dec(myref); /* We are about to remove or overwrite this reference, so tell perl that sendmail no longer holds a reference to it */ + } + + if (SvTRUE(data)) { + myref=newSVsv(data); /* Create a new perl scalar variable holding our data */ + SvREFCNT_inc(myref); /* We are letting sendmail "remember" this reference independently of the perl caller, so let perl know about this additional reference count */ + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, (void *)myref)); + } else { + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, NULL)); + } + OUTPUT: + RETVAL + + SV * + smfi_getrefpriv(ctx) + Sendmail_Milter_Context ctx; + CODE: + SV* myref; + myref=(SV *) smfi_getpriv(ctx); + if(myref!=0) SvREFCNT_inc(myref); /* Let perl know that a new thing is about to reference our "managed by sendmail" SV pointer. If we don't do this, our SV will get deallocated by perl when the calling sub exits. */ + RETVAL = myref; + OUTPUT: + RETVAL +

Subject:

cnd_patch_xs.txt

# diffc ori_Milter.xs Milter.xs *** ori_Milter.xs 2014-05-14 02:47:02.886840245 +0000 --- Milter.xs 2006-11-30 10:54:03.000000000 +0000 *************** *** 467,468 **** --- 467,504 ---- OUTPUT: RETVAL + + + bool + smfi_setrefpriv(ctx, data) + Sendmail_Milter_Context ctx; + SV* data; + CODE: + SV* myref; + + /* First - check to see if they've already got a reference set... */ + myref=(SV *) smfi_getpriv(ctx); + if(myref!=0) { /* Yes */ + SvREFCNT_dec(myref); /* We are about to remove or overwrite this reference, so tell perl that sendmail no longer holds a reference to it */ + } + + if (SvTRUE(data)) { + myref=newSVsv(data); /* Create a new perl scalar variable holding our data */ + SvREFCNT_inc(myref); /* We are letting sendmail "remember" this reference independently of the perl caller, so let perl know about this additional reference count */ + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, (void *)myref)); + } else { + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, NULL)); + } + OUTPUT: + RETVAL + + SV * + smfi_getrefpriv(ctx) + Sendmail_Milter_Context ctx; + CODE: + SV* myref; + myref=(SV *) smfi_getpriv(ctx); + if(myref!=0) SvREFCNT_inc(myref); /* Let perl know that a new thing is about to reference our "managed by sendmail" SV pointer. If we don't do this, our SV will get deallocated by perl when the calling sub exits. */ + RETVAL = myref; + OUTPUT: + RETVAL +

Mon May 19 04:09:17 2014 fredrik [...] xpd.se - Correspondence added

Subject:	Re: [rt.cpan.org #23717] memory anti-leak
Date:	Mon, 19 May 2014 10:09:01 +0200
To:	bug-Sendmail-Milter [...] rt.cpan.org
From:	Fredrik Söderblom <fredrik [...] xpd.se>

Many thanks! I'll test it and report any progress. /f On 14 maj 2014, at 04:52, CDRAKE via RT <bug-Sendmail-Milter@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=23717 > > > On Wed Mar 10 14:47:07 2010, bartd wrote:

>> On Thu Nov 30 05:37:24 2006, CDRAKE wrote:

>>> >>> If you agree that my interpretation is correct, I can send you the >>> patch which seems to fix this.

>> >> I would be interested in seeing this patch. I've some issues using >> Sendmail::Milter with Sendmail 8.14.x and I suspect this could be the cause. >>

> Cor Fredrik - talk about testing *my* memory! > > Here's what I find I added to the .xs file (attached: cnd_patch_xs.txt and below). I can't remember if I changed anything else, but the only backup I could find has 2006 dates on only that one source file, so this is probably it: > > [root@nwt Sendmail-Milter-0.18]# diffc ori_Milter.xs Milter.xs > *** ori_Milter.xs 2014-05-14 02:47:02.886840245 +0000 > --- Milter.xs 2006-11-30 10:54:03.000000000 +0000 > *************** > *** 467,468 **** > --- 467,504 ---- > OUTPUT: > RETVAL > + > + > + bool > + smfi_setrefpriv(ctx, data) > + Sendmail_Milter_Context ctx; > + SV* data; > + CODE: > + SV* myref; > + > + /* First - check to see if they've already got a reference set... */ > + myref=(SV *) smfi_getpriv(ctx); > + if(myref!=0) { /* Yes */ > + SvREFCNT_dec(myref); /* We are about to remove or overwrite this reference, so tell perl that sendmail no longer holds a reference to it */ > + } > + > + if (SvTRUE(data)) { > + myref=newSVsv(data); /* Create a new perl scalar variable holding our data */ > + SvREFCNT_inc(myref); /* We are letting sendmail "remember" this reference independently of the perl caller, so let perl know about this additional reference count */ > + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, (void *)myref)); > + } else { > + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, NULL)); > + } > + OUTPUT: > + RETVAL > + > + SV * > + smfi_getrefpriv(ctx) > + Sendmail_Milter_Context ctx; > + CODE: > + SV* myref; > + myref=(SV *) smfi_getpriv(ctx); > + if(myref!=0) SvREFCNT_inc(myref); /* Let perl know that a new thing is about to reference our "managed by sendmail" SV pointer. If we don't do this, our SV will get deallocated by perl when the calling sub exits. */ > + RETVAL = myref; > + OUTPUT: > + RETVAL > + > > > # diffc ori_Milter.xs Milter.xs > *** ori_Milter.xs 2014-05-14 02:47:02.886840245 +0000 > --- Milter.xs 2006-11-30 10:54:03.000000000 +0000 > *************** > *** 467,468 **** > --- 467,504 ---- > OUTPUT: > RETVAL > + > + > + bool > + smfi_setrefpriv(ctx, data) > + Sendmail_Milter_Context ctx; > + SV* data; > + CODE: > + SV* myref; > + > + /* First - check to see if they've already got a reference set... */ > + myref=(SV *) smfi_getpriv(ctx); > + if(myref!=0) { /* Yes */ > + SvREFCNT_dec(myref); /* We are about to remove or overwrite this reference, so tell perl that sendmail no longer holds a reference to it */ > + } > + > + if (SvTRUE(data)) { > + myref=newSVsv(data); /* Create a new perl scalar variable holding our data */ > + SvREFCNT_inc(myref); /* We are letting sendmail "remember" this reference independently of the perl caller, so let perl know about this additional reference count */ > + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, (void *)myref)); > + } else { > + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, NULL)); > + } > + OUTPUT: > + RETVAL > + > + SV * > + smfi_getrefpriv(ctx) > + Sendmail_Milter_Context ctx; > + CODE: > + SV* myref; > + myref=(SV *) smfi_getpriv(ctx); > + if(myref!=0) SvREFCNT_inc(myref); /* Let perl know that a new thing is about to reference our "managed by sendmail" SV pointer. If we don't do this, our SV will get deallocated by perl when the calling sub exits. */ > + RETVAL = myref; > + OUTPUT: > + RETVAL > +

Download signature.asc
application/pgp-signature 801b

Message body not shown because it is not plain text.

Sun May 25 06:08:37 2014 fredrik [...] xpd.se - Correspondence added

Subject:	Re: [rt.cpan.org #23717] memory anti-leak
Date:	Sun, 25 May 2014 11:56:38 +0200
To:	bug-Sendmail-Milter [...] rt.cpan.org
From:	Fredrik Söderblom <fredrik [...] xpd.se>

Awesome, thanks! I applied it as in the attached file, eg replacing the code for "smfi_setpriv()" and "smfi_getpriv()" with your code. However it didn't affect (what i believe now is another bug), "make test" still crashes on both 32 and 64 bit, SuSE and Ubuntu. $ make test PERL_DL_NONLAZY=1 /usr/bin/perl "-Iblib/lib" "-Iblib/arch" test.pl ------------------------------------------------------------------------ Interpreter pool tests. See sample.pl for a sample Milter. ------------------------------------------------------------------------ Running starvation test... (Core dump indicates failure ;-) ------------------------------------------------------------------------ test_wrapper: Original interpreter cloned: 0x09ec3008 test_wrapper: Analysing callback... test_wrapper: It's a code reference to: 0x409112c0 test_wrapper: Calling callback 0x409112ac from aTHX 0x40900478. ---> Starting callback from interpreter: [0x40900478]. ---> Finished callback from interpreter: [0x40900478]. test_wrapper: Analysing callback... test_wrapper: It's a code reference to: 0x409112c0 test_wrapper: Calling callback 0x409112ac from aTHX 0x40900478. ---> Starting callback from interpreter: [0x40900478]. ---> Finished callback from interpreter: [0x40900478]. test_wrapper: Analysing callback... test_wrapper: It's a code reference to: 0x409112c0 test_wrapper: Calling callback 0x409112ac from aTHX 0x40900478. ---> Starting callback from interpreter: [0x40900478]. ---> Finished callback from interpreter: [0x40900478]. intpool pthread_mutex_destroy() failed: 16 at test.pl line 32. make: *** [test_dynamic] Error 255 It fails on the "Sendmail::Milter::test_intpools(1, 0, 2, 2, \&perl_callback);" call and if comment that one out, all the other tests passes. I'm too much of a perl noob to understand why the test fails however, it seems like pthread_mutex_destroy() returns EBUSY (16), which is "The implementation has detected an attempt to destroy the object referenced by mutex while it is locked or referenced (for example, while being used in a pthread_cond_timedwait() or pthread_cond_wait()) by another thread." Do you have ideas what could be causing this? Or am i on a wild-goose chase here? Best Regards, /f

Message body is not shown because sender requested not to inline it.

On 14 maj 2014, at 04:52, CDRAKE via RT <bug-Sendmail-Milter@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=23717 > > > On Wed Mar 10 14:47:07 2010, bartd wrote:

>> On Thu Nov 30 05:37:24 2006, CDRAKE wrote:

>>> >>> If you agree that my interpretation is correct, I can send you the >>> patch which seems to fix this.

>> >> I would be interested in seeing this patch. I've some issues using >> Sendmail::Milter with Sendmail 8.14.x and I suspect this could be the cause. >>

> Cor Fredrik - talk about testing *my* memory! > > Here's what I find I added to the .xs file (attached: cnd_patch_xs.txt and below). I can't remember if I changed anything else, but the only backup I could find has 2006 dates on only that one source file, so this is probably it: > > [root@nwt Sendmail-Milter-0.18]# diffc ori_Milter.xs Milter.xs > *** ori_Milter.xs 2014-05-14 02:47:02.886840245 +0000 > --- Milter.xs 2006-11-30 10:54:03.000000000 +0000 > *************** > *** 467,468 **** > --- 467,504 ---- > OUTPUT: > RETVAL > + > + > + bool > + smfi_setrefpriv(ctx, data) > + Sendmail_Milter_Context ctx; > + SV* data; > + CODE: > + SV* myref; > + > + /* First - check to see if they've already got a reference set... */ > + myref=(SV *) smfi_getpriv(ctx); > + if(myref!=0) { /* Yes */ > + SvREFCNT_dec(myref); /* We are about to remove or overwrite this reference, so tell perl that sendmail no longer holds a reference to it */ > + } > + > + if (SvTRUE(data)) { > + myref=newSVsv(data); /* Create a new perl scalar variable holding our data */ > + SvREFCNT_inc(myref); /* We are letting sendmail "remember" this reference independently of the perl caller, so let perl know about this additional reference count */ > + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, (void *)myref)); > + } else { > + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, NULL)); > + } > + OUTPUT: > + RETVAL > + > + SV * > + smfi_getrefpriv(ctx) > + Sendmail_Milter_Context ctx; > + CODE: > + SV* myref; > + myref=(SV *) smfi_getpriv(ctx); > + if(myref!=0) SvREFCNT_inc(myref); /* Let perl know that a new thing is about to reference our "managed by sendmail" SV pointer. If we don't do this, our SV will get deallocated by perl when the calling sub exits. */ > + RETVAL = myref; > + OUTPUT: > + RETVAL > + > > > # diffc ori_Milter.xs Milter.xs > *** ori_Milter.xs 2014-05-14 02:47:02.886840245 +0000 > --- Milter.xs 2006-11-30 10:54:03.000000000 +0000 > *************** > *** 467,468 **** > --- 467,504 ---- > OUTPUT: > RETVAL > + > + > + bool > + smfi_setrefpriv(ctx, data) > + Sendmail_Milter_Context ctx; > + SV* data; > + CODE: > + SV* myref; > + > + /* First - check to see if they've already got a reference set... */ > + myref=(SV *) smfi_getpriv(ctx); > + if(myref!=0) { /* Yes */ > + SvREFCNT_dec(myref); /* We are about to remove or overwrite this reference, so tell perl that sendmail no longer holds a reference to it */ > + } > + > + if (SvTRUE(data)) { > + myref=newSVsv(data); /* Create a new perl scalar variable holding our data */ > + SvREFCNT_inc(myref); /* We are letting sendmail "remember" this reference independently of the perl caller, so let perl know about this additional reference count */ > + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, (void *)myref)); > + } else { > + RETVAL = MI_BOOL_CVT(smfi_setpriv(ctx, NULL)); > + } > + OUTPUT: > + RETVAL > + > + SV * > + smfi_getrefpriv(ctx) > + Sendmail_Milter_Context ctx; > + CODE: > + SV* myref; > + myref=(SV *) smfi_getpriv(ctx); > + if(myref!=0) SvREFCNT_inc(myref); /* Let perl know that a new thing is about to reference our "managed by sendmail" SV pointer. If we don't do this, our SV will get deallocated by perl when the calling sub exits. */ > + RETVAL = myref; > + OUTPUT: > + RETVAL > +

Download signature.asc
application/pgp-signature 801b

Message body not shown because it is not plain text.