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
Changes in regex recursion in 5.24 #15394
Comments
From jg@jgsoft.comHi. While preparing RegexBuddy to support Perl 5.24 I discovered some Here are some sample regexes along with the subject strings and the re: /aa$|a(?R)a|a/ re: /(?:\1|a)([bcd])\1(?:(?R)|e)\1/ re: /(.)\W*+(?R)\W*+\1|\W*+.\W*+/i I also found that infinite recursion is no longer an error but is The perlbug page in the docs tells me you want the output of perl -V so I've attached a sample program that demonstrates the matches. X marks 5.022002 Perl 5.24 finds partial matches: 5.024000 Please let me know whether the Perl developers consider this to be a Kind regards, -- |
From jg@jgsoft.comSummary of my perl5 (revision 5 version 24 subversion 0) configuration: Characteristics of this binary (from libperl): |
From jg@jgsoft.comSummary of my perl5 (revision 5 version 22 subversion 2) configuration: Characteristics of this binary (from libperl): |
From jg@jgsoft.com |
From @dcollinsnA few bisects later... This was "broken" in: d5a00e4 is the first bad commit Unify GOSTART and GOSUB GOSTART is a special case of GOSUB, we can remove a lot of offset twiddling, GOSUB has 2 arguments, ARG() and ARG2L(), which are interpreted as Prior to this patch the argument to GOSUB would always be >=, and unlike :040000 040000 98c201349ff073a1e14744c0471ef3d9ab5fb584 a2df6849db8b2292fe1340f916f941c0e3c0f80f M pod And it was "fixed" in: da7cf1c is the first bad commit fix #128109 - do not move RExC_open_parens[0] in reginsert In d5a00e4 I merged GOSUB and GOSTART, This tripped up in reginsert in a subtle way, the start of the pattern This patches fixes this up, and adds an assert to check that reginsert() This was noticed by hv while debugging [perl #128085], thanks hugo! :100644 100644 c6823b51e82c55b39104ebb58d52a1a86d540a9e e6b352b2b796001d8501bf373b3fdf46956c9799 M regcomp.c This should perhaps be included in 5.24.1? |
The RT System itself - Status changed from 'new' to 'open' |
From @dcollinsnPatch adds two of the requestor's testcases that I used for the bisect to re_tests for regression testing. Confirmed passing on blead, failing on 5.24.0. |
From @dcollinsn0001-perl-128420-Add-tests-for-regex-recursion.patchFrom 6965891ac125072bbd890010ea752a27b34b585f Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Fri, 17 Jun 2016 19:40:57 -0400
Subject: [PATCH] [perl #128420] Add tests for regex recursion
d5a00e4af introduced a bug in reginsert that was fixed by da7cf1cc7,
originally documented in [perl #128109]. This patch adds two
regression tests for the testcase reported by Jan Goyvaerts in
[perl #128420].
---
t/re/re_tests | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/re/re_tests b/t/re/re_tests
index 34ac94a..7e8522d 100644
--- a/t/re/re_tests
+++ b/t/re/re_tests
@@ -1966,6 +1966,8 @@ ab(?#Comment){2}c abbc y $& abbc
.{1}?? - c - Nested quantifiers
.{1}?+ - c - Nested quantifiers
(?:.||)(?|)000000000@ 000000000@ y $& 000000000@ # [perl #126405]
+aa$|a(?R)a|a aaa y $& aaa # [perl 128420] recursive matches
+(?:\1|a)([bcd])\1(?:(?R)|e)\1 abbaccaddedcb y $& abbaccaddedcb # [perl 128420] recursive match with backreferences
# Keep these lines at the end of the file
# vim: softtabstop=0 noexpandtab
--
2.8.1
|
From [Unknown Contact. See original ticket]Patch adds two of the requestor's testcases that I used for the bisect to re_tests for regression testing. Confirmed passing on blead, failing on 5.24.0. |
From @khwilliamsonThanks, applied as ec5bd22 I'm marking this ticket as a 5.24.1 blocker |
@khwilliamson - Status changed from 'open' to 'pending release' |
From @demerphqOn 17 June 2016 at 03:25, Jan Goyvaerts <perlbug-followup@perl.org> wrote:
Could very likely be. We tried to detect infinite recursion better
I will review this. If at all possible please test out blead as well. There was some strangeness about the 5.24 release, and its possible a cheers, |
From @demerphqOn 19 June 2016 at 08:48, demerphq <demerphq@gmail.com> wrote:
Yeah, this is a regression in 5.24 that should be fixed in 5.24.1 Like one way or another this is my fault. My apologies for any inconvenience. Yves -- |
From jg@jgsoft.comyves orton via RT wrote:
I get the exact same behavior in 5.24.1 as in 5.24.0. Is this still Kind regards, -- |
@jkeenan - Status changed from 'pending release' to 'open' |
From @xsawyerxAt this point, I think 5.26.1 and 5.24.3 might be better for this. On 04/10/2017 02:27 AM, Jan Goyvaerts wrote:
|
From @demerphqOn 11 April 2017 at 12:09, Sawyer X <xsawyerx@gmail.com> wrote:
You want me to backport the patch? Yves -- |
From jg@jgsoft.comdemerphq wrote:
You can do whatever you think is best for the Perl community. I just I'm the developer of RegexBuddy which emulates the regex features in Another change I've noticed in Perl 5.24.0 and 5.24.1 compared with Kind regards, -- |
From @demerphqOn 11 April 2017 at 12:36, Jan Goyvaerts <jg@jgsoft.com> wrote:
That was aimed at Sawyer X. :-) I will put together a patch for this.
I was under the impression that the patch was already applied to the
If you notice things like that could file a new ticket please? I am cheers, -- |
From @khwilliamsonOn 04/11/2017 05:02 AM, demerphq wrote:
These are now legal. The first one warns if warnings are enabled, as Assuming NOT a POSIX class since the name must be all lowercase letters The single letter constructs, which you assume are meant to be Posix If you want to see what a pattern compiles into, and hence what it matches, use re qw(Debug COMPILE); That shows that [[:w:]] matches a string that contains a ']' immediately It's impossible to figure out for sure what was the programmer's intent. |
From jg@jgsoft.comKarl Williamson wrote:
Dinkumware's implementation of std::regex (and possibly other Boost::regex additionally supports [:v:] and [:h:] to match vertical and
Thanks for the tip.
If the programmer intended to add a literal : to the character class Anyway, Perl 5.24's behavior is fine. It's just that the change took me Kind regards, -- |
From @demerphqOn 12 April 2017 at 08:02, Jan Goyvaerts <jg@jgsoft.com> wrote:
Do you know why? Do they not support \w \s and \d?
I dont think we handle POSIX charclasses very cleanly. We warn about 1. I think in the next release we should produce a deprecation warning 2. Then in a release or so afterwards we could make them errors. 3. Then in a release or so after that we could add support for these IMO, even if we dont do step 3 we should step 1 and 2. Yves -- |
From jg@jgsoft.comdemerphq wrote:
std::regex supports \w, \s, and \d when using its "ECMAScript" grammar boost::regex supports \w, \s, \d, and \h in all grammars. It does not
Sounds like a good plan to me. Anyway, RegexBuddy will emulate whatever Another inconsistency you have right now is that Perl 5.24 does throw Kind regards, -- |
From jg@jgsoft.comHi. Running this under Strawberry Perl 5.24.1 if ("aqzaqzaqz" =~ /(a(?R)z|q)*/) { causes the Perl interpreter to crash. In Perl 5.22 it correctly matches the whole subject string. Kind regards, -- |
From zefram@fysh.orgJan Goyvaerts wrote:
This has been fixed in blead. -zefram |
From @iabynOn Mon, May 01, 2017 at 02:51:16AM +0100, Zefram wrote:
in particular, v5.25.0-36-gda7cf1c: fix #128109 - do not move RExC_open_parens[0] in reginsert -- |
@iabyn - Status changed from 'open' to 'resolved' |
From @ilmariDave Mitchell <davem@iabyn.com> writes:
That commit, and v5.25.1-196-gec5bd2262b, which adds a test for it, -- |
From @demerphqOn 2 May 2017 at 17:39, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:
I just pushed 78b60e1 Add tests for regex recursion to maint-5.24 -- I took the liberty of including 5edefd5 which was These patches are equivalent to (respectively): Cheers, -- |
From @demerphqOn 11 April 2017 at 12:36, Jan Goyvaerts <jg@jgsoft.com> wrote:
Rereading this thread I realized I did not comment on this, so i will now. I would not emulate the buggy behavior.
The patches are now pushed to the maint branch, so the next release of Yves |
From @steve-m-hayOn 2 May 2017 at 18:38, demerphq <demerphq@gmail.com> wrote:
I don't think this should have been done right now. maint-5.22 and There are many, many other bug fixes (crashes and otherwise) that |
From @demerphqOn 2 May 2017 at 22:49, Steve Hay <steve.m.hay@googlemail.com> wrote:
Sigh. Honestly I give up. Our release process is broken. If it weren't Anyway, bulk reverted in 9ecaf4d cheers, -- |
From @steve-m-hayOn 3 May 2017 at 07:56, demerphq <demerphq@gmail.com> wrote:
Thanks.
I wasn't aware that the discussions over changes in branch management Regarding remembering that patches get backported, I think the Also, patches are supposed to get three votes before being backported It's not obvious to me that this process needs to change since it |
From @demerphqOn 3 May 2017 at 09:25, Steve Hay <steve.m.hay@googlemail.com> wrote:
I had no direct thoughts on this, but when i wrote that mail I was
Ok, I think I missed some of these details being announced. However I Also this maint-votes branch is pretty clunky. Why don't we just put a Anyway, if there is a file I can edit for these things then obviously
I was under the impression that there were, at least for two of the
No. I withdraw that. Albeit clunkier than I would like there is a On the other hand, I then think we should put push blocks on these If I had pushed and gotten back a message back telling me what to do I ########################################################################### git push origin $branch:backports/$branch-TOPIC And then we write add-backport-patch to do whatever vote file git Yves -- |
From @steve-m-hayOn 3 May 2017 at 09:00, demerphq <demerphq@gmail.com> wrote:
You're right - there is no mention of it in the docs (even in blead),
I think this may have been discussed at the time the voting files were
Sorry if I missed that. This is why having a file *somewhere* to keep
I think I would welcome the idea of push blocks (on the proposed blead
|
From @demerphq[using * to mark my comments since apparently gmail html to text is
* Ok, cool.
* I see. Hrm. Thinking about it more if there was a tool that managed * I guess I have to write a tool. :-)
* Yes. The issues of votes requires somewhere to store and track them.
* this is all me here We could do that, but it would require some finagling. Perhaps better vote: demerphq then the push hook can validate that, and if it is missing give a (IIRC There is a more formal git concept of signed-off-by but i dont cheers, -- |
From @craigberryOn Wed, May 3, 2017 at 4:21 AM, demerphq <demerphq@gmail.com> wrote:
I think what's missing in this discussion is the degree to which the All of which is to say, heavy-duty automation that enforces the maint |
From @demerphqOn 4 May 2017 23:41, "Craig A. Berry" <craig.a.berry@gmail.com> wrote: On Wed, May 3, 2017 at 4:21 AM, demerphq <demerphq@gmail.com> wrote:
I think what's missing in this discussion is the degree to which the Thanks for saying this. All of which is to say, heavy-duty automation that enforces the maint When I was in high school I did a short course in a sheet metal and machine Having said that you have a point about not making policy from a black Swan We can ruminate on this for a bit and revisit it the next time someone Cheers |
From @steve-m-hayOn 3 May 2017 at 09:19, Steve Hay <steve.m.hay@googlemail.com> wrote:
I've now documented the current process in commit Hopefully this will avert similar confusion in the future. If not then |
Migrated from rt.perl.org#128420 (status was 'resolved')
Searchable as RT128420$
The text was updated successfully, but these errors were encountered: