Segfault in S_study_chunk (regcomp.c:4870) #16947
Comments
From @dur-randirCreated by @dur-randirWhile fuzzing perl v5.29.9-63-g2496d8f3f7 built with afl and run 0 =~ /0(?n)|()(()(()(?0)|(?0)))(0)/ to segfault. GDB stack trace is following #0 0x000055555568f92c in S_study_chunk (pRExC_state=0x7fffffffd650, This is present a regression between 5.24 and 5.26, bisect points to commit da7cf1c 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 Perl Info |
From @dur-randirAnother similar example, caused by the same commit, but with another stack trace: 0 =~ /0(?n)|()(()((?0)|(?0)))0*\N0/ Program terminated with signal SIGFPE, Arithmetic exception. |
From @khwilliamsonHere is what the pattern looks up before the start of optimization 1: BRANCH (4) |
|
The RT System itself - Status changed from 'new' to 'open' |
From @khwilliamsonOn 4/10/19 5:22 PM, Karl Williamson via RT wrote:
This is the result of the first example in the ticket. |
From @hvdsOn Wed, 10 Apr 2019 16:31:13 -0700, [email protected] wrote:
As far as I can see, the loop: is falling off the end in one of its incarnations; I don't yet understand why that's happening. I wonder whether it would be valid to change the loop condition to FWIW I find it marginally easier to track what's going on with the hv/study_chunk branch, even though it's a bit out of date. That gives this stack trace: .. but deepest stack has had at least two further levels of study_chunk -> rck_make_trie frames before coming back to hit this SEGV. Unsurprisingly this does not fail if perl is built without PERL_ENABLE_TRIE_OPTIMISATION. Changing the condition to Hugo |
From @khwilliamsonOn 4/11/19 10:15 AM, Hugo van der Sanden via RT wrote:
I would think that using cur != scan && OP(cur) != END' |
From @hvdsOn Thu, 11 Apr 2019 10:05:43 -0700, public@khwilliamson.com wrote:
Hmm, the second case still fails with that variant: I'll try digging into that when I have some more time - Yves, if you have any chance to look at this I'm sure that would help a lot. Hugo |
From @hvdsOn Thu, 11 Apr 2019 10:41:35 -0700, hv wrote:
With -Dr that second case hits an assertion with all three variants of the loop: Disturbingly, if I fix the assertion as advised with the patch below, that case survives with -Dr but still gives a floating point exception without -Dr. It turns out that _with_ -Dr we're hitting the startbranch..scan loop twice; _without_ -Dr we're hitting it a third time, and by the looks of it that third time the scan we're aiming at is in an optimized-out section: The difference appears to be because in at least some cases we only mark optimized-out nodes as OPTIMIZED if -Dr is enabled: the point of divergence is the test Hugo Inline Patchdiff --git a/regcomp.c b/regcomp.c
index fbd5c18..ba2b2b5 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -8316,12 +8316,11 @@ Perl_re_op_compile(pTHX_ SV ** const patternp, int pat_count,
* the OPEN/CURLYM (CURLYM's are special and can act like OPEN's)
* it refers to.
*
- * If for some reason someone writes code that optimises
- * away a GOSUB opcode then the assert should be changed to
- * an if(scan) to guard the ARG2L_SET() - Yves
- *
*/
- assert(scan && OP(scan) == GOSUB);
+ assert(scan);
+ if (OP(scan) == OPTIMIZED)
+ continue;
+ assert(OP(scan) == GOSUB);
ARG2L_SET( scan, RExC_open_parens[ARG(scan)] - REGNODE_OFFSET(scan));
}
|
From @dur-randirOn Thu, 25 Apr 2019 04:18:31 -0700, hv wrote:
Maybe this will help, I have a case that fails even with this patch both with and without -Dr: "000000"=~/0(?0)|0(?|0|0)/ emitting 'panic: regexp memory corruption'. |
From @hvdsOn Sun, 05 May 2019 00:39:15 -0700, randir wrote:
Cool, I'll take a look at that when I'm back home at the end of the week if nobody gets there first. Hugo |
|
I hoped the hv/gh17553 branch would fix this, but no. |
Sorry, I also completely dropped this one. I'll try to dig some more - an initial look shows that the Clearly the first case should have had a "10: TRIE-EXACT[y]" (or possibly "EXACT <y>") that's gone missing. |
|
Ok, what's happening is approximately:
A possible solution is to inhibit all changes that involve rewriting nodes while enframed - things like join_exact, making tries, curly upgrades, switches between anyof nodes and exactf nodes etc. In this case it's enough just to inhibit tries: @xsawyerx I think we should aim to have this (along with a test) as part of 5.32; I'll start looking at other types of transformation to inhibit similarly, I don't know to what extent we should be aiming to cover all of them for 5.32. |
|
Created PR #17714 for this. |
|
Let's have it approved and merged prior to 5.31.11 (in 9 days). |
|
merged via 869e8e3 |
|
I want to keep this open for the other types of regexp transformation, I'll look further at those tomorrow. |
|
@xsawyerx @khwilliamson: I've created PR #17715 to cover all the other examples I can find of op types being changed during study_chunk. The nature of the change is the same for each chunk, and in every case I expect the outer frame will eventually see the same ops and make the same decision, so I feel pretty confident this should not introduce problems. |
|
@hvds Thank you for doing the work. I approved it as well. |
|
@khwilliamson++ has merged the second branch, closing now. |
|
It turns out this a bit more complicated:
The fixes merged do not take account of SUSPEND, so the optional optimizations and mandatory fixups are not happening at all, as reported by @dur-randir in #17715: To fix this, we need to replace the simple I'll start working on that now. |
|
I've pushed the branch https://github.com/Perl/perl5/tree/gh16947-3 with my proposed fix for this for early eyeballs (@khwilliamson, @demerphq in particular). I'm testing it now, and will also try to add some more tests exploring interactions between GOSUB and SUSPEND at various depths. |
|
On 4/18/20 9:52 AM, Hugo van der Sanden wrote:
I've pushed the branch https://github.com/Perl/perl5/tree/gh16947-3 with
my proposed fix for this for early eyeballs ***@***.***
<https://github.com/khwilliamson>, @demerphq
<https://github.com/demerphq> in particular).
I'm testing it now, and will also try to add some more tests exploring
interactions between GOSUB and SUSPEND at various depths.
At the very least, that clears up the panic.
jimk
|
|
I observed no new failures on this branch after a day, but since it'a stochastic process, it's not a bullet-proof confirmation. I'm keeping it to run more. |
|
I've made a pull request #17736 for this. I've eyeballed -Dr output for a variety of possible frame-to-frame interactions, and they all look ok to me. There's a possibility that interaction between enframing and simple recursion may still be able to trip this up, though I haven't found an example that does so. If that were possible we'd also need to give study_chunk an additional argument so as to pass through either |
|
Fix approved! |
|
Merged #17736. |
Migrated from rt.perl.org#134017 (status was 'open')
Searchable as RT134017$
The text was updated successfully, but these errors were encountered: