Memory leak in perl 5.24.0 when use re qw[strict] is used #15382
Comments
From zdm@softvisio.netCreated by zdm@softvisio.netReply-To: zdm@softvisio.net This is a bug report for perl from zdm@softvisio.net, ----------------------------------------------------------------- Hi. This code produce memory leak: use re qw[strict]; If I remove "use re qw[strict];" - mem. leak disappearing. Perl Info |
From @dcollinsnConfirmed 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 I was able to minimize the test case to: use re qw[strict]; 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) { 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. |
From @dcollinsn0001-perl-128313-Fix-memory-leak-in-POSIX-class-warnings.patchFrom 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
|
|
The RT System itself - Status changed from 'new' to 'open' |
From @demerphqOn 8 June 2016 at 17:50, Dan Collins via RT <perlbug-followup@perl.org> wrote:
Hrm. Did the patch you tried look like this? Inline Patchdiff --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
if (warn_text) { Yves |
From @dcollinsnVaguely, yes...but I realize that I may not have set *posix_warnings =
|
From @demerphqOn 8 June 2016 at 18:27, Dan Collins <dcollinsn@gmail.com> wrote:
Yes it passed all tests here. I pushed it just now: commit ee072c8 [perl #128313] Fix leak in perl 5.24 with strict and regex posix This patch is a refinement of one written by Dan Collins. Any thanks for this patch should go to him. Yves |
From @khwilliamsonOn 06/08/2016 10:44 AM, demerphq wrote:
I'm concerned that you guys are falling into the same trap I did If it isn't ok, one option is to add this mortalization only if it's re |
From @dcollinsnYves' 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. |
From @dcollinsn0002-perl-128313-Fix-memory-leak-in-POSIX-class-warnings.patchFrom 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
|
From @cpansproutOn Wed Jun 08 08:50:49 2016, dcollinsn@gmail.com wrote:
See t/op/svleak.t -- Father Chrysostomos |
From @dcollinsnIndeed. 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. |
From @dcollinsn0003-perl-128313-test-for-memory-leak-in-POSIX-classes.patchFrom 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
|
From @khwilliamsonOn 06/08/2016 02:38 PM, Dan Collins via RT wrote:
Notice that my email just said 'concerned'. I'm not sure the memory
|
From @demerphqOn 8 Jun 2016 20:38, "Dan Collins via RT" <perlbug-followup@perl.org> wrote:
Wait, wait. :-) Seems to me that if this is a real concern, and i trust karls judgement Cheers
colon,
punctuation
the name.
|
From @demerphqOn 9 Jun 2016 00:06, "Karl Williamson" <public@khwilliamson.com> wrote:
I think a better solution is to put warn_text in RExC struct and reuse the Yves |
From @demerphqPushed as 7eec73e Cheers, On 9 June 2016 at 10:21, demerphq <demerphq@gmail.com> wrote:
-- |
From @dcollinsnCan you please also consider and apply the test for this bug, which is (I On Fri, Jun 10, 2016 at 7:35 AM, demerphq <demerphq@gmail.com> wrote:
|
From @cpansproutOn Mon Jun 13 05:19:21 2016, dcollinsn@gmail.com wrote:
Applied as 222c4b0. Thank you. -- Father Chrysostomos |
|
@cpansprout - Status changed from 'open' to 'pending release' |
From sc@charnell.plus.comCreated by sc@charnell.plus.comThis is a bug report for perl from sc@charnell.plus.com, ----------------------------------------------------------------- The script runs continuously and when running with perl 5.24.0 its memory footprint increases markedly over time. I think I have narrowed the issue down to a string match operation and have developed a test-case, 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; my $sensors = 8; # 9 sensors, 0..8 my $line; my @test_data; sub simple_sub for ($sensor = 0; $sensor <= $sensors; $sensor++) { return; sub main my $line; return; main(); Perl Info |
From @cpansproutOn Wed Jul 20 07:37:14 2016, sc@charnell.plus.com wrote:
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 The first leaks. The second does not. So it was fixed somewhere between them. -- Father Chrysostomos |
|
The RT System itself - Status changed from 'new' to 'open' |
From sc@charnell.plus.comOn Wed Jul 20 13:10:46 2016, sprout wrote:
Thanks. -- Stewart Charnell |
From @khwilliamsonBy eyeballing the memory usage, I have determined that this bug was fixed by the three commits: 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 |
From @khwilliamson0001-perl-128313-Fix-leak-in-perl-5.24-with-strict-and-re.patchFrom 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
|
From @demerphqI pushed the same patch as commit dfd347f [perl #128313] Fix leak in perl 5.24 with strict and regex posix move warning text to RExC_state (via RExC_warn_text) This way we reuse the same AV each time, and avoid various This patch is the result of detective work and inital patches by Dan Collins It is a squash of the following commits in blead: 222c4b0 0bf54b1 7eec73e ee072c8 I mention this only because I took some time and care to write a more Yves On 6 August 2016 at 21:01, Karl Williamson via RT
-- |
From @cpansproutOn Mon Aug 08 06:30:43 2016, demerphq wrote:
Does that mean we have three votes now? -- Father Chrysostomos |
From @demerphqOn 8 August 2016 at 15:37, Father Chrysostomos via RT
If I wasnt part of the original vote count then yes. Yves -- |
From @cpansproutOn Mon Aug 08 07:01:23 2016, demerphq wrote:
Are you aware of the maint-votes branch? I have added your vote to the votes-5.24.xml file. -- Father Chrysostomos |
From @khwilliamsonThank 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 Perl 5.26.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#128313 (status was 'resolved')
Searchable as RT128313$
The text was updated successfully, but these errors were encountered: