Memory leak in concatenation operator overload #16823

p5pRT · 2019-01-22T10:50:40Z

Migrated from rt.perl.org#133789 (status was 'resolved')

Searchable as RT133789$

p5pRT · 2019-01-22T10:50:40Z

From zdm@softvisio.net

Created by zdm@softvisio.net

New memory leak was introduces in perl 5.28.
Please, see the discussion here:
https://stackoverflow.com/questions/54304341/perl-overload-concatenation-operator-problem

#!/usr/bin/env perl

use strict;
use warnings;
use Devel::Refcount qw[refcount];

package AAA {
use Devel::Refcount qw[refcount];

use overload
'.' => sub {
print 'CONCAT, REFCOUNT: ', refcount( $_[0] ), "\n";

# return AAA->new;
return $_[0];
},
fallback => 0;

sub new { return bless {}, $_[0] }

sub DESTROY { print "DESTROY\n" }
}

print "--- start\n";

{
my $o = AAA->new;

my $s = '1' . ( '2' . ( '3' . ( '4' . ( '5' . $o ) ) ) );

print "--- exit scope\n";
print 'REFCOUNT: ', refcount($o), "\n";
}

print "--- end\n";

1;

Output:

--- start
CONCAT, REFCOUNT: 1
CONCAT, REFCOUNT: 3
CONCAT, REFCOUNT: 5
CONCAT, REFCOUNT: 7
CONCAT, REFCOUNT: 9
--- exit scope
REFCOUNT: 6
--- end
DESTROY

Refs count should stay untouched, but it increased on each iteration.

Perl Info


Flags:
    category=core
    severity=high

Site configuration information for perl 5.28.1:

Configured by mockbuild at Mon Jan 21 14:05:45 UTC 2019.

Summary of my perl5 (revision 5 version 28 subversion 1) configuration:

  Platform:
    osname=linux
    osvers=4.16.13-300.fc28.x86_64
    archname=x86_64-linux-ld
    uname='linux 0b8553fb90eb4b24a5147b6ec2b37883
4.16.13-300.fc28.x86_64 #1 smp wed may 30 14:31:00 utc 2018 x86_64
x86_64 x86_64 gnulinux '
    config_args='-des -Dusemorebits -Duselargefiles
-Dprefix=/usr/perlbrew/perls/perl-5.28.1 -Duserelocatableinc
-Dman1dir=none -Dman3dir=none'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=define
    usemymalloc=n
    default_inc_excludes_dot=define
    bincompat5005=undef
  Compiler:
    cc='cc'
    ccflags ='-fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64'
    optimize='-O2'
    cppflags='-fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='8.2.1 20181215 (Red Hat 8.2.1-6)'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long'
    ivsize=8
    nvtype='long double'
    nvsize=16
    Off_t='off_t'
    lseeksize=8
    alignbytes=16
    prototype=define
  Linker and Libraries:
    ld='cc'
    ldflags =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib
/lib64 /usr/lib64 /usr/local/lib64
    libs=-lpthread -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    perllibs=-lpthread -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.28.so
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version='2.28'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E'
    cccdlflags='-fPIC'
    lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'



@INC for perl 5.28.1:
    /mnt/hgfs/projects/pcore-lib/pcore/lib
    /usr/perlbrew/perls/perl-5.28.1/lib/site_perl/5.28.1/x86_64-linux-ld
    /usr/perlbrew/perls/perl-5.28.1/lib/site_perl/5.28.1
    /usr/perlbrew/perls/perl-5.28.1/lib/5.28.1/x86_64-linux-ld
    /usr/perlbrew/perls/perl-5.28.1/lib/5.28.1


Environment for perl 5.28.1:
    HOME=/root
    LANG=en_US.UTF-8
    LANGUAGE=en_US.UTF-8
    LC_ALL=en_US.UTF-8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)

PATH=/usr/perlbrew/bin:/usr/perlbrew/perls/perl-5.28.1/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/mnt/hgfs/projects/pcore-lib/pcore/bin
    PERL5LIB=/mnt/hgfs/projects/pcore-lib/pcore/lib
    PERLBREW=command perlbrew
    PERLBREW_HOME=/usr/perlbrew
    PERLBREW_MANPATH=/usr/perlbrew/perls/perl-5.28.1/man
    PERLBREW_PATH=/usr/perlbrew/bin:/usr/perlbrew/perls/perl-5.28.1/bin
    PERLBREW_PERL=perl-5.28.1
    PERLBREW_ROOT=/usr/perlbrew
    PERLBREW_SHELLRC_VERSION=0.86
    PERLBREW_VERSION=0.86
    PERL_BADLANG (unset)
    PERL_CPANM_HOME=/tmp/.cpanm
    PERL_CPANM_OPT=--metacpan --from https://cpan.metacpan.org/
    SHELL=/bin/bash

p5pRT · 2019-01-23T00:17:47Z

From @tonycoz

On Tue, 22 Jan 2019 02:50:40 -0800, zdm@softvisio.net wrote:

New memory leak was introduces in perl 5.28.
Please, see the discussion here:
https://stackoverflow.com/questions/54304341/perl-overload-
concatenation-operator-problem

#!/usr/bin/env perl

use strict;
use warnings;
use Devel::Refcount qw[refcount];

package AAA {
use Devel::Refcount qw[refcount];

use overload
'.' => sub {
print 'CONCAT, REFCOUNT: ', refcount( $_[0] ), "\n";

# return AAA->new;
return $_[0];
},
fallback => 0;

sub new { return bless {}, $_[0] }

sub DESTROY { print "DESTROY\n" }
}

print "--- start\n";

