[perl #93590] $tainted ~~ [...] failing

When smartmatch is about to start, to avoid calling get-magic (e.g.,
FETCH methods) more than once, it copies any argument that has
get-magic.

Tainting uses get-magic to taint the expression.  Calling mg_get(sv)
on a tainted scalar causes PL_tainted to be set, causing any scalars
modified by sv_setsv_flags to be tainted.  That means that tainting
magic gets copied from one scalar to another.

So when smartmatch tries to copy the variable to avoid repeated calls
to magic, it still copies taint magic to the new variable.

For $scalar ~~ @array (or ~~ [...]), S_do_smartmatch calls itself
recursively for each element of @array, with $scalar (on the suppos-
edly non-magical copy of $scalar) on the left and the element on
the right.

In that recursive call, it again does the get-magic check and copies
the argument.  Since the copied of a tainted variable on the LHS is
magical, it gets copied again.  Since the first copy is a mortal
(marked TEMP) with a refcount of one, the second copy steal its
string buffer.

The outer call to S_do_smartmatch then proceeds with the second ele-
ment of @array, without realising that its copy of $scalar has lost
its string buffer and is now undefined.

So these produce incorrect results under -T (where $^X is ‘perl’):

    $^X =~ ["whatever", undef]  # matches
    $^X =~ ["whatever", "perl"] # fails

This problem did not start occurring until this commit:

commit 8985fe98dcc5c0af2fadeac15dfbc13f553ee7fc
Author: David Mitchell <davem@iabyn.com>
Date:   Thu Dec 30 10:32:44 2010 +0000

    Better handling of magic methods freeing the SV

mg_get used to increase the refcount unconditionally, pushing it on to
the mortals stack.  So the magical copy would have had a refcount of
2, preventing its string buffer from being stolen.  Now it has a ref-
erence count of 1.

This commit solves it by adding a new parameter to S_do_smartmatch
telling it that the variable has already been copied and does not even
need to be checked.  The $scalar~~@array case sets that parameter for
the recursive calls.  That avoids the whole string-stealing problem
*and* avoids extra unnecessary SVs.

1 files changed, 5 insertions, 5 deletions

diff --git a/pp_ctl.c b/pp_ctl.c
index 2e89b7053c..52b0682d0d 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -4445,14 +4445,14 @@ S_destroy_matcher(pTHX_ PMOP *matcher)
 PP(pp_smartmatch)
 {
     DEBUG_M(Perl_deb(aTHX_ "Starting smart match resolution\n"));
-    return do_smartmatch(NULL, NULL);
+    return do_smartmatch(NULL, NULL, 0);
 }
 
 /* This version of do_smartmatch() implements the
  * table of smart matches that is found in perlsyn.
  */
 STATIC OP *
-S_do_smartmatch(pTHX_ HV *seen_this, HV *seen_other)
+S_do_smartmatch(pTHX_ HV *seen_this, HV *seen_other, const bool copied)
 {
     dVAR;
     dSP;
@@ -4464,7 +4464,7 @@ S_do_smartmatch(pTHX_ HV *seen_this, HV *seen_other)
     /* Take care only to invoke mg_get() once for each argument.
      * Currently we do this by copying the SV if it's magical. */
     if (d) {
-	if (SvGMAGICAL(d))
+	if (!copied && SvGMAGICAL(d))
 	    d = sv_mortalcopy(d);
     }
     else
@@ -4775,7 +4775,7 @@ S_do_smartmatch(pTHX_ HV *seen_this, HV *seen_other)
 			
 			PUTBACK;
 			DEBUG_M(Perl_deb(aTHX_ "        recursively comparing array element...\n"));
-			(void) do_smartmatch(seen_this, seen_other);
+			(void) do_smartmatch(seen_this, seen_other, 0);
 			SPAGAIN;
 			DEBUG_M(Perl_deb(aTHX_ "        recursion finished\n"));
 			
@@ -4837,7 +4837,7 @@ S_do_smartmatch(pTHX_ HV *seen_this, HV *seen_other)
 		    PUTBACK;
 		    /* infinite recursion isn't supposed to happen here */
 		    DEBUG_M(Perl_deb(aTHX_ "        recursively testing array element...\n"));
-		    (void) do_smartmatch(NULL, NULL);
+		    (void) do_smartmatch(NULL, NULL, 1);
 		    SPAGAIN;
 		    DEBUG_M(Perl_deb(aTHX_ "        recursion finished\n"));
 		    if (SvTRUEx(POPs))