string eval turns off readonlyness on lexicals

[p5pRT]

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

Searchable as RT19135$

[p5pRT]

From @nwc10

Created by @nwc10

Without ithreads, a lexical stays readonly when it is referenced inside
a string eval​:

$ perl5.8.0-32 -MDevel​::Peek -wle 'my $k = "!"; Internals​::SvREADONLY $k, 1; Dump $k; eval "print \$k"; Dump $k'
SV = PV(0x8112460) at 0x8112210
  REFCNT = 1
  FLAGS = (PADBUSY,PADMY,POK,READONLY,pPOK)
  PV = 0x8111120 "!"\0
  CUR = 1
  LEN = 2
!
SV = PV(0x8112460) at 0x8112210
  REFCNT = 1
  FLAGS = (PADBUSY,PADMY,POK,READONLY,pPOK)
  PV = 0x8111120 "!"\0
  CUR = 1
  LEN = 2

With ithreads it loses its readonlyness​:

perl5.8.0-32-i -MDevel​::Peek -wle 'my $k = "!"; Inter
nals​::SvREADONLY $k, 1; Dump $k; eval "print \$k"; Dump $k'
SV = PV(0x8125c60) at 0x8125a28
  REFCNT = 1
  FLAGS = (PADBUSY,PADMY,POK,READONLY,pPOK)
  PV = 0x8137120 "!"\0
  CUR = 1
  LEN = 2
!
SV = PV(0x8125c60) at 0x8125a28
  REFCNT = 1
  FLAGS = (PADBUSY,PADMY,POK,pPOK)
  PV = 0x8137120 "!"\0
  CUR = 1
  LEN = 2

This is still true in blead. It seems wrong. (It's also the source of a
copy on write bug in blead)

I think it's the code at the line /* could be a freed constant */ in
Perl_pad_free (formerly of op.c, now in pad.c) that's responsible for this.

Perl Info

Flags:
    category=core
    severity=medium

This perlbug was built using Perl v5.8.0 - Sat Jul 13 15:53:56 BST 2002
It is being executed now by  Perl v5.8.0 - Fri Jul 19 22:00:42 BST 2002.

Site configuration information for perl v5.8.0:

Configured by nick at Fri Jul 19 22:00:42 BST 2002.

Summary of my perl5 (revision 5.0 version 8 subversion 0) configuration:
  Platform:
    osname=linux, osvers=2.4.18-rmk7, archname=armv4l-linux
    uname='linux bagpuss.unfortu.net 2.4.18-rmk7 #10 sun jun 23 21:43:05 bst 2002 armv4l unknown '
    config_args='-Dcc=ccache gcc-3.0 -Dld=gcc-3.0 -Ubincompat5005 -Uinstallusrbinperl -Dcf_email=nick@ccl4.org -Dperladmin=nick@ccl4.org -Dinc_version_list=  -Dinc_version_list_init=0 -Doptimize=-Os -de -Uusethreads'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=undef use5005threads=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='ccache gcc-3.0', ccflags ='-fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-Os',
    cppflags='-fno-strict-aliasing -I/usr/local/include'
    ccversion='', gccversion='3.0.4', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=8
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='gcc-3.0', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -lgdbm -ldbm -ldb -ldl -lm -lc -lcrypt -lutil
    perllibs=-lnsl -ldl -lm -lc -lcrypt -lutil
    libc=/lib/libc-2.2.4.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.2.4'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic'
    cccdlflags='-fpic', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    DEVEL17513


@INC for perl v5.8.0:
    /usr/local/lib/perl5/5.8.0/armv4l-linux
    /usr/local/lib/perl5/5.8.0
    /usr/local/lib/perl5/site_perl/5.8.0/armv4l-linux
    /usr/local/lib/perl5/site_perl/5.8.0
    /usr/local/lib/perl5/site_perl
    .


