Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blead perl breaks CPANPLUS-0.055 and other programs that push @ISA, __PACKAGE__ #7999

Closed
p5pRT opened this issue Jun 30, 2005 · 21 comments
Closed

Comments

@p5pRT
Copy link

p5pRT commented Jun 30, 2005

Migrated from rt.perl.org#36434 (status was 'resolved')

Searchable as RT36434$

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From david.dyck@fluke.com

Created by david.dyck@fluke.com

While trying to build
  CPANPLUS-0.055$
with a recent perl (5.9.3)

I got the followin error when processing
the command
  perl Makefile.PL
Modification of a read-only value attempted at /loader/0x815ae80/Term/UI.pm line 20, <> line 12.

The following 3 line test program results in the same error when pushing __PACKAGE__
onto @​ISA​:
  our @​ISA = qw(a b);
  push @​ISA, 'main';
  push @​ISA, __PACKAGE__;

"Modification of a read-only value attempted at isabug.pl line 3."

Renaming the @​ISA variable makes the error message go away.

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl vv5.9.3:

Configured by dcd at Tue Jun 28 09:14:20 PDT 2005.

Summary of my perl5 (revision 5 version 9 subversion 3 patch 25006) configuration:
  Platform:
    osname=linux, osvers=2.4.31, archname=i686-linux
    uname='linux dd 2.4.31 #1 wed jun 1 09:20:05 pdt 2005 i686 '
    config_args='-Accflags=-DPERL_DISABLE_PMC -Dmksymlinks -Dinstallusrbinperl -Uversiononly -Dusedevel -Doptimize=-O3 -g -de -Dcf_email=david.dyck@fluke.com'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=undef useithreads=undef usemultiplicity=undef
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=undef use64bitall=undef uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-DPERL_DISABLE_PMC -DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O3 -g',
    cppflags='-DPERL_DISABLE_PMC -DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include'
    ccversion='', gccversion='egcs-2.91.66.1 19990314/Linux (egcs-1.1.2 release)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=4
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lgdbm -ldbm -ldb -ldl -lm -lc
    perllibs=-ldl -lm -lc
    libc=/lib/libc.so.5.4.44, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fpic', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    


@INC for perl vv5.9.3:
    /usr/local/lib/perl5/5.9.3/i686-linux
    /usr/local/lib/perl5/5.9.3
    /usr/local/lib/perl5/site_perl/5.9.3/i686-linux
    /usr/local/lib/perl5/site_perl/5.9.3
    /usr/local/lib/perl5/site_perl/5.9.2/i686-linux
    /usr/local/lib/perl5/site_perl/5.9.2
    /usr/local/lib/perl5/site_perl
    .


Environment for perl vv5.9.3:
    HOME=/home/dcd
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/dcd/bin:/sbin:/usr/local/bin:/bin:/usr/bin:/usr/X11/bin:/usr/games:/usr/local/samba:/home/hobbes/tools/scripts:/home/hobbes/tools/linux:/usr0/hobbes/tools/scripts:/usr0/dcd/bin:/apps/general/bin:/usr/public
    PERL5_CPANPLUS_CONFIG=/home/dcd/.cpanplus/config
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From david.dyck@fluke.com

The following 3 line test program results in the same error when pushing __PACKAGE__
onto @​ISA​:
our @​ISA = qw(a b);
push @​ISA, 'main';
push @​ISA, __PACKAGE__;

"Modification of a read-only value attempted at isabug.pl line 3."

if I change the line
  push @​ISA, __PACKAGE__;
to
  push @​ISA, __PACKAGE__.'';

then the error message is also supressed!

At best the error message is confusing, as it seems that
by making the __PACKAGE__ into a temporary expression
when appending an empty string, that the error message
is refering to the read-only'ness of __PACKAGE__,
but it isn't apparent that __PACKAGE__ is being
modified.

anyone have any ideas what is really going on?

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @demerphq

On 6/30/05, David Dyck <david.dyck@​fluke.com> wrote​:

