multiconcat: /$a(?{ $b . "c" })/ could crash
authorDavid Mitchell <davem@iabyn.com>
Mon, 19 Feb 2018 21:32:36 +0000 (21:32 +0000)
committerDavid Mitchell <davem@iabyn.com>
Mon, 19 Feb 2018 22:06:49 +0000 (22:06 +0000)
RT #132772

Due to the weird order in which codeblocks within patterns are optimised,
it was possible for the OP_CONCAT -> OP_MULTICONCAT optimisation to leave
freed ops in the op execution chain, leading to assertion failures or
crashes.

In particular, optimize_optree() needs to always be called before
CALL_PEEP(), otherwise the individual subtrees which make up the two
children of a concat op may not have the head of subtree as the last op in
the subtree's op_next chain: in the subtree

    RV2SV
      |
     GV

this subtree gets peephole-optimised to

    ex-RV2SV
      |
     GVSV

and GVSV->op_next no longer points to the ex-RV2SV but rather directly
to RV2SV->op_next. But S_maybe_multiconcat() assumes that the head of each
subtree is the last op to be executed: It updates RV2SV->op_next when
reorganising the optree, but leaves GVSV->op_next possibly pointing at a
freed op.

This commit provides a minimal fix by unconditionally calling
optimize_optree() on each code block in Perl_pmruntime(). This may
mean that optimize_optree() may be run against the same code block again
later, but apart from the slight inefficiency, this should be harmless.

A more general fix will be applied post 5.28.0 release.

op.c
t/re/pat_re_eval.t

diff --git a/op.c b/op.c
index b94553f..2275177 100644 (file)
--- a/op.c
+++ b/op.c
@@ -6952,9 +6952,15 @@ Perl_pmruntime(pTHX_ OP *o, OP *expr, OP *repl, UV flags, I32 floor)
                op_null(scope);
            }
 
-           if (is_compiletime)
-               /* runtime finalizes as part of finalizing whole tree */
-                optimize_optree(o);
+            /* XXX optimize_optree() must be called on o before
+             * CALL_PEEP(), as currently S_maybe_multiconcat() can't
+             * currently cope with a peephole-optimised optree.
+             * Calling optimize_optree() here ensures that condition
+             * is met, but may mean optimize_optree() is applied
+             * to the same optree later (where hopefully it won't do any
+             * harm as it can't convert an op to multiconcat if it's
+             * already been converted */
+            optimize_optree(o);
 
            /* have to peep the DOs individually as we've removed it from
             * the op_next chain */
index efcff4e..f88a865 100644 (file)
@@ -23,7 +23,7 @@ BEGIN {
 
 our @global;
 
-plan tests => 497;  # Update this when adding/deleting tests.
+plan tests => 502;  # Update this when adding/deleting tests.
 
 run_tests() unless caller;
 
@@ -1278,6 +1278,29 @@ sub run_tests {
         is $max, 2, "RT #126697";
     }
 
+    # RT #132772
+    #
+    # Ensure that optimisation of OP_CONST into OP_MULTICONCAT doesn't
+    # leave any freed ops in the execution path. This is is associated
+    # with rpeep() being called before optimize_optree(), which causes
+    # gv/rv2sv to be prematurely optimised into gvsv, confusing
+    # S_maybe_multiconcat when it tries to reorganise a concat subtree
+    # into a multiconcat list
+
+    {
+        my $a = "a";
+        local $b = "b"; # not lexical, so optimised to OP_GVSV
+        local $_ = "abc";
+        ok /^a(??{ $b."c" })$/,  "RT #132772 - compile time";
+        ok /^$a(??{ $b."c" })$/, "RT #132772 - run time";
+        my $qr = qr/^a(??{ $b."c" })$/;
+        ok /$qr/,  "RT #132772 - compile time time qr//";
+        $qr = qr/(??{ $b."c" })$/;
+        ok /^a$qr$/,  "RT #132772 -  compile time time qr// compound";
+        $qr = qr/$a(??{ $b."c" })$/;
+        ok /^$qr$/,  "RT #132772 -  run time time qr//";
+    }
+
 
 } # End of sub run_tests