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

Opening references to glob copies #10607

Closed
p5pRT opened this issue Sep 5, 2010 · 9 comments
Closed

Opening references to glob copies #10607

p5pRT opened this issue Sep 5, 2010 · 9 comments

Comments

@p5pRT
Copy link

p5pRT commented Sep 5, 2010

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

Searchable as RT77684$

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2010

From @cpansprout

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

$ 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 10cea94, 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​: 9b47cdd
  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
Copy link
Author

p5pRT commented Sep 5, 2010

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 10cea94.

$ 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 10cea94, 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
Copy link
Author

p5pRT commented Sep 5, 2010

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
--- 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
Copy link
Author

p5pRT commented Sep 7, 2010

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 10cea94.

$ 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 10cea94, 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
Copy link
Author

p5pRT commented Sep 7, 2010

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

@p5pRT
Copy link
Author

p5pRT commented Sep 12, 2010

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 10cea94.

$ 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 10cea94, 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
Copy link
Author

p5pRT commented Sep 12, 2010

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
--- 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
Copy link
Author

p5pRT commented Sep 13, 2010

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 10cea94.

$ 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 10cea94, 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 526fd1b.

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2010

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

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