Bug #92642 for version: Leak in Perl_new

Sat Feb 01 14:09:18 2014 jpeacock [...] cpan.org - Ticket created

Subject:

Leak in Perl_new_version

There is also a leak in Perl_new_version since if UPG_VERSION croaks, SV "rv" is leaked. Either a early sv_2mortal and at the very end, after UPG_VERSION (and it is guarenteed we can't croak), a SvREFCNT_inc_optimized is required. Or replace the sv_2mortal with SAVEFREESV.

Sat Feb 01 14:15:40 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92642] AutoReply: Leak in Perl_new_version
Date:	Sat, 01 Feb 2014 14:15:31 -0500
To:	bug-version [...] rt.cpan.org, jpeacock [...] cpan.org
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 02:09 PM, Bugs in version via RT wrote: Show quoted text

> ------------------------------------------------------------------------- > There is also a leak in Perl_new_version since if UPG_VERSION croaks, SV "rv" is leaked. Either a early sv_2mortal and at the very end, after UPG_VERSION (and it is guarenteed we can't croak), a SvREFCNT_inc_optimized is required. Or replace the sv_2mortal with SAVEFREESV. >

I think this is sufficient: --- a/vutil/vutil.c Sat Feb 01 13:47:17 2014 -0500 +++ b/vutil/vutil.c Sat Feb 01 14:10:53 2014 -0500 @@ -558,6 +558,8 @@ #endif PERL_ARGS_ASSERT_UPG_VERSION; + sv_2mortal(ver); /* in case we croak before we return */ + if (SvNOK(ver) #if PERL_VERSION_LT(5,17,2) || (SvTYPE(ver) == SVt_PVMG && !SvPOK(ver) && SvNOKp(ver)) @@ -674,6 +676,8 @@ #if PERL_VERSION_LT(5,19,8) && defined(USE_ITHREADS) LEAVE; #endif + + SvREFCNT_inc_optimized(ver); return ver; } That schedules ver (which is rv from new_version) to be freed at the end of the scope, unless we are just about to return, where we inc it back to life again. John

Sat Feb 01 14:15:41 2014 The RT System itself - Status changed from 'new' to 'open'

Sat Feb 01 14:27:15 2014 BULKDD [...] cpan.org - Correspondence added

On Sat Feb 01 14:15:40 2014, john.peacock@havurah-software.org wrote: Show quoted text

> On 02/01/2014 02:09 PM, Bugs in version via RT wrote:

> > ------------------------------------------------------------------------- > > There is also a leak in Perl_new_version since if UPG_VERSION croaks, > > SV "rv" is leaked. Either a early sv_2mortal and at the very end, > > after UPG_VERSION (and it is guarenteed we can't croak), a > > SvREFCNT_inc_optimized is required. Or replace the sv_2mortal with > > SAVEFREESV. > >

> > I think this is sufficient: > > > --- a/vutil/vutil.c Sat Feb 01 13:47:17 2014 -0500 > +++ b/vutil/vutil.c Sat Feb 01 14:10:53 2014 -0500 > @@ -558,6 +558,8 @@ > #endif > PERL_ARGS_ASSERT_UPG_VERSION; > > + sv_2mortal(ver); /* in case we croak before we return */ > + > if (SvNOK(ver) > #if PERL_VERSION_LT(5,17,2) > || (SvTYPE(ver) == SVt_PVMG && !SvPOK(ver) && SvNOKp(ver)) > @@ -674,6 +676,8 @@ > #if PERL_VERSION_LT(5,19,8) && defined(USE_ITHREADS) > LEAVE; > #endif > + > + SvREFCNT_inc_optimized(ver); > return ver; > } > > That schedules ver (which is rv from new_version) to be freed at the > end > of the scope, unless we are just about to return, where we inc it back > to life again. > > John

SvREFCNT_inc_optimized is psuedo-code. Pick the correct one from perlapi, NN simple void probably.

Sat Feb 01 14:27:57 2014 BULKDD [...] cpan.org - Correspondence added

I also forgot, there are 2 returns in that function. Both need the _inc.

Sat Feb 01 14:42:58 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92642] Leak in Perl_new_version
Date:	Sat, 01 Feb 2014 14:42:47 -0500
To:	bug-version [...] rt.cpan.org, jpeacock [...] cpan.org
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 02:27 PM, Daniel Dragan via RT wrote: Show quoted text

> SvREFCNT_inc_optimized is psuedo-code. Pick the correct one from perlapi, NN simple void probably.

Sorry, I didn't check. Yes, NN seems appropriate. And those changes are are isolated to upg_version itself, which only has a single return site. John

Sat Feb 01 14:44:44 2014 jpeacock [...] cpan.org - Taken

Sat Feb 01 14:44:44 2014 jpeacock [...] cpan.org - Requestor JPEACOCK deleted

Sat Feb 01 18:41:40 2014 BULKDD [...] cpan.org - Correspondence added

On Sat Feb 01 14:09:18 2014, JPEACOCK wrote: Show quoted text

> There is also a leak in Perl_new_version since if UPG_VERSION croaks, > SV "rv" is leaked. Either a early sv_2mortal and at the very end, > after UPG_VERSION (and it is guarenteed we can't croak), a > SvREFCNT_inc_optimized is required. Or replace the sv_2mortal with > SAVEFREESV.

https://bitbucket.org/jpeacock/version/commits/b56054bcaf22f5f9212e608638112e8441861a48 this is wrong. upg_version is public API. upg_version doesn't own the SV it is passed. What if you pass in a SV that was already mortalized in non-Peacock code, then mortal it again in upg_version, then croak non-numeric? "Attempt to free unreferenced scalar" time. It must be done in new_version, since new_version creates and owns that SV.

Sat Feb 01 18:51:15 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92642] Leak in Perl_new_version
Date:	Sat, 01 Feb 2014 18:50:54 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 06:41 PM, Daniel Dragan via RT wrote: Show quoted text

> > https://bitbucket.org/jpeacock/version/commits/b56054bcaf22f5f9212e608638112e8441861a48 this is wrong. upg_version is public API. upg_version doesn't own the SV it is passed. What if you pass in a SV that was already mortalized in non-Peacock code, then mortal it again in upg_version, then croak non-numeric? "Attempt to free unreferenced scalar" time. It must be done in new_version, since new_version creates and owns that SV. >

Is this better (obviously taking out the other changeset)? diff -r 863c77584d65 vutil/vutil.c --- a/vutil/vutil.c Sat Feb 01 18:44:23 2014 -0500 +++ b/vutil/vutil.c Sat Feb 01 18:48:56 2014 -0500 @@ -525,7 +525,8 @@ } } #endif - return UPG_VERSION(rv, FALSE); + sv_2mortal(rv); /* in case upg_version croaks before it returns */ + return SvREFCNT_inc_NN(UPG_VERSION(rv, FALSE)); } The other return in new_version is from the block where we are copying from an existing object, so upg_version never gets called. John

