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 perl 5.24.0 when use re qw[strict] is used #15382

Closed
p5pRT opened this issue Jun 3, 2016 · 33 comments
Closed

Memory leak in perl 5.24.0 when use re qw[strict] is used #15382

p5pRT opened this issue Jun 3, 2016 · 33 comments

Comments

@p5pRT
Copy link

p5pRT commented Jun 3, 2016

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

Searchable as RT128313$

@p5pRT
Copy link
Author

p5pRT commented Jun 3, 2016

From zdm@softvisio.net

Created by zdm@softvisio.net

Reply-To​: zdm@​softvisio.net
From​: zdm@​softvisio.net
Message-Id​: <5.24.0_3968_1464951615@​WIN-OPJJ674O0JK>
To​: perlbug@​perl.org
Subject​: Memory leak in perl 5.24.0 when use re qw[strict] is used

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

-----------------------------------------------------------------

Hi.

This code produce memory leak​:

use re qw[strict];
my $s = 'aaa';
while (1) {
  my $ps = 'aa';
  $s =~ /[^.]+\.$ps\z/sm;
}

If I remove "use re qw[strict];" - mem. leak disappearing.

Perl Info

Flags:
    category=core
    severity=high

Site configuration information for perl 5.24.0:

Configured by strawberry-perl at Tue May 10 21:33:22 2016.

Summary of my perl5 (revision 5 version 24 subversion 0) configuration:

  Platform:
    osname=MSWin32, osvers=6.3, archname=MSWin32-x64-multi-thread
    uname='Win32 strawberry-perl 5.24.0.1 #1 Tue May 10 21:30:49 2016 x64'
    config_args='undef'
    hint=recommended, useposix=true, d_sigaction=undef
    useithreads=define, usemultiplicity=define
    use64bitint=define, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags =' -s -O2 -DWIN32 -DWIN64 -DCONSERVATIVE
-DPERL_TEXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS
-fwrapv -fno-strict-aliasing -mms-bitfields',
    optimize='-s -O2',
    cppflags='-DWIN32'
    ccversion='', gccversion='4.9.2', gccosandvers=''
    intsize=4, longsize=4, ptrsize=8, doublesize=8, byteorder=12345678,
doublekind=3
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16,
longdblkind=3
    ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='long
long', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='g++.exe', ldflags ='-s -L"D:\devel\perl\perl\lib\CORE"
-L"D:\devel\perl\c\lib"'
    libpth=D:\devel\perl\c\lib D:\devel\perl\c\x86_64-w64-mingw32\lib
D:\devel\perl\c\lib\gcc\x86_64-w64-mingw32\4.9.2
    libs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32
-ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr
-lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    perllibs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool
-lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid
-lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    libc=, so=dll, useshrplib=true, libperl=libperl524.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs, dlext=xs.dll, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags='-mdll -s -L"D:\devel\perl\perl\lib\CORE"
-L"D:\devel\perl\c\lib"'



@INC for perl 5.24.0:
    d:/projects/pcore/lib
    d:/devel/perl/perl/site/lib/MSWin32-x64-multi-thread
    d:/devel/perl/perl/site/lib
    d:/devel/perl/perl/vendor/lib
    d:/devel/perl/perl/lib
    .


Environment for perl 5.24.0:
    HOME (unset)
    LANG=en_US
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)

PATH=C:\Windows;C:\Windows\system32;C:\Windows\system32\wbem;C:\Windows\System32\WindowsPowerShell\v1.0;d:\apps\bin;d:\apps\gpg;d:\devel\bin;d:\devel\perl\perl\site\bin;d:\devel\perl\perl\bin;d:\devel\perl\c\bin;d:\devel\hg;d:\devel\jre\bin;d:\devel\sencha\sencha-cmd;d:\devel\nodejs;d:\devel\msys2\usr\bin;d:\projects\pcore\bin;;d:/data
    PERL5LIB=d:/projects/pcore/lib;
    PERL_BADLANG (unset)
    SHELL (unset)

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @dcollinsn

Confirmed in blead on linux.

Valgrind with --leak-check=full --show-leak-kinds=all reports several records of the type​:

  ==9552== 7,339,416 bytes in 53,965 blocks are still reachable in loss record 651 of 651
  ==9552== at 0x4C2AB5C​: realloc (vg_replace_malloc.c​:785)
  ==9552== by 0x55BF69​: Perl_safesysrealloc (util.c​:274)
  ==9552== by 0x5C83D1​: Perl_sv_grow (sv.c​:1602)
  ==9552== by 0x5ED031​: Perl_sv_catpvn_flags (sv.c​:5414)
  ==9552== by 0x60DD01​: Perl_sv_vcatpvfn_flags (sv.c​:11499)
  ==9552== by 0x60CC6C​: Perl_sv_vsetpvfn (sv.c​:10834)
  ==9552== by 0x604835​: Perl_vnewSVpvf (sv.c​:9448)
  ==9552== by 0x604738​: Perl_newSVpvf (sv.c​:9433)
  ==9552== by 0x52A809​: S_handle_possible_posix (regcomp.c​:14050)
  ==9552== by 0x53246B​: S_regclass (regcomp.c​:15853)
  ==9552== by 0x52287F​: S_regatom (regcomp.c​:12392)
  ==9552== by 0x51D66D​: S_regpiece (regcomp.c​:11483)

I was able to minimize the test case to​:

  use re qw[strict];
  my $s = 'aaa';
  my $ps = 'aa';
  while (1) {
  $s =~ /[^.]+$ps/;
  }

The memory is allocated at 14050​:

  ADD_POSIX_WARNING(p, "there must be a starting '​:'");

The function will eventually return at 14498​:

  return NOT_MEANT_TO_BE_A_POSIX_CLASS;

Leaving AV* warn_text not mortalized, since that doesn't happen until 14533​:

  if (posix_warnings) {
  /* mortalize to avoid a leak with FATAL warnings */
  *posix_warnings = (AV *) sv_2mortal((SV *) warn_text);
  }
  else {
  SvREFCNT_dec_NN(warn_text);
  }

A possible solution is to mortalize warn_text at every location where S_handle_possible_posix can return NOT_MEANT_TO_BE_A_POSIX_CLASS. This is a bit ugly since I had to copy/paste the code to each of 6 locations, but it's the only way I could get `make test` to pass. At first, I tried making warn_text mortal when it's first allocated, but that seemed to make it get freed too early.

No idea how to write a test for this.

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @dcollinsn

0001-perl-128313-Fix-memory-leak-in-POSIX-class-warnings.patch
From e39d6012d7aafe4de01afea5c32965e69936dd76 Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Wed, 8 Jun 2016 11:26:29 -0400
Subject: [PATCH] [perl #128313] Fix memory leak in POSIX class warnings

Certain classes that "may be" POSIX classes result in POSIX warnings
being added to warn_text, but never freed, resulting in a slow but
present memory leak. We need to mortalize warn_text.

warn_text is presently mortalized or freed late in the function,
just before it would return successfully. However, there are a number
of points where this function can return failure. If a POSIX warning
is generated and the function returns before warn_text can be made
mortal, it will never be freed.
---
 regcomp.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/regcomp.c b/regcomp.c
index 0b1e606..0566d0e 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -14072,6 +14072,9 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
         /* We consider something like [^:^alnum:]] to not have been intended to
          * be a posix class, but XXX maybe we should */
         if (complement) {
+            if (warn_text) {
+                warn_text = (AV *)sv_2mortal((SV *) warn_text);
+            }
             return NOT_MEANT_TO_BE_A_POSIX_CLASS;
         }
 
@@ -14099,6 +14102,9 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
          * this leaves this construct looking like [:] or [:^], which almost
          * certainly weren't intended to be posix classes */
         if (has_opening_bracket) {
+            if (warn_text) {
+                warn_text = (AV *)sv_2mortal((SV *) warn_text);
+            }
             return NOT_MEANT_TO_BE_A_POSIX_CLASS;
         }
 
@@ -14116,6 +14122,9 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
             /* XXX We are currently very restrictive here, so this code doesn't
              * consider the possibility that, say, /[alpha.]]/ was intended to
              * be a posix class. */
+            if (warn_text) {
+                warn_text = (AV *)sv_2mortal((SV *) warn_text);
+            }
             return NOT_MEANT_TO_BE_A_POSIX_CLASS;
         }
 
@@ -14286,6 +14295,9 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
             }
 
             /* Otherwise, it can't have meant to have been a class */
+            if (warn_text) {
+                warn_text = (AV *)sv_2mortal((SV *) warn_text);
+            }
             return NOT_MEANT_TO_BE_A_POSIX_CLASS;
         }
 
@@ -14336,6 +14348,9 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
          * class name.  (We can do this on the first pass, as any second pass
          * will yield an even shorter name) */
         if (name_len < 3) {
+            if (warn_text) {
+                warn_text = (AV *)sv_2mortal((SV *) warn_text);
+            }
             return NOT_MEANT_TO_BE_A_POSIX_CLASS;
         }
 
@@ -14495,6 +14510,9 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
             }
 
             /* Here neither pass found a close-enough class name */
+            if (warn_text) {
+                warn_text = (AV *)sv_2mortal((SV *) warn_text);
+            }
             return NOT_MEANT_TO_BE_A_POSIX_CLASS;
         }
 
-- 
2.8.1

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @demerphq

On 8 June 2016 at 17​:50, Dan Collins via RT <perlbug-followup@​perl.org> wrote​:

Confirmed in blead on linux.

Valgrind with --leak-check=full --show-leak-kinds=all reports several records of the type​:

==9552== 7\,339\,416 bytes in 53\,965 blocks are still reachable in loss record 651 of 651
==9552==    at 0x4C2AB5C&#8203;: realloc \(vg\_replace\_malloc\.c&#8203;:785\)
==9552==    by 0x55BF69&#8203;: Perl\_safesysrealloc \(util\.c&#8203;:274\)
==9552==    by 0x5C83D1&#8203;: Perl\_sv\_grow \(sv\.c&#8203;:1602\)
==9552==    by 0x5ED031&#8203;: Perl\_sv\_catpvn\_flags \(sv\.c&#8203;:5414\)
==9552==    by 0x60DD01&#8203;: Perl\_sv\_vcatpvfn\_flags \(sv\.c&#8203;:11499\)
==9552==    by 0x60CC6C&#8203;: Perl\_sv\_vsetpvfn \(sv\.c&#8203;:10834\)
==9552==    by 0x604835&#8203;: Perl\_vnewSVpvf \(sv\.c&#8203;:9448\)
==9552==    by 0x604738&#8203;: Perl\_newSVpvf \(sv\.c&#8203;:9433\)
==9552==    by 0x52A809&#8203;: S\_handle\_possible\_posix \(regcomp\.c&#8203;:14050\)
==9552==    by 0x53246B&#8203;: S\_regclass \(regcomp\.c&#8203;:15853\)
==9552==    by 0x52287F&#8203;: S\_regatom \(regcomp\.c&#8203;:12392\)
==9552==    by 0x51D66D&#8203;: S\_regpiece \(regcomp\.c&#8203;:11483\)

I was able to minimize the test case to​:

use re qw\[strict\];
my $s = 'aaa';
my $ps = 'aa';
while \(1\) \{
    $s =~ /\[^\.\]\+$ps/;
\}

The memory is allocated at 14050​:

ADD\_POSIX\_WARNING\(p\, "there must be a starting '&#8203;:'"\);

The function will eventually return at 14498​:

return NOT\_MEANT\_TO\_BE\_A\_POSIX\_CLASS;

Leaving AV* warn_text not mortalized, since that doesn't happen until 14533​:

if \(posix\_warnings\) \{
    /\* mortalize to avoid a leak with FATAL warnings \*/
    \*posix\_warnings = \(AV \*\) sv\_2mortal\(\(SV \*\) warn\_text\);
\}
else \{
    SvREFCNT\_dec\_NN\(warn\_text\);
\}

A possible solution is to mortalize warn_text at every location where S_handle_possible_posix can return NOT_MEANT_TO_BE_A_POSIX_CLASS. This is a bit ugly since I had to copy/paste the code to each of 6 locations, but it's the only way I could get `make test` to pass. At first, I tried making warn_text mortal when it's first allocated, but that seemed to make it get freed too early.

Hrm. Did the patch you tried look like this?

