Skip Menu |
Report information
Id: 130822
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: petdance <andy [at] petdance.com>
Cc:
AdminCc:

Operating System: Linux
PatchStatus: (no value)
Severity: low
Type: core
Perl Version: 5.24.0
Fixed In: (no value)



To: perlbug [...] perl.org
Date: Mon, 20 Feb 2017 00:50:10 -0500 (EST)
From: andy [...] petdance.com
Subject: Perl_reg_named_buff_fetch may be allocating an AV it doesn't need to
Download (untitled) / with headers
text/plain 4.4k
This is a bug report for perl from andy@petdance.com, generated with the help of perlbug 1.40 running under perl 5.24.0. ----------------------------------------------------------------- [Please describe your issue here] 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? [Please do not change anything below this line] ----------------------------------------------------------------- --- 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
On Sun, 19 Feb 2017 21:50:53 -0800, petdance wrote: Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
How much memory is in an AV?
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 366b
On Mon, 20 Feb 2017 08:39:11 -0800, petdance wrote: Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 471b
On Mon, 20 Feb 2017 20:57:23 -0800, tonyc wrote: Show quoted text
> 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 853eb961c1. Tony
CC: perl5-porters [...] perl.org
Date: Tue, 21 Feb 2017 08:47:29 -0600
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Subject: Re: [perl #130822] Perl_reg_named_buff_fetch may be allocating an AV it doesn't need to
From: Andy Lester <andy [...] petdance.com>
Download (untitled) / with headers
text/plain 390b

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

I've fixed this in 853eb961c1.

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

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 322b
On Tue, 21 Feb 2017 06:48:10 -0800, petdance wrote: Show quoted text
> 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
Download (untitled) / with headers
text/plain 313b
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.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org