The following 3 line test program results in the same error when pushing __PACKAGE__
onto @​ISA​:
our @​ISA = qw(a b);
push @​ISA, 'main';
push @​ISA, __PACKAGE__;

"Modification of a read-only value attempted at isabug.pl line 3."

if I change the line
push @​ISA, __PACKAGE__;
to
push @​ISA, __PACKAGE__.'';

then the error message is also supressed!

Why would you do this anyway? What is the point of a package being ISA itself?

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @schwern

On Thu, Jun 30, 2005 at 10​:02​:27AM +0200, demerphq wrote​:

Why would you do this anyway? What is the point of a package being ISA itself?

Other way around.

  push @​Some​::Other​::Package​::ISA, __PACKAGE__;

Term​::UI does this.

  push @​Term​::ReadLine​::Stub​::ISA, __PACKAGE__
  unless grep { $_ eq __PACKAGE__ } @​Term​::ReadLine​::Stub​::ISA;

--
Michael G Schwern schwern@​pobox.com http​://www.pobox.com/~schwern
You are wicked and wrong to have broken inside and peeked at the
implementation and then relied upon it.
  -- tchrist in <31832.969261130@​chthon>

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @rgs

David Dyck (via RT) wrote​:

The following 3 line test program results in the same error when pushing __PACKAGE__
onto @​ISA​:
our @​ISA = qw(a b);
push @​ISA, 'main';
push @​ISA, __PACKAGE__;

"Modification of a read-only value attempted at isabug.pl line 3."

Renaming the @​ISA variable makes the error message go away.

I can reproduce this with other magic variables as well :

  $ ./perl -e '%ENV = (PATH => __PACKAGE__)'
  Modification of a read-only value attempted at -e line 1.

The patch below solves this immediate problem with @​ISA (similar fix
works also for %ENV et alii), but the question is, why is this patch
necessary for blead and not for maint ?

Index​: sv.c

--- sv.c (révision 5317)
+++ sv.c (copie de travail)
@​@​ -4990,6 +4990,7 @​@​
  && how != PERL_MAGIC_fm
  && how != PERL_MAGIC_sv
  && how != PERL_MAGIC_backref