Inline Patch
diff --git a/regcomp.c b/regcomp.c
index 0b1e606..8562b8f 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -13764,7 +13764,7 @@ S_populate_ANYOF_from_invlist(pTHX_ regnode
*node, SV** invlist_ptr)   \* routine\. q\.v\. \*/ \#define ADD\_POSIX\_WARNING\(p\, text\) STMT\_START \{ \\   if \(posix\_warnings\) \{ \\ \- if \(\! warn\_text\) warn\_text = newAV\(\); \\ \+ if \(\! warn\_text\) warn\_text = \(AV \*\) sv\_2mortal\(\(SV \*\) newAV\(\)\); \\   av\_push\(warn\_text\, Perl\_newSVpvf\(aTHX\_ \\   WARNING\_PREFIX \\   text \\ @​@​ \-14530\,13 \+14530\,9 @​@​ S\_handle\_possible\_posix\(pTHX\_ RExC\_state\_t \*pRExC\_state\,   \}

  if (warn_text) {
- if (posix_warnings) {
- /* mortalize to avoid a leak with FATAL warnings */
- *posix_warnings = (AV *) sv_2mortal((SV *) warn_text);
- }
- else {
- SvREFCNT_dec_NN(warn_text);
- }
+ /* warn_text should only be true if posix_warnings is true */
+ assert(posix_warnings);
+ *posix_warnings = warn_text;
  }
  }
  else if (class_number != OOB_NAMEDCLASS) {

Yves

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @dcollinsn

Vaguely, yes...but I realize that I may not have set *posix_warnings =
warn_text;, and I may have accidentally commented out the whole line. :(
Does that pass tests for you?
On Jun 8, 2016 12​:23 PM, "demerphq" <demerphq@​gmail.com> wrote​:

On 8 June 2016 at 17​:50, Dan Collins via RT <perlbug-followup@​perl.org>
wrote​:

Confirmed in blead on linux.

Valgrind with --leak-check=full --show-leak-kinds=all reports several
records of the type​:

==9552== 7\,339\,416 bytes in 53\,965 blocks are still reachable in

loss record 651 of 651

==9552==    at 0x4C2AB5C&#8203;: realloc \(vg\_replace\_malloc\.c&#8203;:785\)
==9552==    by 0x55BF69&#8203;: Perl\_safesysrealloc \(util\.c&#8203;:274\)
==9552==    by 0x5C83D1&#8203;: Perl\_sv\_grow \(sv\.c&#8203;:1602\)
==9552==    by 0x5ED031&#8203;: Perl\_sv\_catpvn\_flags \(sv\.c&#8203;:5414\)
==9552==    by 0x60DD01&#8203;: Perl\_sv\_vcatpvfn\_flags \(sv\.c&#8203;:11499\)
==9552==    by 0x60CC6C&#8203;: Perl\_sv\_vsetpvfn \(sv\.c&#8203;:10834\)
==9552==    by 0x604835&#8203;: Perl\_vnewSVpvf \(sv\.c&#8203;:9448\)
==9552==    by 0x604738&#8203;: Perl\_newSVpvf \(sv\.c&#8203;:9433\)
==9552==    by 0x52A809&#8203;: S\_handle\_possible\_posix \(regcomp\.c&#8203;:14050\)
==9552==    by 0x53246B&#8203;: S\_regclass \(regcomp\.c&#8203;:15853\)
==9552==    by 0x52287F&#8203;: S\_regatom \(regcomp\.c&#8203;:12392\)
==9552==    by 0x51D66D&#8203;: S\_regpiece \(regcomp\.c&#8203;:11483\)

I was able to minimize the test case to​:

use re qw\[strict\];
my $s = 'aaa';
my $ps = 'aa';
while \(1\) \{
    $s =~ /\[^\.\]\+$ps/;
\}

The memory is allocated at 14050​:

ADD\_POSIX\_WARNING\(p\, "there must be a starting '&#8203;:'"\);

The function will eventually return at 14498​:

return NOT\_MEANT\_TO\_BE\_A\_POSIX\_CLASS;

Leaving AV* warn_text not mortalized, since that doesn't happen until
14533​:

if \(posix\_warnings\) \{
    /\* mortalize to avoid a leak with FATAL warnings \*/
    \*posix\_warnings = \(AV \*\) sv\_2mortal\(\(SV \*\) warn\_text\);
\}
else \{
    SvREFCNT\_dec\_NN\(warn\_text\);
\}

A possible solution is to mortalize warn_text at every location where
S_handle_possible_posix can return NOT_MEANT_TO_BE_A_POSIX_CLASS. This is a
bit ugly since I had to copy/paste the code to each of 6 locations, but
it's the only way I could get `make test` to pass. At first, I tried making
warn_text mortal when it's first allocated, but that seemed to make it get
freed too early.

Hrm. Did the patch you tried look like this?

diff --git a/regcomp.c b/regcomp.c
index 0b1e606..8562b8f 100644
--- a/regcomp.c
+++ b/regcomp.c
@​@​ -13764,7 +13764,7 @​@​ S_populate_ANYOF_from_invlist(pTHX_ regnode
*node, SV** invlist_ptr)
* routine. q.v. */
#define ADD_POSIX_WARNING(p, text) STMT_START {
\
if (posix_warnings) {
\
- if (! warn_text) warn_text = newAV();
\
+ if (! warn_text) warn_text = (AV *) sv_2mortal((SV *)
newAV()); \
av_push(warn_text, Perl_newSVpvf(aTHX_
\
WARNING_PREFIX
\
text
\
@​@​ -14530,13 +14530,9 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
}

         if \(warn\_text\) \{

- if (posix_warnings) {
- /* mortalize to avoid a leak with FATAL warnings */
- *posix_warnings = (AV *) sv_2mortal((SV *) warn_text);
- }
- else {
- SvREFCNT_dec_NN(warn_text);
- }
+ /* warn_text should only be true if posix_warnings is
true */
+ assert(posix_warnings);
+ *posix_warnings = warn_text;
}
}
else if (class_number != OOB_NAMEDCLASS) {

Yves

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @demerphq

On 8 June 2016 at 18​:27, Dan Collins <dcollinsn@​gmail.com> wrote​:

Vaguely, yes...but I realize that I may not have set *posix_warnings =
warn_text;, and I may have accidentally commented out the whole line. :(
Does that pass tests for you?

Yes it passed all tests here.

I pushed it just now​:

commit ee072c8
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Jun 8 18​:42​:30 2016 +0200

  [perl #128313] Fix leak in perl 5.24 with strict and regex posix
char classes

  This patch is a refinement of one written by Dan Collins.

  Any thanks for this patch should go to him.

Yves

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @khwilliamson

On 06/08/2016 10​:44 AM, demerphq wrote​:

On 8 June 2016 at 18​:27, Dan Collins <dcollinsn@​gmail.com> wrote​:

Vaguely, yes...but I realize that I may not have set *posix_warnings =
warn_text;, and I may have accidentally commented out the whole line. :(
Does that pass tests for you?

Yes it passed all tests here.

I pushed it just now​:

commit ee072c8
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Jun 8 18​:42​:30 2016 +0200

 \[perl \#128313\] Fix leak in perl 5\.24 with strict and regex posix

char classes

 This patch is a refinement of one written by Dan Collins\.

 Any thanks for this patch should go to him\.

Yves

I'm concerned that you guys are falling into the same trap I did
earlier. The problem with mortalizing is that it can lead to excessive
memory use, as those temps don't get freed immediately. See
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127392
In a large pattern, this can be called over and over and over. It may
not be a problem, but it might be, and that needs to be considered.
This doesn't appear to have the potential to use large amounts of memory
per scalar like what was reported in that ticket. So it may be ok.

If it isn't ok, one option is to add this mortalization only if it's re
'strict'. Another would be to make the return into a macro that frees
the space before returning. This would solve the 'ugliness' issue.

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @dcollinsn

Yves' patch changes warn_text from leaking to mortal if posix_warnings is set and the function ends early, no change in any other case. In other words, it /never/ called SvREFCNT_dec_NN(warn_text), either before or now.

Alternately, since posix_warnings isn't set if the function returns early, we can SvREFCNT_dec(warn_text) if we return early. My limited testing suggests that the memory advantage is minimal, but it's probably "more right" to free it immediately before returning. Is the attached patch what you were thinking? It still fixes this bug, and all tests still pass.

(Patch is against blead /before/ Yves' patch was applied, if you'd prefer a patch against blead, that can be arranged.

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @dcollinsn

0002-perl-128313-Fix-memory-leak-in-POSIX-class-warnings.patch
From b04aa5cec7c85fa03e65b7dd86394d818a3233eb Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Wed, 8 Jun 2016 14:26:05 -0400
Subject: [PATCH] [perl #128313] Fix memory leak in POSIX class warnings

Certain classes that "may be" POSIX classes result in POSIX warnings
being added to warn_text, but never freed, resulting in a slow but
present memory leak. We need to ensure that warn_text is freed.

warn_text is presently mortalized late in the function, when it is
assigned to *posix_warnings. However, certain cases can generate
a POSIX warning while also having the function return before that
point. If a POSIX warning is generated and the function returns
before warn_text can be made mortal, it will never be freed.

This patch performs a REFCNT_dec on warn_text immediately before
any early return.
---
 regcomp.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 0b1e606..6bd903d 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -13772,6 +13772,10 @@ S_populate_ANYOF_from_invlist(pTHX_ regnode *node, SV** invlist_ptr)
                                              REPORT_LOCATION_ARGS(p)));     \
         }                                                                   \
     } STMT_END
+#define RETURN_EARLY(retval) STMT_START {                                   \
+        if (warn_text) SvREFCNT_dec_NN(warn_text);                          \
+        return (retval);                                                    \
+    } STMT_END
 
 STATIC int
 S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
@@ -13913,7 +13917,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
     PERL_ARGS_ASSERT_HANDLE_POSSIBLE_POSIX;
 
     if (p >= e) {
-        return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+        RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
     }
 
     if (*(p - 1) != '[') {
@@ -14002,7 +14006,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
                     *updated_parse_ptr = (char *) temp_ptr;
                 }
 
-                return OOB_NAMEDCLASS;
+                RETURN_EARLY(OOB_NAMEDCLASS);
             }
         }
 
@@ -14072,7 +14076,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
         /* We consider something like [^:^alnum:]] to not have been intended to
          * be a posix class, but XXX maybe we should */
         if (complement) {
-            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
         }
 
         complement = 1;
@@ -14099,7 +14103,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
          * this leaves this construct looking like [:] or [:^], which almost
          * certainly weren't intended to be posix classes */
         if (has_opening_bracket) {
-            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
         }
 
         /* But this function can be called when we parse the colon for
@@ -14116,7 +14120,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
             /* XXX We are currently very restrictive here, so this code doesn't
              * consider the possibility that, say, /[alpha.]]/ was intended to
              * be a posix class. */
-            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
         }
 
         /* Here we have something like 'foo:]'.  There was no initial colon,
@@ -14286,7 +14290,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
             }
 
             /* Otherwise, it can't have meant to have been a class */
-            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
         }
 
         /* If we ran off the end, and the final character was a punctuation
@@ -14336,7 +14340,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
          * class name.  (We can do this on the first pass, as any second pass
          * will yield an even shorter name) */
         if (name_len < 3) {
-            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
         }
 
         /* Find which class it is.  Initially switch on the length of the name.
@@ -14495,7 +14499,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
             }
 
             /* Here neither pass found a close-enough class name */
-            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
         }
 
     probably_meant_to_be:
