On Thu, Jun 30, 2005 at 06:25:20PM +0200, Rafael Garcia-Suarez wrote:
> > and indeed the breakage in bleed occurs there: the SV attached to
> > the const op has started getting the FAKE as well as READONLY flag set
> > (presumably its now a shared string), and when pp_push copies the SV and
> > pushes the copy, sv_setsv() leaves the copy fake+readonly too, which then
> > breaks when ISA element magic is attached to it.
>
> Right. And I don't really see a good way to fix it for now, except
> expanding on the sv.c patch I sent earlier. Allowing sv_magic to put
> magic on SvFAKE scalars wouldn't be a good idea. But allowing more
> types of magic can work. Opinions ?
Looking at it further: I think its kosher for __PACKAGE__ to return a
FAKE,READONLY constant; you get similar effects with hash keys:
$ ./perl -e '%h=qw(main,1); push @ISA, (keys %h)[0]'
Modification of a read-only value attempted at -e line 1.
The big difference between maint and bleed isn't so much that
__PACKAGE__ is now FAKE, but that in maint, sv_setsv()
on a FAKE,READONLY src SV triggers a copy and the dst is a plain SV; in
bleed it maintains it's COWness.
Later, in sv_magic() when attempting to attach magic, (assuming it's one
of the privileged magics as shown in your patch), then the magic is
attached, but first the SV is sv_upgrade()d to PVMG, which as a side
effect, triggers a copy. Which is why things like attaching pos magic
works okay:
$ ./perl -Ilib -MDevel::Peek -e '$x=__PACKAGE__; Dump $x; $x =~ /m/g; Dump $x'
SV = PV(0x8265a88) at 0x8280540
REFCNT = 1
FLAGS = (POK,FAKE,READONLY,pPOK)
PV = 0x827a5b4 "main"
CUR = 4
LEN = 0
SV = PVMG(0x827b3b8) at 0x8280540
REFCNT = 1
FLAGS = (SMG,POK,pPOK)
IV = 0
NV = 0
PV = 0x826d9a0 "main"\0
CUR = 4
LEN = 8
MAGIC = 0x8288e20
MG_VIRTUAL = &PL_vtbl_mglob
MG_TYPE = PERL_MAGIC_regex_global(g)
MG_LEN = 1
$
Notice that it's the same SV, but SvPVX has changed.
My provisional feeling is the correct fix is to expand the list of magics
that are exempt from PL_no_modify in sv_magic(); but I'm nervous that
this might also allow bad things (but can't think of any).
...
Thinking further, rather than allowing lots of extra magics, just
unconditionally allow anything that is FAKE,READONLY.
Seems to work...
Change 25032 by [EMAIL PROTECTED] on 2005/06/30 22:41:07
[perl #36434] assigning shared consts (eg __PACKAGE__) to magic vars
Affected files ...
... //depot/perl/sv.c#940 edit
... //depot/perl/t/op/magic.t#77 edit
Differences ...
==== //depot/perl/sv.c#940 (text) ====
@@ -4984,7 +4984,12 @@
sv_force_normal_flags(sv, 0);
#endif
if (SvREADONLY(sv)) {
- if (IN_PERL_RUNTIME
+ if (
+ /* its okay to attach magic to shared strings; the subsequent
+ * upgrade to PVMG will unshare the string */
+ !(SvFAKE(sv) && SvTYPE(sv) < SVt_PVMG)
+
+ && IN_PERL_RUNTIME
&& how != PERL_MAGIC_regex_global
&& how != PERL_MAGIC_bm
&& how != PERL_MAGIC_fm
==== //depot/perl/t/op/magic.t#77 (xtext) ====
@@ -36,7 +36,7 @@
return 1;
}
-print "1..56\n";
+print "1..57\n";
$Is_MSWin32 = $^O eq 'MSWin32';
$Is_NetWare = $^O eq 'NetWare';
@@ -432,9 +432,12 @@
local @ISA;
local %ENV;
eval { push @ISA, __PACKAGE__ };
- ok( $@ eq '', 'Push a constant on a magic array', '#36434' );
+ ok( $@ eq '', 'Push a constant on a magic array');
$@ and print "# $@";
eval { %ENV = (PATH => __PACKAGE__) };
- ok( $@ eq '', 'Assign a constant to a magic hash', '#36434' );
+ ok( $@ eq '', 'Assign a constant to a magic hash');
+ $@ and print "# $@";
+ eval { my %h = qw(A B); %ENV = (PATH => (keys %h)[0]) };
+ ok( $@ eq '', 'Assign a shared key to a magic hash');
$@ and print "# $@";
}
--
Thank God I'm an atheist.....
