develooper Front page | perl.perl5.porters | Postings from December 2004

[perl #32968] [PATCH] Re: B::walkoptree segfaults

Thread Previous | Thread Next
Stephen McCamant
December 28, 2004 10:14
[perl #32968] [PATCH] Re: B::walkoptree segfaults
Message ID:
>>>>> "AT" == Alexey Tourbin (via RT) writes:

AT> # New Ticket Created by  Alexey Tourbin 
AT> # Please include the string:  [perl #32968]
AT> # in the subject line of all future correspondence about this issue. 
AT> # <URL: >

AT> Hello,

AT> Recently I reported this bug on perl5-porters mailing list.
AT> Since there seems to be no immediate interest to this bug,
AT> I file it for future revision.  I assign severity=high
AT> because it makes perl crash with a simple expression
AT> given on a command line:

AT> $ perl -MO=Debug /usr/lib/perl5/ >/dev/null
AT> zsh: segmentation fault
AT> $ perl -MO=Bblock /usr/lib/perl5/
AT> zsh: segmentation fault
AT> $

AT> Detailed explanation and test cases are available via the
AT> following links:


AT> The problem seems to be somewhere around pmreplroot: Perl always
AT> expects pmreplroot to be a reference (or a pointer), while
AT> occasionally pmreplroot is a scalar (or an integer).

Try the following patch.

It appears there were two problems with walkoptree and pmreplroot:
first, there was a logic error in the previous addition of pmreplroot
support: the code was claiming that the pmreplroot kid of the OP was a
always PMOP, while in general it isn't: it's only the *parent* we know
to be a PMOP in this branch. This was causing a segfault on a s///.

The second problem was analogous to the change needed in
walkoptree_slow: when the OP is an OP_PUSHRE (the regex in a split()),
the replroot is sometimes reused to store a pointer to the array being
split into, either as a GV* or as a PADOFFSET under ithreads. In that
case, walkoptree shouldn't recurse. The second hunk adds a comment to
op.h mentioning this weirdness. As the comment implies, it would be
even better documentation to change the code to achieve the reuse with
a union rather than with casts, but maybe that's more work than it's

This is my first attempt at making a patch against the SVK-mirrored
Subversion repository; let me know if it doesn't apply right.

 -- Stephen

Index: ext/B/B.xs
--- ext/B/B.xs	(revision 18175)
+++ ext/B/B.xs	(working copy)
@@ -408,10 +408,10 @@ walkoptree(pTHX_ SV *opsv, char *method)
 	    walkoptree(aTHX_ opsv, method);
-    if (o && (cc_opclass(aTHX_ o) == OPc_PMOP)
+    if (o && (cc_opclass(aTHX_ o) == OPc_PMOP) && o->op_type != OP_PUSHRE
 	    && (kid = cPMOPo->op_pmreplroot))
-	sv_setiv(newSVrv(opsv, opclassnames[OPc_PMOP]), PTR2IV(kid));
+	sv_setiv(newSVrv(opsv, cc_opclassname(aTHX_ kid)), PTR2IV(kid));
 	walkoptree(aTHX_ opsv, method);
Index: op.h
--- op.h	(revision 18175)
+++ op.h	(working copy)
@@ -267,7 +267,7 @@ struct pmop {
     OP *	op_first;
     OP *	op_last;
-    OP *	op_pmreplroot;
+    OP *	op_pmreplroot; /* (type is really union {OP*,GV*,PADOFFSET}) */
     OP *	op_pmreplstart;
     PMOP *	op_pmnext;		/* list of all scanpats */

Thread Previous | Thread Next Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About