@@ -14530,13 +14534,9 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
             }
 
             if (warn_text) {
-                if (posix_warnings) {
-                    /* mortalize to avoid a leak with FATAL warnings */
-                    *posix_warnings = (AV *) sv_2mortal((SV *) warn_text);
-                }
-                else {
-                    SvREFCNT_dec_NN(warn_text);
-                }
+                /* warn_text is only true if posix_warnings is true */
+                assert(posix_warnings);
+                *posix_warnings = (AV *) sv_2mortal((SV *) warn_text);
             }
         }
         else if (class_number != OOB_NAMEDCLASS) {
@@ -14562,6 +14562,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
     return OOB_NAMEDCLASS;
 }
 #undef ADD_POSIX_WARNING
+#undef RETURN_EARLY
 
 STATIC unsigned  int
 S_regex_set_precedence(const U8 my_operator) {
-- 
2.8.1

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @cpansprout

On Wed Jun 08 08​:50​:49 2016, dcollinsn@​gmail.com wrote​:

No idea how to write a test for this.

See t/op/svleak.t

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @dcollinsn

Indeed. This test will work with any proposed fix for this patch, and is confirmed failing on blead before Yves' commit, confirmed passing on blead after Yves' commit, and confirmed passing with my patch 0002.

So I believe that the best course of action, unless khw believes that this is still likely to increase memory usage significantly in some cases, is to revert ee072c8 and apply 0002 and 0003 - unless there's an argument that ee072c8 is better than 0002.patch.

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @dcollinsn

0003-perl-128313-test-for-memory-leak-in-POSIX-classes.patch
From 1e1c9128aaf421762f66498ed90fecc9621976da Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Wed, 8 Jun 2016 16:26:07 -0400
Subject: [PATCH] [perl #128313] test for memory leak in POSIX classes

---
 t/op/svleak.t | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/t/op/svleak.t b/t/op/svleak.t
index 595bf3e..eac20fb 100644
--- a/t/op/svleak.t
+++ b/t/op/svleak.t
@@ -15,7 +15,7 @@ BEGIN {
 
 use Config;
 
-plan tests => 131;
+plan tests => 132;
 
 # run some code N times. If the number of SVs at the end of loop N is
 # greater than (N-1)*delta at the end of loop 1, we've got a leak
@@ -537,3 +537,14 @@ EOF
 
     ::leak(5, 0, \&f, q{goto shouldn't leak @_});
 }
+
+# [perl #128313] POSIX warnings shouldn't leak
+{
+    no warnings 'experimental';
+    use re 'strict';
+    my $a = 'aaa';
+    my $b = 'aa';
+    sub f { $a =~ /[^.]+$b/; }
+    ::leak(2, 0, \&f, q{use re 'strict' shouldn't leak warning strings});
+}
-- 
2.8.1

@p5pRT
Copy link
Author

p5pRT commented Jun 8, 2016

From @khwilliamson

On 06/08/2016 02​:38 PM, Dan Collins via RT wrote​:

Indeed. This test will work with any proposed fix for this patch, and is confirmed failing on blead before Yves' commit, confirmed passing on blead after Yves' commit, and confirmed passing with my patch 0002.

So I believe that the best course of action, unless khw believes that this is still likely to increase memory usage significantly in some cases, is to revert ee072c8 and apply 0002 and 0003 - unless there's an argument that ee072c8 is better than 0002.patch.

Notice that my email just said 'concerned'. I'm not sure the memory
issue is a real problem. But, since the patch is already written, and
solves the problem once and for all, I think your action proposal is
correct, but lets wait to hear from Yves.

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=128313

@p5pRT
Copy link
Author

p5pRT commented Jun 9, 2016

From @demerphq

On 8 Jun 2016 20​:38, "Dan Collins via RT" <perlbug-followup@​perl.org> wrote​:

Yves' patch changes warn_text from leaking to mortal if posix_warnings is
set and the function ends early, no change in any other case. In other
words, it /never/ called SvREFCNT_dec_NN(warn_text), either before or now.

Alternately, since posix_warnings isn't set if the function returns
early, we can SvREFCNT_dec(warn_text) if we return early. My limited
testing suggests that the memory advantage is minimal, but it's probably
"more right" to free it immediately before returning. Is the attached patch
what you were thinking? It still fixes this bug, and all tests still pass.

(Patch is against blead /before/ Yves' patch was applied, if you'd prefer
a patch against blead, that can be arranged.

Wait, wait. :-)

Seems to me that if this is a real concern, and i trust karls judgement
there although i admit to being surprised, then this av should just become
part of the RExC struct and it should be reused each time.

Cheers
Yves

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=128313

From b04aa5cec7c85fa03e65b7dd86394d818a3233eb Mon Sep 17 00​:00​:00 2001
From​: Dan Collins <dcollinsn@​gmail.com>
Date​: Wed, 8 Jun 2016 14​:26​:05 -0400
Subject​: [PATCH] [perl #128313] Fix memory leak in POSIX class warnings

Certain classes that "may be" POSIX classes result in POSIX warnings
being added to warn_text, but never freed, resulting in a slow but
present memory leak. We need to ensure that warn_text is freed.

warn_text is presently mortalized late in the function, when it is
assigned to *posix_warnings. However, certain cases can generate
a POSIX warning while also having the function return before that
point. If a POSIX warning is generated and the function returns
before warn_text can be made mortal, it will never be freed.

This patch performs a REFCNT_dec on warn_text immediately before
any early return.
---
regcomp.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 0b1e606..6bd903d 100644
--- a/regcomp.c
+++ b/regcomp.c
@​@​ -13772,6 +13772,10 @​@​ S_populate_ANYOF_from_invlist(pTHX_ regnode
*node, SV** invlist_ptr)
REPORT_LOCATION_ARGS(p)));
  \
}
  \
} STMT_END
+#define RETURN_EARLY(retval) STMT_START {
  \
+ if (warn_text) SvREFCNT_dec_NN(warn_text);
  \
+ return (retval);
  \
+ } STMT_END

STATIC int
S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
@​@​ -13913,7 +13917,7 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
PERL_ARGS_ASSERT_HANDLE_POSSIBLE_POSIX;

 if \(p >= e\) \{

- return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+ RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
}

 if \(\*\(p \- 1\) \!= '\['\) \{

@​@​ -14002,7 +14006,7 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
*updated_parse_ptr = (char *) temp_ptr;
}

- return OOB_NAMEDCLASS;
+ RETURN_EARLY(OOB_NAMEDCLASS);
}
}