+ && how != PERL_MAGIC_isaelem
  )
  {
  Perl_croak(aTHX_ PL_no_modify);

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From david.dyck@fluke.com

On Thu, 30 Jun 2005 at 15​:51 +0200, Rafael Garcia-Suarez wrote​:

David Dyck (via RT) wrote​:

The following 3 line test program results in the same error when pushing __PACKAGE__
onto @​ISA​:
our @​ISA = qw(a b);
push @​ISA, 'main';
push @​ISA, __PACKAGE__;

"Modification of a read-only value attempted at isabug.pl line 3."

Renaming the @​ISA variable makes the error message go away.

I can reproduce this with other magic variables as well :

$ ./perl -e '%ENV = (PATH => __PACKAGE__)'
Modification of a read-only value attempted at -e line 1.

Thanks for the patch.

Before I applied the patch, I tried another experiment
using B​::Deparse and was surprised to see that
all 3 "push" lines deparse the same.
If the line deparses the same, than why wast the
error message only issued for the "push @​ISA, __PACKAGE__" line?
(it was also interesting to see that eval{}'ing the offending
line (not shown) is another work-around that still modifys @​ISA
as desired.)

$ cat isabug.pl
our @​ISA = qw(a b);
push @​ISA, 'main';
push @​ISA, __PACKAGE__.'';
push @​ISA, __PACKAGE__;
print join("\t", @​ISA),"\n";

$ perl -MO=Deparse isabug.pl
our(@​ISA) = ('a', 'b');
push @​ISA, 'main';
push @​ISA, 'main';
push @​ISA, 'main';
print join("\t", @​ISA), "\n";
isabug.pl syntax OK

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From david.dyck@fluke.com

On Thu, 30 Jun 2005 at 08​:09 -0000, Michael G Schwern via RT...​:

On Thu, Jun 30, 2005 at 10​:02​:27AM +0200, demerphq wrote​:

Why would you do this anyway? What is the point of a package being ISA itself?

Other way around.

push @​Some​::Other​::Package​::ISA, __PACKAGE__;

Term​::UI does this.

push @​Term​::ReadLine​::Stub​::ISA, __PACKAGE__
unless grep { $_ eq __PACKAGE__ } @​Term​::ReadLine​::Stub​::ISA;

Michael points to the very line that highlighted the problem, as
that push to ISA resulted in the
  Modification of a read-only value attempted
error with blead perl. (my example was contrived, but not as simple
as it could have been.)

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @rgs

David Dyck wrote​:

On Thu, 30 Jun 2005 at 15​:51 +0200, Rafael Garcia-Suarez wrote​:

David Dyck (via RT) wrote​:

The following 3 line test program results in the same error when pushing __PACKAGE__
onto @​ISA​:
our @​ISA = qw(a b);
push @​ISA, 'main';
push @​ISA, __PACKAGE__;

"Modification of a read-only value attempted at isabug.pl line 3."

Renaming the @​ISA variable makes the error message go away.

I can reproduce this with other magic variables as well :

$ ./perl -e '%ENV = (PATH => __PACKAGE__)'
Modification of a read-only value attempted at -e line 1.

Thanks for the patch.

Before I applied the patch, I tried another experiment
using B​::Deparse and was surprised to see that
all 3 "push" lines deparse the same.
If the line deparses the same, than why wast the
error message only issued for the "push @​ISA, __PACKAGE__" line?

Because __PACKAGE__ is a constant. Use Devel​::Peek to peek at it.

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From david.dyck@fluke.com

On Thu, 30 Jun 2005 at 16​:42 +0200, Rafael Garcia-Suarez <rgarciasuarez@​man...​:

From​: Rafael Garcia-Suarez <rgarciasuarez@​mandriva.com>

If the line deparses the same, than why was the
error message only issued for the "push @​ISA, __PACKAGE__" line?

Because __PACKAGE__ is a constant. Use Devel​::Peek to peek at it.

Thanks for the pointer to Devel​::Peek, I don't use it enough...

$ perl -e 'use Devel​::Peek; Dump( __PACKAGE__);'
SV = PV(0x8153890) at 0x8164118
  REFCNT = 1
  FLAGS = (POK,FAKE,READONLY,pPOK)
  PV = 0x815be8c "main"
  CUR = 4
  LEN = 0

$ perl -e 'use Devel​::Peek; Dump( 'main' );'
SV = PV(0x8153890) at 0x8164110
  REFCNT = 1
  FLAGS = (POK,READONLY,pPOK)
  PV = 0x81638e8 "main"\0
  CUR = 4
  LEN = 8

I guess __PACKAGE__ is one of those constants that O​::Deparse
can't regenerate the correct source for, right? Seems like
they are both READONLY but __PACKAGE__ is "FAKE".

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @JohnPeacock

David Dyck wrote​:

I guess __PACKAGE__ is one of those constants that O​::Deparse
can't regenerate the correct source for, right? Seems like
they are both READONLY but __PACKAGE__ is "FAKE".

There's nothing to regenerate; the tokenizer has already eaten the
original contents and replaced it with PL_curstash​:

...toke.c​:4350 or so...

    case KEY\_\_\_PACKAGE\_\_&#8203;:
        yylval\.opval = \(OP\*\)newSVOP\(OP\_CONST\, 0\,
                                    \(PL\_curstash
                                     ? newSVhek\(HvNAME\_HEK\(PL\_curstash\)\)
                                     : &PL\_sv\_undef\)\);
        TERM\(THING\);

John

p.s. is this _really_ necessary​:

      if \(name\[1\] == '\_' &&
          name\[2\] == 'P' &&
          name\[3\] == 'A' &&
          name\[4\] == 'C' &&
          name\[5\] == 'K' &&
          name\[6\] == 'A' &&
          name\[7\] == 'G' &&
          name\[8\] == 'E' &&
          name\[9\] == '\_' &&
          name\[10\] == '\_'\)
      \{                                       /\* \_\_PACKAGE\_\_ \*/
        return \-KEY\_\_\_PACKAGE\_\_;
      \}

i.e. isn't strEQ going to shortcircuit just as fast as individual char
tests???

--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @iabyn

On Thu, Jun 30, 2005 at 11​:43​:33AM -0400, John Peacock wrote​:

David Dyck wrote​:
There's nothing to regenerate; the tokenizer has already eaten the
original contents and replaced it with PL_curstash​:

...toke.c​:4350 or so...

   case KEY\_\_\_PACKAGE\_\_&#8203;:
       yylval\.opval = \(OP\*\)newSVOP\(OP\_CONST\, 0\,
                                   \(PL\_curstash
                                    ? 
                                    newSVhek\(HvNAME\_HEK\(PL\_curstash\)\)
                                    : &PL\_sv\_undef\)\);
       TERM\(THING\);

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.

--
The perl5 internals are a complete mess. It's like Jenga - to get the
perl5 tower taller and do something new you select a block somewhere in
the middle, with trepidation pull it out slowly, and then carefully
balance it somewhere new, hoping the whole edifice won't collapse as a
result.
  - Nicholas Clark, based on an original by Simon Cozens.

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @rgs

Dave Mitchell wrote​:

On Thu, Jun 30, 2005 at 11​:43​:33AM -0400, John Peacock wrote​:

David Dyck wrote​:
There's nothing to regenerate; the tokenizer has already eaten the
original contents and replaced it with PL_curstash​:

...toke.c​:4350 or so...

   case KEY\_\_\_PACKAGE\_\_&#8203;:
       yylval\.opval = \(OP\*\)newSVOP\(OP\_CONST\, 0\,
                                   \(PL\_curstash
                                    ? 
                                    newSVhek\(HvNAME\_HEK\(PL\_curstash\)\)
                                    : &PL\_sv\_undef\)\);
       TERM\(THING\);

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 ?

For the record, here's a regression test :

Index​: t/op/magic.t

--- t/op/magic.t (révision 5317)
+++ t/op/magic.t (copie de travail)
@​@​ -36,7 +36,7 @​@​
  return 1;
}

