Opening references to glob copies

p5pRT commented 14 years ago

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

Searchable as RT77684$

p5pRT commented 14 years ago

From @cpansprout

This is a regression introduced in the past week by yours truly\, with change 10cea945821.

$ perl5.13.4 -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' b $ bleadperl -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' *main::foo $ ls GLOB(0x8017b8)

bleadperl behaves the same way a 5.8.x. It seems it was changed unintentionally in 5.10.0. In any case\, the 5.10/12 behaviour is less surprising and more useful.

Note that I am only talking about copies of globs here\, that is\, globs with the FAKE flag on. (That was why I said ‘real globs’ in the commit message for 10cea945821\, but I forgot regenerate the patch after updating my working copy.)

Flags: category=core severity=low

Site configuration information for perl 5.13.4:

Configured by sprout at Sun Aug 29 17:21:22 PDT 2010.

Summary of my perl5 (revision 5 version 13 subversion 4 patch v5.13.4-30-g9b47cdd) configuration: Snapshot of: 9b47cddefd9b4a322e6382c8979ceeb2c3ac25c9 Platform: osname=darwin\, osvers=10.4.0\, archname=darwin-thread-multi-2level uname='darwin pint.local 10.4.0 darwin kernel version 10.4.0: fri apr 23 18:28:53 pdt 2010; root:xnu-1504.7.4~1release_i386 i386 ' config_args='-de -Dusedevel -Duseithreads' hint=recommended\, useposix=true\, d_sigaction=define useithreads=define\, usemultiplicity=define useperlio=define\, d_sfio=undef\, uselargefiles=define\, usesocks=undef use64bitint=undef\, use64bitall=undef\, uselongdouble=undef usemymalloc=n\, bincompat5005=undef Compiler: cc='cc'\, ccflags ='-fno-common -DPERL_DARWIN -no-cpp-precomp -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'\, optimize='-O3'\, cppflags='-no-cpp-precomp -fno-common -DPERL_DARWIN -no-cpp-precomp -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion=''\, gccversion='4.2.1 (Apple Inc. build 5664)'\, gccosandvers='' intsize=4\, longsize=4\, ptrsize=4\, doublesize=8\, byteorder=1234 d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=16 ivtype='long'\, ivsize=4\, nvtype='double'\, nvsize=8\, Off_t='off_t'\, lseeksize=8 alignbytes=8\, prototype=define Linker and Libraries: ld='env MACOSX_DEPLOYMENT_TARGET=10.3 cc'\, ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /usr/lib libs=-ldbm -ldl -lm -lutil -lc perllibs=-ldl -lm -lutil -lc libc=/usr/lib/libc.dylib\, so=dylib\, useshrplib=false\, libperl=libperl.a gnulibc_version='' Dynamic Linking: dlsrc=dl_dlopen.xs\, dlext=bundle\, d_dlsymun=undef\, ccdlflags=' ' cccdlflags=' '\, lddlflags=' -bundle -undefined dynamic_lookup -L/usr/local/lib -fstack-protector'

Locally applied patches:

@INC for perl 5.13.4: /usr/local/lib/perl5/site_perl/5.13.4/darwin-thread-multi-2level /usr/local/lib/perl5/site_perl/5.13.4 /usr/local/lib/perl5/5.13.4/darwin-thread-multi-2level /usr/local/lib/perl5/5.13.4 /usr/local/lib/perl5/site_perl .

Environment for perl 5.13.4: DYLD_LIBRARY_PATH (unset) HOME=/Users/sprout LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/X11/bin:/usr/local/bin PERL_BADLANG (unset) SHELL=/bin/bash

p5pRT commented 14 years ago

From @cpansprout

On Sep 5\, 2010\, at 1:01 PM\, Father Chrysostomos wrote:

This is a regression introduced in the past week by yours truly\, with change 10cea945821.

$ perl5.13.4 -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' b $ bleadperl -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' *main::foo $ ls GLOB(0x8017b8)

bleadperl behaves the same way a 5.8.x. It seems it was changed unintentionally in 5.10.0. In any case\, the 5.10/12 behaviour is less surprising and more useful.