@​@​ -14072,7 +14076,7 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
/* We consider something like [^​:^alnum​:]] to not have been
intended to
* be a posix class, but XXX maybe we should */
if (complement) {
- return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+ RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
}

     complement = 1;

@​@​ -14099,7 +14103,7 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
* this leaves this construct looking like [​:] or [​:^], which
almost
* certainly weren't intended to be posix classes */
if (has_opening_bracket) {
- return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+ RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
}

     /\* But this function can be called when we parse the colon for

@​@​ -14116,7 +14120,7 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
/* XXX We are currently very restrictive here, so this code
doesn't
* consider the possibility that, say, /[alpha.]]/ was
intended to
* be a posix class. */
- return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+ RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
}

     /\* Here we have something like 'foo&#8203;:\]'\.  There was no initial

colon,

@​@​ -14286,7 +14290,7 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
}

         /\* Otherwise\, it can't have meant to have been a class \*/

- return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+ RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
}

     /\* If we ran off the end\, and the final character was a

punctuation

@​@​ -14336,7 +14340,7 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
* class name. (We can do this on the first pass, as any second
pass
* will yield an even shorter name) */
if (name_len < 3) {
- return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+ RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
}

     /\* Find which class it is\.  Initially switch on the length of

the name.

@​@​ -14495,7 +14499,7 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
}

         /\* Here neither pass found a close\-enough class name \*/

- return NOT_MEANT_TO_BE_A_POSIX_CLASS;
+ RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
}

 probably\_meant\_to\_be&#8203;:

@​@​ -14530,13 +14534,9 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
}

         if \(warn\_text\) \{

- if (posix_warnings) {
- /* mortalize to avoid a leak with FATAL warnings */
- *posix_warnings = (AV *) sv_2mortal((SV *)
warn_text);
- }
- else {
- SvREFCNT_dec_NN(warn_text);
- }
+ /* warn_text is only true if posix_warnings is true */
+ assert(posix_warnings);
+ *posix_warnings = (AV *) sv_2mortal((SV *) warn_text);
}
}
else if (class_number != OOB_NAMEDCLASS) {
@​@​ -14562,6 +14562,7 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
return OOB_NAMEDCLASS;
}
#undef ADD_POSIX_WARNING
+#undef RETURN_EARLY

STATIC unsigned int
S_regex_set_precedence(const U8 my_operator) {
--
2.8.1

@p5pRT
Copy link
Author

p5pRT commented Jun 9, 2016

From @demerphq

On 9 Jun 2016 00​:06, "Karl Williamson" <public@​khwilliamson.com> wrote​:

On 06/08/2016 02​:38 PM, Dan Collins via RT wrote​:

Indeed. This test will work with any proposed fix for this patch, and is
confirmed failing on blead before Yves' commit, confirmed passing on blead
after Yves' commit, and confirmed passing with my patch 0002.

So I believe that the best course of action, unless khw believes that
this is still likely to increase memory usage significantly in some cases,
is to revert ee072c8 and apply 0002 and
0003 - unless there's an argument that ee072c8 is better than 0002.patch.

Notice that my email just said 'concerned'. I'm not sure the memory
issue is a real problem. But, since the patch is already written, and
solves the problem once and for all, I think your action proposal is
correct, but lets wait to hear from Yves.

I think a better solution is to put warn_text in RExC struct and reuse the
same av over and over. I will put togethdr a patch for review.

Yves

@p5pRT
Copy link
Author

p5pRT commented Jun 10, 2016

From @demerphq

Pushed as 7eec73e

Cheers,
Yves

On 9 June 2016 at 10​:21, demerphq <demerphq@​gmail.com> wrote​:

On 9 Jun 2016 00​:06, "Karl Williamson" <public@​khwilliamson.com> wrote​:

On 06/08/2016 02​:38 PM, Dan Collins via RT wrote​:

Indeed. This test will work with any proposed fix for this patch, and is
confirmed failing on blead before Yves' commit, confirmed passing on blead
after Yves' commit, and confirmed passing with my patch 0002.

So I believe that the best course of action, unless khw believes that
this is still likely to increase memory usage significantly in some cases,
is to revert ee072c8 and apply 0002 and
0003 - unless there's an argument that ee072c8 is better than 0002.patch.

Notice that my email just said 'concerned'. I'm not sure the memory issue
is a real problem. But, since the patch is already written, and solves the
problem once and for all, I think your action proposal is correct, but lets
wait to hear from Yves.

I think a better solution is to put warn_text in RExC struct and reuse the
same av over and over. I will put togethdr a patch for review.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Jun 10, 2016

From @demerphq

On 10 June 2016 at 13​:32, demerphq <demerphq@​gmail.com> wrote​:

Pushed as 7eec73e

And amended by 0bf54b1

Yves

@p5pRT
Copy link
Author

p5pRT commented Jun 13, 2016

From @dcollinsn

Can you please also consider and apply the test for this bug, which is (I
believe) attachment 3 to this ticket, before closing this?

On Fri, Jun 10, 2016 at 7​:35 AM, demerphq <demerphq@​gmail.com> wrote​:

On 10 June 2016 at 13​:32, demerphq <demerphq@​gmail.com> wrote​:

Pushed as 7eec73e

And amended by 0bf54b1

Yves

@p5pRT
Copy link
Author

p5pRT commented Jun 13, 2016

From @cpansprout

On Mon Jun 13 05​:19​:21 2016, dcollinsn@​gmail.com wrote​:

Can you please also consider and apply the test for this bug, which is (I
believe) attachment 3 to this ticket, before closing this?

Applied as 222c4b0. Thank you.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jun 13, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Jul 20, 2016

From sc@charnell.plus.com

Created by sc@charnell.plus.com

This is a bug report for perl from sc@​charnell.plus.com,
generated with the help of perlbug 1.40 running under perl 5.24.0.

-----------------------------------------------------------------
A perl script I use to parse the XML produced from a Current cost energy meter looks to have a memory leak
after I updated from perl 5.22.0 to perl 5.24.0.

The script runs continuously and when running with perl 5.24.0 its memory footprint increases markedly over time.
The memory footprint did not increase with perl 5.22.0.
I have tried the script on armv5tel & x86_64 platforms running archlinux and both platforms exhibit the issue.

I think I have narrowed the issue down to a string match operation and have developed a test-case,
the limit used in the 'for' loop in the main subroutine can be increased to allow the script to run for long
enough for the issue to be apparent.

The perl program from which the test-case was taken was written a long while ago, so my perl may not be the best.

The test-case follows​:

#!/usr/bin/perl -w

use strict;
use warnings;
use POSIX;

my $sensors = 8; # 9 sensors, 0..8

my $line;

my @​test_data;
my $test_data = ("<msg><src>CC128-v1.48</src><dsb>00860</dsb><time>11​:28​:22</time><tmpr>19.0</tmpr><sensor>2</sensor><id>01210</id><type>1</type><ch1><watts>00101</watts></ch1></msg>
<msg><src>CC128-v1.48</src><dsb>00898</dsb><time>17​:16​:30</time><hist><dsw>00900</dsw><type>1</type><units>kwhr</units><data><sensor>0</sensor><h006>0.000</h006><h004>0.046</h004></data></hist></msg>
");

sub simple_sub
{
  my ($line) = @​_;
  my $sensor;
  my $sep = "h";
  my $found = 0;

  for ($sensor = 0; $sensor <= $sensors; $sensor++) {
  # 2 values per sensor
  if ($line =~ m!<time>(\d+)​:(\d+)​:(\d+)</time><hist>.*<data><sensor>$sensor</sensor><$sep(\d+)>([\-\d.]+)</$sep(\d+)><$sep(\d+)>([\-\d.]+)</$sep(\d+)></data>!) {
  $found = 1;
  }
  }

  return;
}

sub main
{

  my $line;
  my $loop;
 
  @​test_data = split /\n/, $test_data;
 
  foreach $line (@​test_data) {
  for ($loop = 0; $loop < 100000; $loop++) {
  simple_sub($line);
  }
  }

  return;
}

main();
exit

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.24.0:

Configured by builduser at Sun Jun  5 09:49:52 MDT 2016.

Summary of my perl5 (revision 5 version 24 subversion 0) configuration:
   
  Platform:
    osname=linux, osvers=3.10.96-3-arch, archname=armv5tel-linux-thread-multi
    uname='linux leming 3.10.96-3-arch #1 smp preempt thu mar 24 19:24:55 mdt 2016 armv5tel gnulinux '
    config_args='-des -Dusethreads -Duseshrplib -Doptimize=-march=armv5te -O2 -pipe -fstack-protector --param=ssp-buffer-size=4 -Dprefix=/usr -Dvendorprefix=/usr -Dprivlib=/usr/share/perl5/core_perl -Darchlib=/usr/lib/perl5/core_perl -Dsitelib=/usr/share/perl5/site_perl -Dsitearch=/usr/lib/perl5/site_perl -Dvendorlib=/usr/share/perl5/vendor_perl -Dvendorarch=/usr/lib/perl5/vendor_perl -Dscriptdir=/usr/bin/core_perl -Dsitescript=/usr/bin/site_perl -Dvendorscript=/usr/bin/vendor_perl -Dinc_version_list=none -Dman1ext=1perl -Dman3ext=3perl -Dlddlflags=-shared -Wl,-O1,--sort-common,--as-needed,-z,relro -Dldflags=-Wl,-O1,--sort-common,--as-needed,-z,relro'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    use64bitint=undef, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-march=armv5te -O2 -pipe -fstack-protector --param=ssp-buffer-size=4',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion='', gccversion='6.1.1 20160501', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234, doublekind=3
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=8, longdblkind=0
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags ='-Wl,-O1,--sort-common,--as-needed,-z,relro -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib/gcc/armv5tel-unknown-linux-gnueabi/6.1.1/include-fixed /usr/lib /lib
    libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.23.so, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.23'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/lib/perl5/core_perl/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -Wl,-O1,--sort-common,--as-needed,-z,relro -L/usr/local/lib -fstack-protector-strong'



@INC for perl 5.24.0:
    /usr/lib/perl5/site_perl
    /usr/share/perl5/site_perl
    /usr/lib/perl5/vendor_perl
    /usr/share/perl5/vendor_perl
    /usr/lib/perl5/core_perl
    /usr/share/perl5/core_perl
    .


Environment for perl 5.24.0:
    HOME=/root
    LANG=en_GB.utf8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Jul 20, 2016

From @cpansprout

On Wed Jul 20 07​:37​:14 2016, sc@​charnell.plus.com wrote​:

A perl script I use to parse the XML produced from a Current cost
energy meter looks to have a memory leak
after I updated from perl 5.22.0 to perl 5.24.0.

The script runs continuously and when running with perl 5.24.0 its
memory footprint increases markedly over time.
The memory footprint did not increase with perl 5.22.0.

Confirmed. No leak in 5.22 or current bleadperl. 5.24.0 and 5.24.1-RC1 both leak.

The two 5.25 dev releases I have installed are​:

v5.25.1-75-gcbef69c
v5.25.2-196-g620f73f

The first leaks. The second does not. So it was fixed somewhere between them.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 20, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Jul 22, 2016

From sc@charnell.plus.com

On Wed Jul 20 13​:10​:46 2016, sprout wrote​:

On Wed Jul 20 07​:37​:14 2016, sc@​charnell.plus.com wrote​:

A perl script I use to parse the XML produced from a Current cost
energy meter looks to have a memory leak
after I updated from perl 5.22.0 to perl 5.24.0.

The script runs continuously and when running with perl 5.24.0 its
memory footprint increases markedly over time.
The memory footprint did not increase with perl 5.22.0.

Confirmed. No leak in 5.22 or current bleadperl. 5.24.0 and 5.24.1-
RC1 both leak.

The two 5.25 dev releases I have installed are​:

v5.25.1-75-gcbef69c
v5.25.2-196-g620f73f

The first leaks. The second does not. So it was fixed somewhere
between them.

Thanks.

--

Stewart Charnell

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2016

From @khwilliamson

By eyeballing the memory usage, I have determined that this bug was fixed by the three commits​:
ee072c8
7eec73e
0bf54b1

and that this ticket is the same as #128313 (which I'm merging it into). That ticket only showed a leak with 'use re "strict"'. This shows it can happen outside that.

Since this is a regression, this is a candidate for fixing in a 5.24 maintenance release. I have created a patch which has those 3 commits merged into one (the 2nd and 3rd were modifications of the first) and attached it to this message, and am adding it to the blocker and maint votes lists

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2016

From @khwilliamson

0001-perl-128313-Fix-leak-in-perl-5.24-with-strict-and-re.patch
From 961c05f10d8a0c851d14092322f3b0afc372b7e0 Mon Sep 17 00:00:00 2001
From: Yves Orton <demerphq@gmail.com>
Date: Wed, 8 Jun 2016 18:42:30 +0200
Subject: [PATCH] [perl #128313] Fix leak in perl 5.24 with strict and regex
 posix char classes

This patch is a refinement of one written by Dan Collins.

Any thanks for this patch should go to him.
---
 regcomp.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 0b1e606..86173db 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -199,6 +199,7 @@ struct RExC_state_t {
     scan_frame *frame_head;
     scan_frame *frame_last;
     U32         frame_count;
+    AV         *warn_text;
 #ifdef ADD_TO_REGEXEC
     char 	*starttry;		/* -Dr: where regtry was called. */
 #define RExC_starttry	(pRExC_state->starttry)
@@ -290,6 +291,7 @@ struct RExC_state_t {
 #define RExC_frame_count (pRExC_state->frame_count)
 #define RExC_strict (pRExC_state->strict)
 #define RExC_study_started      (pRExC_state->study_started)
+#define RExC_warn_text (pRExC_state->warn_text)
 
 /* Heuristic check on the complexity of the pattern: if TOO_NAUGHTY, we set
  * a flag to disable back-off on the fixed/floating substrings - if it's
@@ -6764,6 +6766,7 @@ Perl_re_op_compile(pTHX_ SV ** const patternp, int pat_count,
 #endif
     }
 
+    pRExC_state->warn_text = NULL;
     pRExC_state->code_blocks = NULL;
     pRExC_state->num_code_blocks = 0;
 
@@ -13764,8 +13767,8 @@ S_populate_ANYOF_from_invlist(pTHX_ regnode *node, SV** invlist_ptr)
  * routine. q.v. */
 #define ADD_POSIX_WARNING(p, text)  STMT_START {                            \
         if (posix_warnings) {                                               \
-            if (! warn_text) warn_text = newAV();                           \
-            av_push(warn_text, Perl_newSVpvf(aTHX_                          \
+            if (! RExC_warn_text ) RExC_warn_text = (AV *) sv_2mortal((SV *) newAV()); \
+            av_push(RExC_warn_text, Perl_newSVpvf(aTHX_                          \
                                              WARNING_PREFIX                 \
                                              text                           \
                                              REPORT_LOCATION,               \
@@ -13896,7 +13899,6 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
     bool has_opening_colon    = FALSE;
     int class_number          = OOB_NAMEDCLASS; /* Out-of-bounds until find
                                                    valid class */
-    AV* warn_text             = NULL;   /* any warning messages */
     const char * possible_end = NULL;   /* used for a 2nd parse pass */
     const char* name_start;             /* ptr to class name first char */
 
@@ -13912,6 +13914,9 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
 
     PERL_ARGS_ASSERT_HANDLE_POSSIBLE_POSIX;
 
+    if (posix_warnings && RExC_warn_text)
+        av_clear(RExC_warn_text);
+
     if (p >= e) {
         return NOT_MEANT_TO_BE_A_POSIX_CLASS;
     }
@@ -14529,14 +14534,8 @@ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
                 ADD_POSIX_WARNING(p, "there is no terminating ']'");
             }
 
-            if (warn_text) {
-                if (posix_warnings) {
-                    /* mortalize to avoid a leak with FATAL warnings */
-                    *posix_warnings = (AV *) sv_2mortal((SV *) warn_text);
-                }
-                else {
-                    SvREFCNT_dec_NN(warn_text);
-                }
+            if (posix_warnings && RExC_warn_text && av_top_index(RExC_warn_text) > -1) {
+                *posix_warnings = RExC_warn_text;
             }
         }
         else if (class_number != OOB_NAMEDCLASS) {
-- 
2.5.0

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2016

From @demerphq

I pushed the same patch as

commit dfd347f
(origin/yves/backport_128313, yves/backport_128313)
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Jun 8 18​:42​:30 2016 +0200

  [perl #128313] Fix leak in perl 5.24 with strict and regex posix
char classes

  move warning text to RExC_state (via RExC_warn_text)

  This way we reuse the same AV each time, and avoid various
refcount bookkeeping issues, all at a relatively modest cost (IMO)

  This patch is the result of detective work and inital patches by Dan Collins
  with additional coding to take advantage of the regexp engine internals by
  Yves Orton after feedback from Karl Williams.

  It is a squash of the following commits in blead​:

  222c4b0
  [perl #128313] test for memory leak in POSIX classes

  0bf54b1
  fixup, guard av_top_index() for null RExC_warn_text

  7eec73e
  move warning text to RExC_state (via RExC_warn_text)

  ee072c8
  [perl #128313] Fix leak in perl 5.24 with strict and regex
posix char classes

I mention this only because I took some time and care to write a more
detailed commit message. :-)

Yves

On 6 August 2016 at 21​:01, Karl Williamson via RT
<perlbug-followup@​perl.org> wrote​:

By eyeballing the memory usage, I have determined that this bug was fixed by the three commits​:
ee072c8
7eec73e
0bf54b1

and that this ticket is the same as #128313 (which I'm merging it into). That ticket only showed a leak with 'use re "strict"'. This shows it can happen outside that.

Since this is a regression, this is a candidate for fixing in a 5.24 maintenance release. I have created a patch which has those 3 commits merged into one (the 2nd and 3rd were modifications of the first) and attached it to this message, and am adding it to the blocker and maint votes lists

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=128680

From 961c05f10d8a0c851d14092322f3b0afc372b7e0 Mon Sep 17 00​:00​:00 2001
From​: Yves Orton <demerphq@​gmail.com>
Date​: Wed, 8 Jun 2016 18​:42​:30 +0200
Subject​: [PATCH] [perl #128313] Fix leak in perl 5.24 with strict and regex
posix char classes

This patch is a refinement of one written by Dan Collins.

Any thanks for this patch should go to him.
---
regcomp.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 0b1e606..86173db 100644
--- a/regcomp.c
+++ b/regcomp.c
@​@​ -199,6 +199,7 @​@​ struct RExC_state_t {
scan_frame *frame_head;
scan_frame *frame_last;
U32 frame_count;
+ AV *warn_text;
#ifdef ADD_TO_REGEXEC
char *starttry; /* -Dr​: where regtry was called. */
#define RExC_starttry (pRExC_state->starttry)
@​@​ -290,6 +291,7 @​@​ struct RExC_state_t {
#define RExC_frame_count (pRExC_state->frame_count)
#define RExC_strict (pRExC_state->strict)
#define RExC_study_started (pRExC_state->study_started)
+#define RExC_warn_text (pRExC_state->warn_text)

/* Heuristic check on the complexity of the pattern​: if TOO_NAUGHTY, we set
* a flag to disable back-off on the fixed/floating substrings - if it's
@​@​ -6764,6 +6766,7 @​@​ Perl_re_op_compile(pTHX_ SV ** const patternp, int pat_count,
#endif
}

+ pRExC_state->warn_text = NULL;
pRExC_state->code_blocks = NULL;
pRExC_state->num_code_blocks = 0;

@​@​ -13764,8 +13767,8 @​@​ S_populate_ANYOF_from_invlist(pTHX_ regnode *node, SV** invlist_ptr)
* routine. q.v. */
#define ADD_POSIX_WARNING(p, text) STMT_START { \
if (posix_warnings) { \
- if (! warn_text) warn_text = newAV(); \
- av_push(warn_text, Perl_newSVpvf(aTHX_ \
+ if (! RExC_warn_text ) RExC_warn_text = (AV *) sv_2mortal((SV *) newAV()); \
+ av_push(RExC_warn_text, Perl_newSVpvf(aTHX_ \
WARNING_PREFIX \
text \
REPORT_LOCATION, \
@​@​ -13896,7 +13899,6 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
bool has_opening_colon = FALSE;
int class_number = OOB_NAMEDCLASS; /* Out-of-bounds until find
valid class */
- AV* warn_text = NULL; /* any warning messages */
const char * possible_end = NULL; /* used for a 2nd parse pass */
const char* name_start; /* ptr to class name first char */

@​@​ -13912,6 +13914,9 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,

 PERL\_ARGS\_ASSERT\_HANDLE\_POSSIBLE\_POSIX;

+ if (posix_warnings && RExC_warn_text)
+ av_clear(RExC_warn_text);
+
if (p >= e) {
return NOT_MEANT_TO_BE_A_POSIX_CLASS;
}
@​@​ -14529,14 +14534,8 @​@​ S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
ADD_POSIX_WARNING(p, "there is no terminating ']'");
}

- if (warn_text) {
- if (posix_warnings) {
- /* mortalize to avoid a leak with FATAL warnings */
- *posix_warnings = (AV *) sv_2mortal((SV *) warn_text);
- }
- else {
- SvREFCNT_dec_NN(warn_text);
- }
+ if (posix_warnings && RExC_warn_text && av_top_index(RExC_warn_text) > -1) {
+ *posix_warnings = RExC_warn_text;
}
}
else if (class_number != OOB_NAMEDCLASS) {
--
2.5.0

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2016

From @cpansprout

On Mon Aug 08 06​:30​:43 2016, demerphq wrote​:

I pushed the same patch as

commit dfd347f
(origin/yves/backport_128313, yves/backport_128313)
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Jun 8 18​:42​:30 2016 +0200

[perl #128313] Fix leak in perl 5.24 with strict and regex posix
char classes

Does that mean we have three votes now?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2016

From @demerphq

On 8 August 2016 at 15​:37, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Mon Aug 08 06​:30​:43 2016, demerphq wrote​:

I pushed the same patch as

commit dfd347f
(origin/yves/backport_128313, yves/backport_128313)
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Wed Jun 8 18​:42​:30 2016 +0200

[perl #128313] Fix leak in perl 5.24 with strict and regex posix
char classes

Does that mean we have three votes now?

If I wasnt part of the original vote count then yes.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2016

From @cpansprout

On Mon Aug 08 07​:01​:23 2016, demerphq wrote​:

On 8 August 2016 at 15​:37, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

Does that mean we have three votes now?

If I wasnt part of the original vote count then yes.

Are you aware of the maint-votes branch? I have added your vote to the votes-5.24.xml file.

--

Father Chrysostomos

@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