Environment for perl v5.8.0:
    HOME=/home/nick
    LANG (unset)
    LANGUAGE (unset)
    LC_CTYPE=en_GB.ISO-8859-1
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/nick/bin:/usr/local/bin:/usr/bin:/bin:/usr/bin/X11:/usr/games:/sbin:/usr/sbin:/usr/local/sbin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

[p5pRT]

From @schwern

Confirmed to still exist in 5.8.6 and blead.

[p5pRT]

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

[p5pRT]

From @chorny

Same result on 5.12.0 with ithreads.

On Sat Dec 14 11​:15​:57 2002, nicholas wrote​:

Without ithreads, a lexical stays readonly when it is referenced
inside
a string eval​:

$ perl5.8.0-32 -MDevel​::Peek -wle 'my $k = "!"; Internals​::SvREADONLY
$k, 1; Dump $k; eval "print \$k"; Dump $k'
SV = PV(0x8112460) at 0x8112210
REFCNT = 1
FLAGS = (PADBUSY,PADMY,POK,READONLY,pPOK)
PV = 0x8111120 "!"\0
CUR = 1
LEN = 2
!
SV = PV(0x8112460) at 0x8112210
REFCNT = 1
FLAGS = (PADBUSY,PADMY,POK,READONLY,pPOK)
PV = 0x8111120 "!"\0
CUR = 1
LEN = 2

With ithreads it loses its readonlyness​:

perl5.8.0-32-i -MDevel​::Peek -wle 'my $k = "!"; Internals​::SvREADONLY
$k, 1; Dump $k; eval "print \$k"; Dump $k'
SV = PV(0x8125c60) at 0x8125a28
REFCNT = 1
FLAGS = (PADBUSY,PADMY,POK,READONLY,pPOK)
PV = 0x8137120 "!"\0
CUR = 1
LEN = 2
!
SV = PV(0x8125c60) at 0x8125a28
REFCNT = 1
FLAGS = (PADBUSY,PADMY,POK,pPOK)
PV = 0x8137120 "!"\0
CUR = 1
LEN = 2

This is still true in blead. It seems wrong. (It's also the source of
a copy on write bug in blead)

I think it's the code at the line /* could be a freed constant */ in
Perl_pad_free (formerly of op.c, now in pad.c) that's responsible for
this.

--
Alexandr Ciornii, http​://chorny.net

[p5pRT]

From [Unknown Contact. See original ticket]

Same result on 5.12.0 with ithreads.

On Sat Dec 14 11​:15​:57 2002, nicholas wrote​:

Without ithreads, a lexical stays readonly when it is referenced
inside
a string eval​:

$ perl5.8.0-32 -MDevel​::Peek -wle 'my $k = "!"; Internals​::SvREADONLY
$k, 1; Dump $k; eval "print \$k"; Dump $k'
SV = PV(0x8112460) at 0x8112210
REFCNT = 1
FLAGS = (PADBUSY,PADMY,POK,READONLY,pPOK)
PV = 0x8111120 "!"\0
CUR = 1
LEN = 2
!
SV = PV(0x8112460) at 0x8112210
REFCNT = 1
FLAGS = (PADBUSY,PADMY,POK,READONLY,pPOK)
PV = 0x8111120 "!"\0
CUR = 1
LEN = 2

With ithreads it loses its readonlyness​:

perl5.8.0-32-i -MDevel​::Peek -wle 'my $k = "!"; Internals​::SvREADONLY
$k, 1; Dump $k; eval "print \$k"; Dump $k'
SV = PV(0x8125c60) at 0x8125a28
REFCNT = 1
FLAGS = (PADBUSY,PADMY,POK,READONLY,pPOK)
PV = 0x8137120 "!"\0
CUR = 1
LEN = 2
!
SV = PV(0x8125c60) at 0x8125a28
REFCNT = 1
FLAGS = (PADBUSY,PADMY,POK,pPOK)
PV = 0x8137120 "!"\0
CUR = 1
LEN = 2

This is still true in blead. It seems wrong. (It's also the source of
a copy on write bug in blead)

I think it's the code at the line /* could be a freed constant */ in
Perl_pad_free (formerly of op.c, now in pad.c) that's responsible for
this.

--
Alexandr Ciornii, http​://chorny.net

[p5pRT]

From @cpansprout

The readonliness is turned off explicitly in pad_free under ithreads. See also bug #19022, which resulted from the same change. There is some discussion there, too, but nobody seemed to know exactly why the readonliness needed to be turned off.

Change 4761/2aa1bed, from January of 2000, added that SvREADONLY_off. It is supposed to make sure that pad entries that were constants will not be constants the next time they are used.

My question is​: When the addresses of those scalars are reused, won’t the scalars be wiped clean? (In case it’s not obvious, I have very little idea how pads actually work.)

I tried simply deleting that SvREADONLY_off, and all tests pass (see the first attached patch).

Then I thought that maybe I should check that the SV is freed, and only then do SvREADONLY_off, to preserve the original intent of change 4761/2aa1bed. That worked too.

So, here are two patches. Take your pick.

[p5pRT]

From @cpansprout

From​: Father Chrysostomos <sprout@​cpan.org>

[perl #19135] string eval turns off readonlyness on lexicals

Don’t turn off readonliness on lexicals when freeing pad entries.

Inline Patch

diff -up blead/pad.c blead-19135-readonly-lexicals/pad.c
--- blead/pad.c	2010-09-12 04:32:10.000000000 -0700
+++ blead-19135-readonly-lexicals/pad.c	2010-09-14 16:13:03.000000000 -0700
@@ -1386,11 +1386,6 @@ Perl_pad_free(pTHX_ PADOFFSET po)
 
     if (PL_curpad[po] && PL_curpad[po] != &PL_sv_undef) {
 	SvPADTMP_off(PL_curpad[po]);
-#ifdef USE_ITHREADS
-	/* SV could be a shared hash key (eg bugid #19022) */
-	if (!SvIsCOW(PL_curpad[po]))
-	    SvREADONLY_off(PL_curpad[po]);	/* could be a freed constant */
-#endif
     }
     if ((I32)po < PL_padix)
 	PL_padix = po - 1;
diff -rup blead/t/op/eval.t blead-19135-readonly-lexicals/t/op/eval.t
--- blead/t/op/eval.t	2010-07-03 08:47:11.000000000 -0700
+++ blead-19135-readonly-lexicals/t/op/eval.t	2010-09-14 16:23:18.000000000 -0700
@@ -6,7 +6,7 @@ BEGIN {
     require './test.pl';
 }
 
-print "1..107\n";
+print "1..108\n";
 
 eval 'print "ok 1\n";';
 
@@ -611,3 +611,10 @@ eval $ov;
 print "ok\n";
 EOP
 
+{
+ my $k = "!";
+ Internals::SvREADONLY $k, 1;
+ eval 'my $do_something_with = $k';
+ eval { $k = 'mon' };
+ is $k, "!", "string eval leaves readonly lexicals readonly [perl #19135]";
+}

[p5pRT]

From @cpansprout

From​: Father Chrysostomos <sprout@​cpan.org>

[perl #19135] string eval turns off readonlyness on lexicals

Don’t turn of readonliness on lexicals when freeing pad entries,
unless they are left over from freed constants.

Inline Patch

diff -up blead/pad.c blead-19135-readonly-lexicals/pad.c
--- blead/pad.c	2010-09-12 04:32:10.000000000 -0700
+++ blead-19135-readonly-lexicals/pad.c	2010-09-14 16:13:59.000000000 -0700
@@ -1387,8 +1387,7 @@ Perl_pad_free(pTHX_ PADOFFSET po)
     if (PL_curpad[po] && PL_curpad[po] != &PL_sv_undef) {
 	SvPADTMP_off(PL_curpad[po]);
 #ifdef USE_ITHREADS
-	/* SV could be a shared hash key (eg bugid #19022) */
-	if (!SvIsCOW(PL_curpad[po]))
+	if (SvIS_FREED(PL_curpad[po]))
 	    SvREADONLY_off(PL_curpad[po]);	/* could be a freed constant */
 #endif
     }
diff -rup blead/t/op/eval.t blead-19135-readonly-lexicals/t/op/eval.t
--- blead/t/op/eval.t	2010-07-03 08:47:11.000000000 -0700
+++ blead-19135-readonly-lexicals/t/op/eval.t	2010-09-14 16:23:18.000000000 -0700
@@ -6,7 +6,7 @@ BEGIN {
     require './test.pl';
 }
 
-print "1..107\n";
+print "1..108\n";
 
 eval 'print "ok 1\n";';
 
@@ -611,3 +611,10 @@ eval $ov;
 print "ok\n";
 EOP
 
+{
+ my $k = "!";
+ Internals::SvREADONLY $k, 1;
+ eval 'my $do_something_with = $k';
+ eval { $k = 'mon' };
+ is $k, "!", "string eval leaves readonly lexicals readonly [perl #19135]";
+}

[p5pRT]

From @cpansprout

On Sun Sep 19 12​:10​:11 2010, sprout wrote​:

The readonliness is turned off explicitly in pad_free under ithreads.
See also bug #19022, which resulted from the same change. There is
some discussion there, too, but nobody seemed to know exactly why
the readonliness needed to be turned off.

Change 4761/2aa1bed, from January of 2000, added that SvREADONLY_off.
It is supposed to make sure that pad entries that were constants
will not be constants the next time they are used.

My question is​: When the addresses of those scalars are reused, won’t
the scalars be wiped clean? (In case it’s not obvious, I have very
little idea how pads actually work.)

I tried simply deleting that SvREADONLY_off, and all tests pass (see
the first attached patch).

Then I thought that maybe I should check that the SV is freed, and
only then do SvREADONLY_off, to preserve the original intent of
change 4761/2aa1bed. That worked too.

So, here are two patches. Take your pick.

Has anyone in the know had a chance to review these yet?

[p5pRT]

From @cpansprout

On Sun Sep 26 12​:46​:49 2010, sprout wrote​:

On Sun Sep 19 12​:10​:11 2010, sprout wrote​:

The readonliness is turned off explicitly in pad_free under ithreads.
See also bug #19022, which resulted from the same change. There is
some discussion there, too, but nobody seemed to know exactly why
the readonliness needed to be turned off.

Change 4761/2aa1bed, from January of 2000, added that SvREADONLY_off.
It is supposed to make sure that pad entries that were constants
will not be constants the next time they are used.

My question is​: When the addresses of those scalars are reused, won’t
the scalars be wiped clean? (In case it’s not obvious, I have very
little idea how pads actually work.)

I tried simply deleting that SvREADONLY_off, and all tests pass (see
the first attached patch).

Then I thought that maybe I should check that the SV is freed, and
only then do SvREADONLY_off, to preserve the original intent of
change 4761/2aa1bed. That worked too.

So, here are two patches. Take your pick.

Has anyone in the know had a chance to review these yet?

Well...? :-)

Here is a better test case, with no Internals​:: stuff​:

$ perl5.13.5 -wle 'for my $k(!0) { eval "\$k"; eval { $k = 7 }; print
"a"=~/a/ }'
7

