-
Notifications
You must be signed in to change notification settings - Fork 550
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
regexec.c stack overflow #15669
Comments
From @geeknikI'm not convinced that this is an actual bug, but #p5p was silent when I perl -e '/(?{m}(0)},s\/\/\/})//0' ==6615==ERROR: AddressSanitizer: stack-overflow on address 0x7ffc78f88ca8 SUMMARY: AddressSanitizer: stack-overflow ??:0 calloc ==19424== Stack overflow in thread 1: can't grow stack to 0xffe801f90 |
From @cpansproutOn Mon Oct 17 10:26:19 2016, brian.carpenter@gmail.com wrote:
I think it is.
That is nonsensical code. I do not have 5.16 handy. The output from 5.14 is what I would expect.
It should not even be getting that far. It should fail at compile time. -- Father Chrysostomos |
The RT System itself - Status changed from 'new' to 'open' |
From @dcollinsn$ perl5.14.0 -e '/(?{m}(0)},s\/\/\/})//0' Mostly complete fix for literal /(?{..})/ blocks Change the way that code blocks in patterns are parsed and executed, (Note that this fix only applies to literal code blocks appearing within This change means that for literal /(?{..})/ and /(??{..})/: * the code block is now fully parsed in the same pass as the surrounding * Error and warning messages will now appear to emanate from the main #!/usr/bin/perl has changed from boo at (re_eval 1) line 1. to boo at /tmp/p line 2. * scope and closures now behave as you might expect; for example for my $x (qw(a b c)) { "" =~ /(?{ print $x })/ } now prints "abc" rather than "" * with recursion, it now finds the lexical within the appropriate depth sub recurse { * an earlier fix that stopped 'my' declarations within code blocks * UNITCHECK blocks within literal code blocks are now run as part of the This is all achieved by building upon the efforts of the commits which * the LISTOP generated by the parser, which contains all the code LIST * each of the code blocks has its last op set to null and is * then in re_op_compile(), we concatenate the list of CONSTs to produce * (if the current regex engine isn't the built-in perl one, then we just * then during regex compilation, whenever we encounter a '(?{', we see * During execution, when an EVAL op is encountered, if data->what is :100644 100644 75ee7e7bae9366e089980b8a7d346198645cdfe9 -- |
From @demerphqOn 18 October 2016 at 03:17, Father Chrysostomos via RT
Do you mean just the '/0' or the full pattern? With some editing I read the original code as being the same as (m/(?{ Which is strange but IMO valid code.
I am not sure why. Is it because you expect the first } to close the I would say the 5.14.4 behavior is a misparse. If this is legal: perl -le'print q}foo}' then I would expect this: m}(0)} to be legal too, and i would expect it to be legal inside of a (?{ ...
Looking forward to hearing more on why. Yves -- |
From @demerphqOn 18 October 2016 at 10:00, demerphq <demerphq@gmail.com> wrote:
I dont think the simplified version of the code should infinite loop For this code (because $_ is the empty string) we should perform an m/(?{ m!(0)!, s!!! })/ to m/(?{ m!(0)!, s!(0)!! })/ which should be equivalent valgrind is happy and we get what I expect, $ ./perl -le'(m/(?{ m!(0)!, s!(0)!! })/) / 0' $ ./perl -le'(m/(?{ m!(0)!, s!!! })/) / 0' And here is the valgrind output: $ valgrind ./perl -le'(m/(?{ m!(0)!, s!!! })/) / 0' This is definitely a bug. Yves -- |
From @demerphqOn 18 October 2016 at 10:17, demerphq <demerphq@gmail.com> wrote:
And yet another example of the empty pattern special case causing Yves -- |
From @demerphqOn 18 October 2016 at 10:21, demerphq <demerphq@gmail.com> wrote:
Actually this case can be boiled down to: ./perl -le'/(?{ s!!! })/' which causes an infinite loop due to PL_curpm being set at the start I think we can fix it by checking PL_curpm against PL_reg_curpm when Yves -- |
From @hvdsOn Tue Oct 18 02:30:01 2016, demerphq wrote:
And do what? Even if they're the same, it still might also legitimately be the last successful match, no? It seems to me we can't offer the promised behaviour unless there's a variable that really is set only at the point we complete a successful match. Hugo |
From @demerphqOn 18 October 2016 at 12:03, Hugo van der Sanden via RT
No I don't think so, and all the tests pass. As far as I can tell PL_reg_curpm is a special opcode which we use On the other hand we can simply forbid the use of the empty pattern Yves |
From @cpansproutOn Tue Oct 18 01:01:21 2016, demerphq wrote:
I do not know what happened, but the line that I quoted was: perl -e '/(?{m}(0)},s\/\/\/})//0' not: perl -e '/0'
I see now that I misread the code. the } for the m}} delimiter was what threw me off. -- Father Chrysostomos |
From @cpansproutOn Tue Oct 18 04:08:30 2016, demerphq wrote:
PL_curpm does far too many things. This is not the only bug that results. -- Father Chrysostomos |
From @demerphqOn 18 Oct 2016 23:46, "Father Chrysostomos via RT" <
Can you expand on that thought a bit please? Yves |
From @demerphqOn 19 October 2016 at 09:22, demerphq <demerphq@gmail.com> wrote:
BTW, I pushed a patch to forbid use of the empty pattern inside of Once I understand what else you worry about with PL_curpm I will do a But I figure a run time catchable fatal exception is better than a C Yves -- |
From @cpansproutOn Wed Oct 19 00:23:19 2016, demerphq wrote:
Having PL_curpm point to the actual pattern in the op tree results in different recursion levels sharing the same set of match variables: $u = ",echle etn sJ"; (I thought I had reported this before, but I cannot find the ticket now.) Using a qr// works. Similarly, having m|(?{ // })| try to use the *current* pattern instead of the last pattern to match successfully—solely due to an implementation detail—seems wrong. PL_curpm is serving three purposes: the last match op, the last set of matched buffers ($1 etc.), and the innermost pattern enclosing the currently executing code. -- Father Chrysostomos |
From @demerphqOn 19 October 2016 at 22:51, Father Chrysostomos via RT
Wow. It took me a while to work that through. :-) I see what you mean. That is going to be a lot harder to fix than the
Yes I agree.
Yes, I see what you mean. I think I might have put it differently, but Yves |
From @demerphqOn 19 October 2016 at 23:29, demerphq <demerphq@gmail.com> wrote:
So following up for this I put together a quick hack to make it ./perl -le'$_="aa"; /a/; /(?{ s!!x! })/; print; //; print' I personally think this is no less ugly than the previous patch, but FWIW, this seems to be yet another good reason to get rid of the empty pattern. Yves |
From @demerphqOn 20 October 2016 at 09:37, demerphq <demerphq@gmail.com> wrote:
Oh, here is another reason: ./perl -le'$_="ab"; my $p=qr/(?{ s!!x! })/; /$p/; print; /a/; /$p/; Crazy. -- |
From @cpansproutOn Thu Oct 20 00:37:45 2016, demerphq wrote:
I would prefer it, but I do not feel strongly about it.
I am still opposed to that, unfortunately. :-) -- Father Chrysostomos |
From @cpansproutOn Thu Oct 20 00:42:10 2016, demerphq wrote:
I would agree that the *Perl* code there is crazy and the author of it gets what he deserves. -- Father Chrysostomos |
From @demerphqOn 20 October 2016 at 22:12, Father Chrysostomos via RT
Well I just push a patch to meet your preference.
We can debate that one (again) in another thread. :-) Yves |
Looks like the final fix for this was 5585e75 (20 Oct 2016), closing. |
Migrated from rt.perl.org#129903 (status was 'open')
Searchable as RT129903$
The text was updated successfully, but these errors were encountered: