-
Notifications
You must be signed in to change notification settings - Fork 560
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
Assertion failed in Perl_reg_numbered_buff_fetch, file regcomp.c, line 7459 #14081
Comments
From [email protected]Created by [email protected]Have been running 5.20.1-RC2 here under FreeBSD 10.0 for a couple of Today one of the forked child process has crashed (SIGABRT) Assertion failed: Perl is build with gcc 4.8.4 20140828 with debugging enabled, The extra safeguards were there just in case - to rule out The coredump shows the following: # gdb /usr/local/bin/perl /var/coredumps/perl-97654.core (gdb) bt This happened during processing of the first MIME part (a rather The crash occurs within SpamAssassin code (the last debug Perl Info |
From [email protected]
Made some progress in narrowing this down, can reproduce it The crash involves a s/// operator with a horribly complicated Will try to narrow it down further... |
From @khwilliamsonOn 09/10/2014 10:23 AM, Mark Martinec wrote:
That would be helpful. One perhaps easy option is to try it with the Something else is to run it with valgrind. This may well be the result Perhaps this info will aid you in the narrowing. The core dump Here is the comment at the beginning of Perl_save_re_context(): That indicates what we're up against. It is failing because of a |
|
The RT System itself - Status changed from 'new' to 'open' |
From [email protected]Thanks Karl for a quick response! Got it down to a sensible size, I'm sure it can be reduced further, I realized it also fails on perl 5.20.0 build with clang. The key seems to be in: use re 'taint'; ...removing that avoids the crash. Here is now the test program: #!/usr/bin/perl use strict; my $tlds = qr/ my $email_regex = qr/ my(@body) = ( for (@body) { |
From @khwilliamsonOn 09/10/2014 11:51 AM, Mark Martinec wrote:
This is not a recent regression, as it fails back through at least 5.12. I'm hoping someone with more expertise than I currently have in this |
From [email protected]Got it down to this small test program: #!/usr/bin/perl use strict; my(@body) = ( for (@body) { perl 5.20.{0,1} : |
From @demerphqOn 11 September 2014 15:05, Mark Martinec <Mark.Martinec@ijs.si> wrote:
Are you sure you have the script right? Because that code never fetches I added a "print $1" to the script: $ cat rt122747.t use strict; my(@body) = ( for (@body) { And here is what I get from blead: $ ./perl -Ilib -T rt122747.t /me confused. Yves -- |
From @demerphqOn 10 September 2014 20:55, Karl Williamson <public@khwilliamson.com> wrote:
I cant reproduce it with bleadperl at all. Nor can I reproduce with 5.14. So I am pretty confused here. What script did you use to determine it is Yves -- |
From @HugmeirOn Thu, Sep 11, 2014 at 4:57 PM, demerphq <demerphq@gmail.com> wrote:
I can reproduce this on 5.10-5.20 but only for debugging builds; maybe |
From @demerphq
Bah, thought I was on a DEBUGGING build, but I wasnt. Thanks Brian. Yves -- |
From @cpansproutOn Thu Sep 11 06:07:03 2014, mmartinec wrote:
I think what’s happening is that the kludge to localise $1, etc. is executed when the regexp is in an inconsistent state. rx->subbeg is referring to the string from the previous match ('<mailto:xxxx.xxxx@outlook.com>'), but the offsets for $1 extend beyond the end of the 30-character string: (gdb) p rx->offs[1] A watchpoint on rx->offs shows that it gets swapped out here in regexec.c: 2706 swap = prog->offs; when the backtrace is like this: #0 Perl_regexec_flags (my_perl=0x100803200, rx=0x10082fdf8, stringarg=0x10060b658 "A¹kerèeva xxxx.xxxx@outlook.com ”", strend=0x10060b67d "", strbeg=0x10060b658 "A¹kerèeva xxxx.xxxx@outlook.com ”", minend=0, sv=0x1008063e8, data=0x0, flags=1) at regexec.c:2709 So the ordering of some of this stuff needs to be rethought. A git bisect points me to this commit: commit 44a2ac7 Re: [PATCH] Change implementation of %+ to use a proper tied hash interface and add support for %- But I think it’s a false positive. -- Father Chrysostomos |
From [email protected]
Indeed, I'm using a -DDEBUGGING perl.
Yes. |
From @demerphqOn 11 September 2014 17:36, Mark Martinec <Mark.Martinec@ijs.si> wrote:
Hrm, well even on a DEBUGGING build I cannot replicate in blead. Can you show me your perl -V and the output of MY version of your script? Brian if you happen to have your Configure options handy i would appreciate And again I find it very odd that a script which never reads a capture Yves -- |
From @demerphq#!/usr/bin/perl use strict; my(@body) = ( for (@body) { |
From @demerphq#!/usr/bin/perl use strict; my $tlds = qr/ my $email_regex = qr/ my(@body) = ( for (@body) { |
From [email protected]
$ ./rt122747.t Happens on two hosts, one has 5.20.1-RC2 as documented at the top of the other host is a 5.20.0, built with a clang 3.4.1 compiler, $ perl -V Platform: Characteristics of this binary (from libperl): |
From @demerphqOn 11 September 2014 17:30, Father Chrysostomos via RT <
Yes it almost definitely is. That is the patch where I still do not see where this function is called. Can you show me the Yves |
From @HugmeirOn Thu, Sep 11, 2014 at 5:51 PM, demerphq <demerphq@gmail.com> wrote:
ml99299:perl-blead brfraser$ ./perl -Ilib ~/Downloads/rt122747.t ml99299:perl-blead brfraser$ ./perl -Ilib ~/Downloads/rt122747_2.t $ ./perl -Ilib -V Characteristics of this binary (from libperl): |
From @demerphqOn 11 September 2014 18:13, Brian Fraser <fraserbn@gmail.com> wrote:
Well, this doesnt make any sense to me. Can you show me the exact Configure you used, because so far every one I For instance I used this: ./Configure -Doptimize=-g -d -Dusedevel -Dusethreads -Dcc=ccache\ gcc and this: ./Configure -Doptimize=-g -d -Dusedevel -Dcc=ccache\ gcc -Dld=gcc and neither produce the failure you describe. I would very much like to help fix this, but if I cant replicate it I cant
You have a lot of environment references to Perl 5.18.2. I wonder if that
Here is my perl -V: $ ./perl -Ilib -V Characteristics of this binary (from libperl): -- |
From [email protected]I have repeated the exercise with a fresh install of perl-blead $ perlbrew install blead -DDEBUGGING $ perlbrew use perl-blead $ perl -V Characteristics of this binary (from libperl): $ type perl $ perl ~/rt122747.t |
From @demerphqOn 11 September 2014 20:09, Mark Martinec <Mark.Martinec@ijs.si> wrote:
Thanks. I am still working out why I have not been able to get Perl to I changed the code to not use an assert but rather a simple if and I am I am going to try to get to the bottom of this today. Yves -- |
From @demerphqOn 11 September 2014 20:23, demerphq <demerphq@gmail.com> wrote:
Which was that you need to do a git clean -dfX to wipe some things created Yves -- |
From @demerphqOn 11 September 2014 17:51, demerphq <demerphq@gmail.com> wrote:
Now I can. Configure being overly "helpful" meant i was not building with Now I can replicate I have determined the the use re 'taint'; is apparently Since the re taint is not necessary this means the relation to the utf8 Yves |
From @demerphq#!/usr/bin/perl use strict; my(@body) = ( for (@body) { |
From @demerphqOn 11 September 2014 20:41, demerphq <demerphq@gmail.com> wrote:
I was misreading things. In fact this is relevant: #0 0x00007ffff70e9037 in __GI_raise (sig=sig@entry=6) at Do we really need to use the regex engine for swash init? Wouldnt the Yves -- |
From @demerphqOn 11 September 2014 21:32, demerphq <demerphq@gmail.com> wrote:
Or maybe trigger the swash_init() during regex *compile*. Yves -- |
From @cpansproutOn Thu Sep 11 12:32:57 2014, demerphq wrote:
So you’ve beaten me to the backtrace.
Short of that, could we just stop the init code from using regexps itself? That said, is it even necessary at present to localise $1 et al.? As long as the swash init code does not access $1 after a failed match, would it matter that the current (outer) regexp is in an inconsistent state? Or could we make PL_curpm null during the swash init? -- Father Chrysostomos |
From @cpansproutOn Thu Sep 11 11:26:03 2014, demerphq wrote:
‘rm config.sh Policy.sh’ usually works for me, but I may be missing something. At least I don’t have to rebuild everything, though. -- Father Chrysostomos |
From @demerphqOn 11 September 2014 21:40, Father Chrysostomos via RT <
That is what I meant.
The latter is an amazingly good idea and I am testing a patch that does Yves |
From @demerphqOn 11 September 2014 21:53, demerphq <demerphq@gmail.com> wrote:
It passed basic regex tests. I am running a full test now, but at the same commit 55b10d6 perl #122747: localize PL_curpm to null in _core_swash_init This is a naive patch to set PL_curpm to null before we do any Thanks to FC for the suggestion! Yves -- |
From @demerphqOn 11 September 2014 22:00, demerphq <demerphq@gmail.com> wrote:
And now I just pushed to blead: commit 2c1f00b perl #122747: localize PL_curpm to null in _core_swash_init Set PL_curpm to null before we do any swash intialization Long term you could argue that we should just not use the regex Thanks to FC for the suggestion! I believe that this ticket can be closed. Yves -- |
From @cpansproutOn Thu Sep 11 13:49:31 2014, demerphq wrote:
Wait. Two things: • Let’s commit a test. That localisation doesn’t make much sense to me, even without your PL_curpm change. Saving and restoring the value of something that is just a proxy for a value stored elsewhere is weird. Can we just delete save_re_context? (I guess the latter issue doesn’t need to keep the ticket open.) -- Father Chrysostomos |
From @demerphqOn 11 September 2014 23:42, Father Chrysostomos via RT <
Ah, ahem. Good catch. :-)
Agreed. Lets open a different ticket for that. Yves -- |
From [email protected]
Thanks you all for the fast resolution! Will this be able to make it into 5.20.1-RC3 ? |
From @cpansproutOn Thu Sep 11 14:59:25 2014, mmartinec wrote:
Seeing that this is a crashing bug, it meets the policy. It gets my vote. (2c1f00b, that is, which alone is sufficient for maint.) -- Father Chrysostomos |
From @iabynOn Thu, Sep 11, 2014 at 03:12:12PM -0700, Father Chrysostomos via RT wrote:
On the other hand, its a long-standing (and clearly rare) issue, and -- |
From @jkeenanOn 09/12/2014 06:27 AM, Dave Mitchell wrote:
What-Dave-said++ |
From @demerphqOn 12 September 2014 13:08, James E Keenan <jkeen@verizon.net> wrote:
FWIW, I think delaying for a minor release is a good idea. If Mark really Sorry for any inconvenience Mark, but this patch has characteristics, (such Yves -- |
From [email protected]
Sure, understood. It is indeed uncomfortably close to a 5.20.1 release.
If the issue is otherwise not harmful (like causing memory corruption) The event is not so rare (e.g. one such case per several days of mail |
From @demerphqOn 12 September 2014 17:46, Mark Martinec <Mark.Martinec@ijs.si> wrote:
I think that is correct. I would need to audit to be sure.
Hrm. That is annoying.
I see. I dont know if that changes the balance of issues enough to justify it Yves -- |
From @demerphqOn 11 September 2014 23:42, Father Chrysostomos via RT <
Done in: 409c647
Did you already follow up on this? Yves -- |
From @cpansproutOn Sun Sep 14 09:57:14 2014, demerphq wrote:
Yes, in these commits (in reverse order): 2018906 pp_ctl.c: Remove junk from #endif -- Father Chrysostomos |
From @demerphqOn 14 September 2014 21:49, Father Chrysostomos via RT <
Great. Thanks. BTW, did you dig into the history of the function to see why Yves -- |
From @cpansproutOn Sun Sep 14 14:15:18 2014, demerphq wrote:
7d75537 explains the history.
Yes, originally save_re_context saved a whole list of global variables used by the regexp engine (which no longer exist). The localisation of $1 etc. was added later. I’m not sure that part ever made sense. I’m not sure either that it’s worth trying to figure it out. It was commit ada6e8a that did it, to fix bug #18107. -- Father Chrysostomos |
From @demerphqOn 14 September 2014 23:30, Father Chrysostomos via RT <
Cool thanks, that was very educational. Yves -- |
|
@cpansprout - Status changed from 'open' to 'pending release' |
From @iabynOn Sun, Sep 14, 2014 at 02:30:41PM -0700, Father Chrysostomos via RT wrote:
I've always had the strong suspicion that the $1 localization was an -- |
From @khwilliamsonThanks for submitting this ticket The issue should be resolved with the release today of Perl v5.22. 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#122747 (status was 'resolved')
Searchable as RT122747$
The text was updated successfully, but these errors were encountered: