Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak in concatenation operator overload #16823

Closed
p5pRT opened this issue Jan 22, 2019 · 9 comments
Closed

Memory leak in concatenation operator overload #16823

p5pRT opened this issue Jan 22, 2019 · 9 comments

Comments

@p5pRT
Copy link

p5pRT commented Jan 22, 2019

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

Searchable as RT133789$

@p5pRT
Copy link
Author

p5pRT commented Jan 22, 2019

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
Copy link
Author

p5pRT commented Jan 23, 2019

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
Copy link
Author

p5pRT commented Jan 23, 2019

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

@p5pRT
Copy link
Author

p5pRT commented Jan 24, 2019

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
Copy link
Author

p5pRT commented Jan 25, 2019

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
Copy link
Author

p5pRT commented Feb 5, 2019

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
Copy link
Author

p5pRT commented Feb 18, 2019

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

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

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
Copy link
Author

p5pRT commented May 22, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant