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

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

Closed
p5pRT opened this issue Feb 20, 2017 · 11 comments
Closed

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

p5pRT opened this issue Feb 20, 2017 · 11 comments

Comments

@p5pRT
Copy link

p5pRT commented Feb 20, 2017

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

Searchable as RT130822$

@p5pRT
Copy link
Author

p5pRT commented Feb 20, 2017

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

p5pRT commented Feb 20, 2017

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

p5pRT commented Feb 20, 2017

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

@p5pRT
Copy link
Author

p5pRT commented Feb 20, 2017

From @petdance

How much memory is in an AV?

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2017

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

p5pRT commented Feb 21, 2017

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

p5pRT commented Feb 21, 2017

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

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2017

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

p5pRT commented Feb 21, 2017

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

p5pRT commented May 30, 2017

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

p5pRT commented May 30, 2017

@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