-print "1..54\n";
+print "1..56\n";

$Is_MSWin32 = $^O eq 'MSWin32';
$Is_NetWare = $^O eq 'NetWare';
@​@​ -426,3 +426,15 @​@​
  my @​y = f();
  ok( $x eq "@​y", "return a magic array ($x) vs (@​y)" );
}
+
+# Test for bug [perl #36434]
+{
+ local @​ISA;
+ local %ENV;
+ eval { push @​ISA, __PACKAGE__ };
+ ok( $@​ eq '', 'Push a constant on a magic array' );
+ $@​ and print "# $@​";
+ eval { %ENV = (PATH => __PACKAGE__) };
+ ok( $@​ eq '', 'Assign a constant to a magic hash' );
+ $@​ and print "# $@​";
+}

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @ysth

On Thu, Jun 30, 2005 at 11​:43​:33AM -0400, John Peacock wrote​:

p.s. is this _really_ necessary​:

     if \(name\[1\] == '\_' &&
         name\[2\] == 'P' &&
         name\[3\] == 'A' &&
         name\[4\] == 'C' &&
         name\[5\] == 'K' &&
         name\[6\] == 'A' &&
         name\[7\] == 'G' &&
         name\[8\] == 'E' &&
         name\[9\] == '\_' &&
         name\[10\] == '\_'\)
     \{                                       /\* \_\_PACKAGE\_\_ \*/
       return \-KEY\_\_\_PACKAGE\_\_;
     \}

i.e. isn't strEQ going to shortcircuit just as fast as individual char
tests???

That code is autogenerated; deciding when having perl_keywords.pl use
strEQ would make sense isn't worth the bother IMO.

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @JohnPeacock