[p5pRT]

From [Unknown Contact. See original ticket]

On Sun Sep 26 12​:46​:49 2010, sprout wrote​:

On Sun Sep 19 12​:10​:11 2010, sprout wrote​:

The readonliness is turned off explicitly in pad_free under ithreads.
See also bug #19022, which resulted from the same change. There is
some discussion there, too, but nobody seemed to know exactly why
the readonliness needed to be turned off.

Change 4761/2aa1bed, from January of 2000, added that SvREADONLY_off.
It is supposed to make sure that pad entries that were constants
will not be constants the next time they are used.

My question is​: When the addresses of those scalars are reused, won’t
the scalars be wiped clean? (In case it’s not obvious, I have very
little idea how pads actually work.)

I tried simply deleting that SvREADONLY_off, and all tests pass (see
the first attached patch).

Then I thought that maybe I should check that the SV is freed, and
only then do SvREADONLY_off, to preserve the original intent of
change 4761/2aa1bed. That worked too.

So, here are two patches. Take your pick.

Has anyone in the know had a chance to review these yet?

Well...? :-)

Here is a better test case, with no Internals​:: stuff​:

$ perl5.13.5 -wle 'for my $k(!0) { eval "\$k"; eval { $k = 7 }; print
"a"=~/a/ }'
7

