develooper Front page | perl.perl5.porters | Postings from February 2019

Re: [perl #133789] Memory leak in concatenation operator overload

Thread Previous
From:
Dave Mitchell
Date:
February 5, 2019 14:15
Subject:
Re: [perl #133789] Memory leak in concatenation operator overload
Message ID:
20190205141514.GB14742@iabyn.com
On Thu, Jan 24, 2019 at 12:00:41PM +0000, Dave Mitchell wrote:
> On Tue, Jan 22, 2019 at 04:17:47PM -0800, Tony Cook via RT wrote:
> > > Refs count should stay untouched, but it increased on each iteration.
> > 
> > This is present in blead too.
> > 
> > There's two places the reference count is being incremented.
> > 
>  ...
> 
> > The second is in Perl_pp_multiconcat at the end of the magical value fallback code:
> > 
> > (gdb) bt
> > #0  S_SvREFCNT_inc (sv=0x555555d204a8) at inline.h:194
> > #1  0x000055555578ab26 in Perl_sv_setsv_flags (dstr=0x555555d43580, 
> >     sstr=0x555555d39880, flags=1538) at sv.c:4261
> > #2  0x0000555555748d85 in Perl_pp_multiconcat () at pp_hot.c:1106
> > 
> >         if (is_append)
> >             SvTAINT(targ);
> >         else {
> >             sv_setsv(targ, left); <---
> >             SvSETMAGIC(targ);
> >         }
> > 
> > targ here comes from the PAD:
> > 
> >         SV **svp = &(PAD_SVl(PL_op->op_targ));
> >         targ = *svp;
> 
> After each call to multiconcat that returns an overloaded value, its targ
> will end up holding an extra ref to the overloaded value. So its not a
> open-ended leak in the sense that calling multiconcat in a loop won't leak
> more and more objects, but just delay freeing the last object.
> 
> I'll need to think a bit about the best way to avoid assigning to the
> targ.

Now fixed in blead with this commit:

    commit 4e521aaf3ed717774455b3906bd5aa46bc397319
    Author:     David Mitchell <davem@iabyn.com>
    AuthorDate: Tue Feb 5 13:48:21 2019 +0000

    Avoid leak in multiconcat with overloading.
    
    RT #133789
    
    In the path taken through pp_multiconcat() when one or more args have
    side-effects such tieing or overloading, multiconcat has to decide
    whether to just return the result of all the concatting as-is, or to
    first assign it to an expression or variable if the op includes an
    implicit assign (such as $lex = x.y.z or $a[0] = x.y.z).
    
    The code was getting this right for those two cases, and was also
    getting it right for the append cases ($lex .= x.y.z and $a[0] .= x.y.z),
    which don't need assigns. But for the bare case (x.y.z) it was assigning
    to the op's targ as well as returning the value. Hence leaking a
    reference until destruction of the sub and its pad.
    
    This commit stops the assign in that last case.

which is part of this just-pushed branch:

    Author:     David Mitchell <davem@iabyn.com>
    AuthorDate: Tue Feb 5 14:04:32 2019 +0000

    [MERGE] various overload fixups
    
    This branch contains several commits which simplify the code concerning
    the processing of a value returned by an overload method, and
    specifically whether that value should be returned as-is by the op, or
    assigned to the targ / stack value: $lex = x op y) and (x op= y)
    respectively.
    
    The final commit fixes a bug in pp_multiconcat. That op bypasses most of
    the code in those earlier commits and "rolls it's own", and which was
    getting the set/assign decision wrong in some cases, causing a leak.


-- 
"Foul and greedy Dwarf - you have eaten the last candle."
    -- "Hordes of the Things", BBC Radio.

Thread Previous


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About