{
my $o = AAA->new;

my $s = '1' . ( '2' . ( '3' . ( '4' . ( '5' . $o ) ) ) );

print "--- exit scope\n";
print 'REFCOUNT: ', refcount($o), "\n";
}

print "--- end\n";

1;

Output:

--- start
CONCAT, REFCOUNT: 1
CONCAT, REFCOUNT: 3
CONCAT, REFCOUNT: 5
CONCAT, REFCOUNT: 7
CONCAT, REFCOUNT: 9
--- exit scope
REFCOUNT: 6
--- end
DESTROY

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 first is in leave_adjust_stacks() as the return from the '.' overload is being processed:

(gdb) bt
#0 S_SvREFCNT_inc (sv=0x555555d204a8) at inline.h:194
#1 0x0000555555760634 in Perl_leave_adjust_stacks (from_sp=0x555555d4ae10,
to_sp=0x555555d4ae08, gimme=2 '\002', pass=0) at pp_hot.c:4793
#2 0x0000555555760a48 in Perl_pp_leavesub () at pp_hot.c:4902
#3 0x00005555556f4b41 in Perl_runops_debug () at dump.c:2536
#4 0x0000555555607358 in Perl_amagic_call (left=0x555555d39c40,
right=0x555555d43580, method=70, flags=0) at gv.c:3525
#5 0x0000555555748b19 in Perl_pp_multiconcat () at pp_hot.c:1060

Each of these is added to the tmps stack.

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;

Tony

p5pRT · 2019-01-23T00:17:47Z

The RT System itself - Status changed from 'new' to 'open'

p5pRT · 2019-01-24T12:01:04Z

From @iabyn

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.

--
Diplomacy is telling someone to go to hell in such a way that they'll
look forward to the trip

p5pRT · 2019-01-25T13:56:25Z

From @jkeenan

On Tue, 22 Jan 2019 10:50:40 GMT, zdm@softvisio.net wrote:

This is a bug report for perl from zdm@softvisio.net,
generated with the help of perlbug 1.41 running under perl 5.28.1.

-----------------------------------------------------------------
[Please describe your issue here]

New memory leak was introduces in perl 5.28.
Please, see the discussion here:
https://stackoverflow.com/questions/54304341/perl-overload-
concatenation-operator-problem

#!/usr/bin/env perl

use strict;
use warnings;
use Devel::Refcount qw[refcount];

package AAA {
use Devel::Refcount qw[refcount];

use overload
'.' => sub {
print 'CONCAT, REFCOUNT: ', refcount( $_[0] ), "\n";

# return AAA->new;
return $_[0];
},
fallback => 0;

sub new { return bless {}, $_[0] }

sub DESTROY { print "DESTROY\n" }
}

print "--- start\n";

{
my $o = AAA->new;

my $s = '1' . ( '2' . ( '3' . ( '4' . ( '5' . $o ) ) ) );

print "--- exit scope\n";
print 'REFCOUNT: ', refcount($o), "\n";
}

print "--- end\n";

1;

Output:

--- start
CONCAT, REFCOUNT: 1
CONCAT, REFCOUNT: 3
CONCAT, REFCOUNT: 5
CONCAT, REFCOUNT: 7
CONCAT, REFCOUNT: 9
--- exit scope
REFCOUNT: 6
--- end
DESTROY

Refs count should stay untouched, but it increased on each iteration.

FWIW, bisection points to this commit:

#####
e839e6e is the first bad commit
commit e839e6e
Author: David Mitchell <davem@iabyn.com>
Date: Tue Aug 8 18:42:14 2017 +0100

Add OP_MULTICONCAT op

Allow multiple OP_CONCAT, OP_CONST ops, plus optionally an OP_SASSIGN
or OP_STRINGIFY, to be combined into a single OP_MULTICONCAT op, which can
make things a *lot* faster: 4x or more.
...
#####

Thank you very much.

--
James E Keenan (jkeenan@cpan.org)

p5pRT · 2019-02-05T14:15:28Z

From @iabyn

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 4e521aa
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.

p5pRT · 2019-02-18T09:05:09Z

@iabyn - Status changed from 'open' to 'pending release'

p5pRT · 2019-05-22T15:07:08Z

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.30.0, this and 160 other issues have been
resolved.

Perl 5.30.0 may be downloaded via:
https://metacpan.org/release/XSAWYERX/perl-5.30.0

If you find that the problem persists, feel free to reopen this ticket.

p5pRT · 2019-05-22T15:07:08Z

@khwilliamson - Status changed from 'pending release' to 'resolved'

p5pRT closed this as completed May 22, 2019

p5pRT added Severity High type-core labels Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory leak in concatenation operator overload #16823

Memory leak in concatenation operator overload #16823

p5pRT commented Jan 22, 2019

p5pRT commented Jan 22, 2019

p5pRT commented Jan 23, 2019

p5pRT commented Jan 23, 2019

p5pRT commented Jan 24, 2019

p5pRT commented Jan 25, 2019

p5pRT commented Feb 5, 2019

p5pRT commented Feb 18, 2019

p5pRT commented May 22, 2019

p5pRT commented May 22, 2019

Memory leak in concatenation operator overload #16823

Memory leak in concatenation operator overload #16823

Comments

p5pRT commented Jan 22, 2019

p5pRT commented Jan 22, 2019

From zdm@softvisio.net

Created by zdm@softvisio.net

p5pRT commented Jan 23, 2019

From @tonycoz

p5pRT commented Jan 23, 2019

p5pRT commented Jan 24, 2019

From @iabyn

p5pRT commented Jan 25, 2019

From @jkeenan

p5pRT commented Feb 5, 2019

From @iabyn

p5pRT commented Feb 18, 2019

p5pRT commented May 22, 2019

From @khwilliamson

p5pRT commented May 22, 2019