[p5pRT]

From @iabyn

On Sun, Nov 28, 2010 at 01​:39​:18PM -0800, Father Chrysostomos via RT wrote​:

On Sun Sep 26 12​:46​:49 2010, sprout wrote​:

On Sun Sep 19 12​:10​:11 2010, sprout wrote​:

The readonliness is turned off explicitly in pad_free under ithreads.
See also bug #19022, which resulted from the same change. There is
some discussion there, too, but nobody seemed to know exactly why
the readonliness needed to be turned off.

Change 4761/2aa1bed, from January of 2000, added that SvREADONLY_off.
It is supposed to make sure that pad entries that were constants
will not be constants the next time they are used.

My question is​: When the addresses of those scalars are reused, won’t
the scalars be wiped clean? (In case it’s not obvious, I have very
little idea how pads actually work.)

I tried simply deleting that SvREADONLY_off, and all tests pass (see
the first attached patch).

Then I thought that maybe I should check that the SV is freed, and
only then do SvREADONLY_off, to preserve the original intent of
change 4761/2aa1bed. That worked too.

So, here are two patches. Take your pick.

Has anyone in the know had a chance to review these yet?

Well...? :-)

Here is a better test case, with no Internals​:: stuff​:

$ perl5.13.5 -wle 'for my $k(!0) { eval "\$k"; eval { $k = 7 }; print
"a"=~/a/ }'
7

I think the first fix is correct (just removing the SvREADONLY_off).
The issue it was trying to fix appears to have been properly fixed later
by 3b1c21f, which is why it's safe to
remove it.

The second fix is wrong, in that the SVs in the pad aren't freed in the
RC==0 sense, but instead are left available for new ops (so when during
compiling, an op and its targ are optimised away, that slot (and the
same SV) will get reused). The RO flag is part of the criteria to decide
whether a pad slot is available ("free") for re-use (IS_PADCONST).

--
Thank God I'm an atheist.....

[p5pRT]

From @cpansprout

On Mon Dec 06 07​:40​:09 2010, davem wrote​:

On Sun, Nov 28, 2010 at 01​:39​:18PM -0800, Father Chrysostomos via RT
wrote​:

On Sun Sep 26 12​:46​:49 2010, sprout wrote​:

On Sun Sep 19 12​:10​:11 2010, sprout wrote​:

The readonliness is turned off explicitly in pad_free under
ithreads.
See also bug #19022, which resulted from the same change.
There is
some discussion there, too, but nobody seemed to know exactly why
the readonliness needed to be turned off.

Change 4761/2aa1bed, from January of 2000, added that
SvREADONLY_off.
It is supposed to make sure that pad entries that were constants
will not be constants the next time they are used.

My question is​: When the addresses of those scalars are reused,
won’t
the scalars be wiped clean? (In case it’s not obvious, I have
very
little idea how pads actually work.)

I tried simply deleting that SvREADONLY_off, and all tests pass (see
the first attached patch).

Then I thought that maybe I should check that the SV is freed, and
only then do SvREADONLY_off, to preserve the original intent of
change 4761/2aa1bed. That worked too.

So, here are two patches. Take your pick.

Has anyone in the know had a chance to review these yet?

Well...? :-)

Here is a better test case, with no Internals​:: stuff​:

$ perl5.13.5 -wle 'for my $k(!0) { eval "\$k"; eval { $k = 7 }; print
"a"=~/a/ }'
7

I think the first fix is correct (just removing the SvREADONLY_off).

I’ve just applied a slightly modified version as 3aadd5c.

[p5pRT]

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