Yitzchak Scott-Thoennes wrote​:

That code is autogenerated; deciding when having perl_keywords.pl use
strEQ would make sense isn't worth the bother IMO.

OK, that makes sense. I thought someone went through the trouble to
"optimize" that and it didn't seem worthwhile. It should be possible to
enhance Devel​::Tokenizer​::C to be brighter​: as soon as you get past a
certain length of the token (TBD) and it is otherwise unique, use strEQ,
else use the individual character comparison.

But as you said, it probably isn't worth the trouble (premature
optimization in the other direction)...

John

--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @schwern

On Thu, Jun 30, 2005 at 11​:43​:33AM -0400, John Peacock wrote​:

p.s. is this _really_ necessary​:

     if \(name\[1\] == '\_' &&
         name\[2\] == 'P' &&
         name\[3\] == 'A' &&
         name\[4\] == 'C' &&
         name\[5\] == 'K' &&
         name\[6\] == 'A' &&
         name\[7\] == 'G' &&
         name\[8\] == 'E' &&
         name\[9\] == '\_' &&
         name\[10\] == '\_'\)
     \{                                       /\* \_\_PACKAGE\_\_ \*/
       return \-KEY\_\_\_PACKAGE\_\_;
     \}

Kinda makes me think of those tall, vertical neon signs outside hotels where
each letter is 5 feet high.

--
Michael G Schwern schwern@​pobox.com http​://www.pobox.com/~schwern
Don't try the paranormal until you know what's normal.
  -- "Lords and Ladies" by Terry Prachett

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @nwc10

On Thu, Jun 30, 2005 at 11​:43​:33AM -0400, John Peacock wrote​:

p.s. is this _really_ necessary​:

     if \(name\[1\] == '\_' &&
         name\[2\] == 'P' &&
         name\[3\] == 'A' &&
         name\[4\] == 'C' &&
         name\[5\] == 'K' &&
         name\[6\] == 'A' &&
         name\[7\] == 'G' &&
         name\[8\] == 'E' &&
         name\[9\] == '\_' &&
         name\[10\] == '\_'\)
     \{                                       /\* \_\_PACKAGE\_\_ \*/
       return \-KEY\_\_\_PACKAGE\_\_;
     \}

i.e. isn't strEQ going to shortcircuit just as fast as individual char
tests???

We benchmarked it and the code generated by the current perl_keyword.pl was
the fastest. If you (or anyone else) are (is) curious, it would be
interesting to see what effect tweaking Devel​::Tokenizer​::C to use strEQ
and then regenerating gives.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @JohnPeacock

Nicholas Clark wrote​:

We benchmarked it and the code generated by the current perl_keyword.pl was
the fastest. If you (or anyone else) are (is) curious, it would be
interesting to see what effect tweaking Devel​::Tokenizer​::C to use strEQ
and then regenerating gives.

No, I believe you that it is faster (if only because it saves a function
call and all of the associated thrashing that can happen). It just
looked weird; now that I know that it is 100% machine generated, I don't
have any problem with it as it stands.

John

--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748

@p5pRT
Copy link
Author

p5pRT commented Jun 30, 2005

From @iabyn

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 davem@​davem-splatty 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.....

@p5pRT p5pRT closed this as completed Jul 1, 2005
@p5pRT
Copy link
Author

p5pRT commented Jul 1, 2005

@rgs - Status changed from 'open' to 'resolved'

@p5pRT
Copy link
Author

p5pRT commented Jul 1, 2005

From @rgs

Dave Mitchell wrote​:

Thinking further, rather than allowing lots of extra magics, just
unconditionally allow anything that is FAKE,READONLY.

Since a copy takes place, sounds OK to me.

--
Fix in six / Fix in six / You will need a new remix

@jkeenan jkeenan changed the title blead perl breask CPANPLUS-0.055 and other programs that push @ISA, __PACKAGE__ blead perl breaks CPANPLUS-0.055 and other programs that push @ISA, __PACKAGE__ Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant