Perl_reg_named_buff_fetch may be allocating an AV it doesn't need to

[p5pRT]

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

Searchable as RT130822$

[p5pRT]

From @petdance

Created by @petdance

Look at the retarray variable in Perl_reg_named_buff_fetch in regcomp.c.

Here's an abbreviated version of the function​:

  Perl_reg_named_buff_fetch
  {
  A AV *retarray = NULL;
  B if (flags & RXapif_ALL)
  C retarray=newAV();
  D if (rx && RXp_PAREN_NAMES(rx)) {
  E HE *he_str = hv_fetch_ent( RXp_PAREN_NAMES(rx), namesv, 0, 0 );
  F if (he_str) {
  /* All sorts of manipulation of retarray */
  ...
  if (retarray)
  return newRV_noinc(MUTABLE_SV(retarray));
  }
  }
  return NULL;
  }

The way I read this, retarray is getting a newAV() even though it might
not going to get used.

If the condition at line D is never taken, then retarray will never
be used. It looks like we don't need to do the retarray=newAV() unless
and until the path at line F is taken. We could move lines A-C to just
after line F.

Can someone who knows more internals than me check this out?

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.24.0:

Configured by andy at Sun Jun  5 23:28:46 CDT 2016.

Summary of my perl5 (revision 5 version 24 subversion 0) configuration:
   
  Platform:
    osname=linux, osvers=3.10.0-327.18.2.el7.x86_64, archname=x86_64-linux
    uname='linux clifford 3.10.0-327.18.2.el7.x86_64 #1 smp thu may 12 11:03:55 utc 2016 x86_64 x86_64 x86_64 gnulinux '
    config_args='-de -Dprefix=/home/andy/perl5/perlbrew/perls/perl-5.24.0 -Aeval:scriptdir=/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, 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 -D_FORTIFY_SOURCE=2',
    optimize='-O2',
    cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion='', gccversion='4.8.5 20150623 (Red Hat 4.8.5-4)', 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='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, 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 -lnsl -ldl -lm -lcrypt -lutil -lc
    perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.17.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.17'
  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'

Locally applied patches:
    Devel::PatchPerl 1.38


@INC for perl 5.24.0:
    /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0/x86_64-linux
    /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0
    /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0/x86_64-linux
    /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0
    .


Environment for perl 5.24.0:
    HOME=/home/andy
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin:/home/andy/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin
    PERLBREW_BASHRC_VERSION=0.75
    PERLBREW_HOME=/home/andy/.perlbrew
    PERLBREW_MANPATH=/home/andy/perl5/perlbrew/perls/perl-5.24.0/man
    PERLBREW_PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin
    PERLBREW_PERL=perl-5.24.0
    PERLBREW_ROOT=/home/andy/perl5/perlbrew
    PERLBREW_VERSION=0.75
    PERLCRITIC=/home/andy/tw/Dev/perlcriticrc
    PERL_BADLANG (unset)
    SHELL=/bin/bash

[p5pRT]

From @tonycoz

On Sun, 19 Feb 2017 21​:50​:53 -0800, petdance wrote​:

regcomp.c.

Here's an abbreviated version of the function​:

Perl_reg_named_buff_fetch
{
A AV *retarray = NULL;
B if (flags & RXapif_ALL)
C retarray=newAV();
D if (rx && RXp_PAREN_NAMES(rx)) {
E HE *he_str = hv_fetch_ent( RXp_PAREN_NAMES(rx), namesv,
0, 0 );
F if (he_str) {
/* All sorts of manipulation of retarray */
...
if (retarray)
return newRV_noinc(MUTABLE_SV(retarray));
}
}
return NULL;
}

The way I read this, retarray is getting a newAV() even though it
might
not going to get used.

If the condition at line D is never taken, then retarray will never
be used. It looks like we don't need to do the retarray=newAV()
unless
and until the path at line F is taken. We could move lines A-C to
just
after line F.

Can someone who knows more internals than me check this out?

You seem correct to me.

As is this will leak an AV on each call if the last regexp didn't have named groups​:

# let's leak
./miniperl -e '"x" =~ /x/; while (1) { re​::regname("foo", 1) }'

  PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
29492 tony 20 0 20.000t 1.208g 11280 R 99.9 32.3 0​:15.43 miniperl

Did you want to submit a patch?

Tony

[p5pRT]

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

[p5pRT]

From @petdance

How much memory is in an AV?

[p5pRT]

From @tonycoz

On Mon, 20 Feb 2017 08​:39​:11 -0800, petdance wrote​:

How much memory is in an AV?

On 64-bit systems, 16 bytes for the head, 40 bytes + allocation overhead for the body.

Perl_reg_named_buff_fetch() is part of the implementation of %- and %+ too, so code like​:

  ./perl -Ilib -e '"x" =~ /x/; while (1) { $-{foo} }'

leaks in the same way as my first example.

Tony

[p5pRT]

From @tonycoz

On Mon, 20 Feb 2017 20​:57​:23 -0800, tonyc wrote​:

On Mon, 20 Feb 2017 08​:39​:11 -0800, petdance wrote​:

How much memory is in an AV?

On 64-bit systems, 16 bytes for the head, 40 bytes + allocation
overhead for the body.

Perl_reg_named_buff_fetch() is part of the implementation of %- and %+
too, so code like​:

./perl -Ilib -e '"x" =~ /x/; while (1) { $-{foo} }'

leaks in the same way as my first example.

I've fixed this in 853eb96.

Tony

[p5pRT]

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

[p5pRT]

From @petdance

On Feb 20, 2017, at 11​:50 PM, Tony Cook via RT <perlbug-followup@​perl.org> wrote​:

I've fixed this in 853eb96.

Thanks. I had the same patch on a branch but didn’t have time to send it in last night. I didn’t know about svleak.t, so thumbs up on that.

Shouldn’t this go into the release notes somewhere, that we fixed a memory leak?

--
Andy Lester => www.petdance.com

[p5pRT]

From @tonycoz

On Tue, 21 Feb 2017 06​:48​:10 -0800, petdance wrote​:

Thanks. I had the same patch on a branch but didn’t have time to send
it in last night. I didn’t know about svleak.t, so thumbs up on that.

Shouldn’t this go into the release notes somewhere, that we fixed a
memory leak?

It's now in perldelta.

Tony

[p5pRT]

From @khwilliamson

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

With the release today of Perl 5.26.0, this and 210 other issues have been
resolved.

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

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

[p5pRT]

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