Sun Feb 02 11:48:26 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92642] Leak in Perl_new_version
Date:	Sun, 02 Feb 2014 11:48:15 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 06:51 PM, John Peacock via RT wrote: Show quoted text

> Is this better (obviously taking out the other changeset)? > > diff -r 863c77584d65 vutil/vutil.c > --- a/vutil/vutil.c Sat Feb 01 18:44:23 2014 -0500 > +++ b/vutil/vutil.c Sat Feb 01 18:48:56 2014 -0500 > @@ -525,7 +525,8 @@ > } > } > #endif > - return UPG_VERSION(rv, FALSE); > + sv_2mortal(rv); /* in case upg_version croaks before it returns */ > + return SvREFCNT_inc_NN(UPG_VERSION(rv, FALSE)); > }

Pushed that change to the repository and I'm ready to release to CPAN. John

Mon Feb 03 11:29:43 2014 BULKDD [...] cpan.org - Correspondence added

On Sat Feb 01 18:51:15 2014, john.peacock@havurah-software.org wrote: Show quoted text

> Is this better (obviously taking out the other changeset)? > > diff -r 863c77584d65 vutil/vutil.c > --- a/vutil/vutil.c Sat Feb 01 18:44:23 2014 -0500 > +++ b/vutil/vutil.c Sat Feb 01 18:48:56 2014 -0500 > @@ -525,7 +525,8 @@ > } > } > #endif > - return UPG_VERSION(rv, FALSE); > + sv_2mortal(rv); /* in case upg_version croaks before it returns > */ > + return SvREFCNT_inc_NN(UPG_VERSION(rv, FALSE)); > } > > > The other return in new_version is from the block where we are copying > from an existing object, so upg_version never gets called. > > John

I looked at this and the github head, its fine now.

Mon Feb 03 15:59:06 2014 jpeacock [...] cpan.org - Correspondence added

Fixed in 0.9908 Thanks

Mon Feb 03 15:59:06 2014 jpeacock [...] cpan.org - Status changed from 'open' to 'resolved'

Mon Feb 03 15:59:07 2014 jpeacock [...] cpan.org - Fixed in 0.9908 added

Bug #92642 for version: Leak in Perl_new_version