Note that I am only talking about copies of globs here\, that is\, globs with the FAKE flag on. (That was why I said ‘real globs’ in the commit message for 10cea945821\, but I forgot regenerate the patch after updating my working copy.)

Here is a patch to fix this. Note that this also affects \*$glob_copy\, which is controversial\, but at least I am only restoring the 5.10/12 behaviour in this case. (I think ideally that an explicit * should do a SvFAKE_off temporarily [effectively]\, but in this case the result has no practical application\, so it’s not worth the time.)

p5pRT commented 14 years ago

From @cpansprout

From: Father Chrysostomos \sprout@cpan\.org

[perl #77684] Restore the 5.10/12 behaviour of open $fh\, ">"\, \$glob_copy

This restores the perl 5.10/12 behaviour\, making open treat \$foo as a scalar reference if it is a glob copy (SvFAKE).

Inline Patch

```diff --- blead-fh-segv2.base/perlio.c 2010-09-02 06:17:59.000000000 -0700 +++ blead-fh-segv2/perlio.c 2010-09-02 08:25:35.000000000 -0700 @@ -1449,7 +1449,7 @@ PerlIO_layer_from_ref(pTHX_ SV *sv) /* * For any scalar type load the handler which is bundled with perl */ - if (SvTYPE(sv) < SVt_PVAV && !isGV_with_GP(sv)) { + if (SvTYPE(sv) < SVt_PVAV && (!isGV_with_GP(sv) || SvFAKE(sv))) { PerlIO_funcs *f = PerlIO_find_layer(aTHX_ STR_WITH_LEN("scalar"), 1); /* This isn't supposed to happen, since PerlIO::scalar is core, * but could happen anyway in smaller installs or with PAR */ diff -rup blead-fh-segv2.base/t/io/open.t blead-fh-segv2/t/io/open.t --- blead-fh-segv2.base/t/io/open.t 2010-09-02 06:17:59.000000000 -0700 +++ blead-fh-segv2/t/io/open.t 2010-09-02 06:22:53.000000000 -0700 @@ -10,7 +10,7 @@ $| = 1; use warnings; use Config; -plan tests => 110; +plan tests => 111; my $Perl = which_perl(); @@ -337,3 +337,13 @@ fresh_perl_is( ', 'ok', { stderr => 1 }, '[perl #77492]: open $fh, ">", \*glob causes SEGV'); + +# [perl #77684] Opening a reference to a glob copy. +{ + my $var = *STDOUT; + open my $fh, ">", \$var; + print $fh "hello"; + is $var, "hello", '[perl #77684]: open $fh, ">", \$glob_copy' + # when this fails, it leaves an extra file: + or unlink \*STDOUT; +} ```

p5pRT commented 14 years ago

From @rgarcia

On 5 September 2010 22:12\, Father Chrysostomos \sprout@cpan\.org wrote:

On Sep 5\, 2010\, at 1:01 PM\, Father Chrysostomos wrote:

This is a regression introduced in the past week by yours truly\, with change 10cea945821.

$ perl5.13.4 -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' b $ bleadperl -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' *main::foo $ ls GLOB(0x8017b8)

bleadperl behaves the same way a 5.8.x. It seems it was changed unintentionally in 5.10.0. In any case\, the 5.10/12 behaviour is less surprising and more useful.

Note that I am only talking about copies of globs here\, that is\, globs with the FAKE flag on. (That was why I said ‘real globs’ in the commit message for 10cea945821\, but I forgot regenerate the patch after updating my working copy.)

Here is a patch to fix this. Note that this also affects \*$glob_copy\, which is controversial\, but at least I am only restoring the 5.10/12 behaviour in this case. (I think ideally that an explicit * should do a SvFAKE_off temporarily [effectively]\, but in this case the result has no practical application\, so it’s not worth the time.)

Your patch does not work for me :

t$ ./perl harness io/open.t io/open.t .. 75/111 Assertion failed: (!isGV_with_GP(s->var))\, function PerlIOScalar_pushed\, file scalar.xs\, line 50. io/open.t .. Failed 1/111 subtests (less 1 skipped subtest: 109 okay)

Test Summary Report

io/open.t (Wstat: 6 Tests: 110 Failed: 0) Non-zero wait status: 6 Parse errors: Bad plan. You planned 111 tests but ran 110. Files=1\, Tests=110\, 2 wallclock secs ( 0.05 usr 0.00 sys + 0.06 cusr 0.05 csys = 0.16 CPU) Result: FAIL

p5pRT commented 14 years ago

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

p5pRT commented 14 years ago

From @cpansprout

On Sep 7\, 2010\, at 2:46 AM\, Rafael Garcia-Suarez wrote:

On 5 September 2010 22:12\, Father Chrysostomos \sprout@cpan\.org wrote:

On Sep 5\, 2010\, at 1:01 PM\, Father Chrysostomos wrote:

This is a regression introduced in the past week by yours truly\, with change 10cea945821.

$ perl5.13.4 -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' b $ bleadperl -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' *main::foo $ ls GLOB(0x8017b8)

bleadperl behaves the same way a 5.8.x. It seems it was changed unintentionally in 5.10.0. In any case\, the 5.10/12 behaviour is less surprising and more useful.

Note that I am only talking about copies of globs here\, that is\, globs with the FAKE flag on. (That was why I said ‘real globs’ in the commit message for 10cea945821\, but I forgot regenerate the patch after updating my working copy.)

Here is a patch to fix this. Note that this also affects \*$glob_copy\, which is controversial\, but at least I am only restoring the 5.10/12 behaviour in this case. (I think ideally that an explicit * should do a SvFAKE_off temporarily [effectively]\, but in this case the result has no practical application\, so it’s not worth the time.)

Your patch does not work for me :

t$ ./perl harness io/open.t io/open.t .. 75/111 Assertion failed: (!isGV_with_GP(s->var))\, function PerlIOScalar_pushed\, file scalar.xs\, line 50. io/open.t .. Failed 1/111 subtests (less 1 skipped subtest: 109 okay)

I tried building with -DDEBUGGING\, and I get the same assertion failure. If I remove the assertion from SvCUR\, it just works.

What::s happening is that PerlIO'scalar is doing SvCUR_set(s->var\, 0) where s->var is the glob. By setting SvCUR\, it::s actually wiping the GvFLAGS. But then the variable turns into a PV afterwards anyway\, so it works by accident.

This revised patch adds some sv_force_normals to ext/PerlIO_scalar/scalar.xs.

p5pRT commented 14 years ago

From @cpansprout

From: Father Chrysostomos \sprout@cpan\.org

[perl #77684] Restore the 5.10/12 behaviour of open $fh\, ">"\, \$glob_copy

This restores the perl 5.10/12 behaviour\, making open treat \$foo as a scalar reference if it is a glob copy (SvFAKE).

It also fixes an existing assertion failure that the test now trig- gers. PerlIOScalar_pushed was not downgrading the sv before set- ting SvCUR.

Inline Patch

```diff --- blead-fh-segv2.base/perlio.c 2010-09-02 06:17:59.000000000 -0700 +++ blead-fh-segv2/perlio.c 2010-09-02 08:25:35.000000000 -0700 @@ -1449,7 +1449,7 @@ PerlIO_layer_from_ref(pTHX_ SV *sv) /* * For any scalar type load the handler which is bundled with perl */ - if (SvTYPE(sv) < SVt_PVAV && !isGV_with_GP(sv)) { + if (SvTYPE(sv) < SVt_PVAV && (!isGV_with_GP(sv) || SvFAKE(sv))) { PerlIO_funcs *f = PerlIO_find_layer(aTHX_ STR_WITH_LEN("scalar"), 1); /* This isn't supposed to happen, since PerlIO::scalar is core, * but could happen anyway in smaller installs or with PAR */ diff -rup blead-fh-segv2.base/t/io/open.t blead-fh-segv2/t/io/open.t --- blead-fh-segv2.base/t/io/open.t 2010-09-02 06:17:59.000000000 -0700 +++ blead-fh-segv2/t/io/open.t 2010-09-02 06:22:53.000000000 -0700 @@ -10,7 +10,7 @@ $| = 1; use warnings; use Config; -plan tests => 110; +plan tests => 111; my $Perl = which_perl(); @@ -337,3 +337,13 @@ fresh_perl_is( ', 'ok', { stderr => 1 }, '[perl #77492]: open $fh, ">", \*glob causes SEGV'); + +# [perl #77684] Opening a reference to a glob copy. +{ + my $var = *STDOUT; + open my $fh, ">", \$var; + print $fh "hello"; + is $var, "hello", '[perl #77684]: open $fh, ">", \$glob_copy' + # when this fails, it leaves an extra file: + or unlink \*STDOUT; +} diff -rup blead-fh-segv2.base/ext/PerlIO-scalar/scalar.xs blead-fh-segv2/ext/PerlIO-scalar/scalar.xs --- blead-fh-segv2.base/ext/PerlIO-scalar/scalar.xs 2010-05-06 02:19:09.000000000 -0700 +++ blead-fh-segv2/ext/PerlIO-scalar/scalar.xs 2010-09-08 21:47:55.000000000 -0700 @@ -47,9 +47,15 @@ PerlIOScalar_pushed(pTHX_ PerlIO * f, co SvUPGRADE(s->var, SVt_PV); code = PerlIOBase_pushed(aTHX_ f, mode, Nullsv, tab); if (!SvOK(s->var) || (PerlIOBase(f)->flags) & PERLIO_F_TRUNCATE) + { + sv_force_normal(s->var); SvCUR_set(s->var, 0); + } if ((PerlIOBase(f)->flags) & PERLIO_F_APPEND) + { + sv_force_normal(s->var); s->posn = SvCUR(s->var); + } else s->posn = 0; SvSETMAGIC(s->var); @@ -166,6 +172,7 @@ PerlIOScalar_write(pTHX_ PerlIO * f, con SV *sv = s->var; char *dst; SvGETMAGIC(sv); + sv_force_normal(sv); if ((PerlIOBase(f)->flags) & PERLIO_F_APPEND) { dst = SvGROW(sv, SvCUR(sv) + count); offset = SvCUR(sv); ```

p5pRT commented 14 years ago

From @rgarcia

On 12 September 2010 21:19\, Father Chrysostomos \sprout@cpan\.org wrote:

On Sep 7\, 2010\, at 2:46 AM\, Rafael Garcia-Suarez wrote:

On 5 September 2010 22:12\, Father Chrysostomos \sprout@cpan\.org wrote:

On Sep 5\, 2010\, at 1:01 PM\, Father Chrysostomos wrote:

This is a regression introduced in the past week by yours truly\, with change 10cea945821.

$ perl5.13.4 -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' b $ bleadperl -le '$a = *foo; open $fh\, ">"\, \$a; print $fh\, "b"; print $a' *main::foo $ ls GLOB(0x8017b8)

bleadperl behaves the same way a 5.8.x. It seems it was changed unintentionally in 5.10.0. In any case\, the 5.10/12 behaviour is less surprising and more useful.

Note that I am only talking about copies of globs here\, that is\, globs with the FAKE flag on. (That was why I said ‘real globs’ in the commit message for 10cea945821\, but I forgot regenerate the patch after updating my working copy.)

Here is a patch to fix this. Note that this also affects \*$glob_copy\, which is controversial\, but at least I am only restoring the 5.10/12 behaviour in this case. (I think ideally that an explicit * should do a SvFAKE_off temporarily [effectively]\, but in this case the result has no practical application\, so it’s not worth the time.)

Your patch does not work for me :

t$ ./perl harness io/open.t io/open.t .. 75/111 Assertion failed: (!isGV_with_GP(s->var))\, function PerlIOScalar_pushed\, file scalar.xs\, line 50. io/open.t .. Failed 1/111 subtests (less 1 skipped subtest: 109 okay)

I tried building with -DDEBUGGING\, and I get the same assertion failure. If I remove the assertion from SvCUR\, it just works.

What::s happening is that PerlIO'scalar is doing SvCUR_set(s->var\, 0) where s->var is the glob. By setting SvCUR\, it::s actually wiping the GvFLAGS. But then the variable turns into a PV afterwards anyway\, so it works by accident.

This revised patch adds some sv_force_normals to ext/PerlIO_scalar/scalar.xs.

Thanks\, applied to bleadperl as change 526fd1b4d7270fff44588238f2411032c109da6e.

p5pRT commented 14 years ago

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

Perl / perl5