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

Filehandle opened from ref to ref hangs on reading #13213

Closed
p5pRT opened this issue Aug 30, 2013 · 11 comments
Closed

Filehandle opened from ref to ref hangs on reading #13213

p5pRT opened this issue Aug 30, 2013 · 11 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Aug 30, 2013

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

Searchable as RT119529$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 30, 2013

From @epa

Created by @epa

This causes perl to hang chewing CPU​:

  my $x = \42;
  open my $fh, "<", \$x;
  my $got = <$fh>;

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.16.3:

Configured by Red Hat, Inc. at Fri May  3 12:10:03 UTC 2013.

Summary of my perl5 (revision 5 version 16 subversion 3) configuration:
   
  Platform:
    osname=linux, osvers=2.6.32-358.2.1.el6.x86_64, archname=x86_64-linux-thread-multi
    uname='linux buildvm-24.phx2.fedoraproject.org 2.6.32-358.2.1.el6.x86_64 #1 smp wed feb 20 12:17:37 est 2013 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Doptimize=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic -Dccdlflags=-Wl,--enable-new-dtags -Dlddlflags=-shared -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic -Wl,-z,relro  -DDEBUGGING=-g -Dversion=5.16.3 -Dmyhostname=localhost -Dperladmin=root@localhost -Dcc=gcc -Dcf_by=Red Hat, Inc. -Dprefix=/usr -Dvendorprefix=/usr -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl5 -Dsitearch=/usr/local/lib64/perl5 -Dprivlib=/usr/share/perl5 -Dvendorlib=/usr/share/perl5/vendor_perl -Darchlib=/usr/lib64/perl5 -Dvendorarch=/usr/lib64/perl5/vendor_perl -Darchname=x86_64-linux-thread-multi -Dlibpth=/usr/local/lib64 /lib64 /usr/lib64 -Duseshrplib -Dusethreads -Duseithreads -Dusedtrace=/usr/bin/dtrace -Duselargefiles -Dd_semctl_semun -Di_db -Ui_ndbm -Di_gdbm -Di_shadow -Di_syslog -Dman3ext=3pm -Duseperlio -Dinstallusrbinperl=n -Ubincompat5005 -Uversiononly -Dpager=/usr/bin/less -isr -Dd_gethostent_r_proto -Ud_endhostent_r_proto -Ud_sethostent_r_proto -Ud_endprotoent_r_proto -Ud_setprotoent_r_proto -Ud_endservent_r_proto -Ud_setservent_r_proto -Dscriptdir=/usr/bin -Dusesitecustomize'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.7.2 20121109 (Red Hat 4.7.2-8)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -fstack-protector'
    libpth=/usr/local/lib64 /lib64 /usr/lib64
    libs=-lresolv -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc -lgdbm_compat
    perllibs=-lresolv -lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.16'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,--enable-new-dtags -Wl,-rpath,/usr/lib64/perl5/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wl,-z,relro '

Locally applied patches:
    


@INC for perl 5.16.3:
    /home/eda/lib/perl5/
    /usr/local/lib64/perl5
    /usr/local/share/perl5
    /usr/lib64/perl5/vendor_perl
    /usr/share/perl5/vendor_perl
    /usr/lib64/perl5
    /usr/share/perl5
    .


Environment for perl 5.16.3:
    HOME=/home/tradingsystems
    LANG=en_GB.UTF-8
    LANGUAGE (unset)
    LC_COLLATE=C
    LC_CTYPE=en_GB.UTF-8
    LC_MESSAGES=en_GB.UTF-8
    LC_MONETARY=en_GB.UTF-8
    LC_NUMERIC=en_GB.UTF-8
    LC_TIME=en_GB.UTF-8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/eda/bin:/home/eda/bin:/usr/local/bin:/usr/bin:/sbin:/usr/sbin:/sbin:/usr/sbin
    PERL5LIB=/home/eda/lib/perl5/
    PERL_BADLANG (unset)
    SHELL=/bin/bash


______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 30, 2013

From victor@vsespb.ru

On Fri Aug 30 10​:48​:16 2013, eda@​waniasset.com wrote​:

hang chewing CPU​:

and RAM, at least under 5.19.3

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 30, 2013

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 30, 2013

From @wolfsage

On Fri, Aug 30, 2013 at 1​:48 PM, Ed Avis <perlbug-followup@​perl.org> wrote​:

This causes perl to hang chewing CPU​:

my $x = \\42;
open my $fh\, "\<"\, \\$x;
my $got = \<$fh>;

This seems to be in sv.c, in Perl_sv_gets, in the loop starting here, at
least for me​:

8097 for (;;) {
8098 screamer​:
8099 if (cnt > 0) {
8100 if (rslen) {
8101 while (cnt > 0) { /* this |
eat */
8102 cnt--;
8103 if ((*bp++ = *ptr++) == rslast) /* really |
dust */
8104 goto thats_all_folks; /* screams |
sed :-) */
8105 }
8106 }
8107 else {
8108 Copy(ptr, bp, cnt, char); /* this |
eat */
8109 bp += cnt; /* screams |
dust */
8110 ptr += cnt; /* louder |
sed :-) */
8111 cnt = 0;
8112 assert (!shortbuffered);
8113 goto cannot_be_shortbuffered;
8114 }
8115 }
8116

I haven't chased it any further. I didn't see it on perl-5.10.1, but I do
see it on perl-5.14.2.

-- Matthew Horsfall (alh)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 21, 2013

From @cpansprout

On Fri Aug 30 12​:46​:31 2013, alh wrote​:

On Fri, Aug 30, 2013 at 1​:48 PM, Ed Avis <perlbug-followup@​perl.org>
wrote​:

This causes perl to hang chewing CPU​:

my $x = \\42;
open my $fh\, "\<"\, \\$x;
my $got = \<$fh>;

This seems to be in sv.c, in Perl_sv_gets, in the loop starting here, at
least for me​:

8097 for (;;) {
8098 screamer​:
8099 if (cnt > 0) {
8100 if (rslen) {
8101 while (cnt > 0) { /* this |
eat */
8102 cnt--;
8103 if ((*bp++ = *ptr++) == rslast) /* really |
dust */
8104 goto thats_all_folks; /* screams |
sed :-) */
8105 }
8106 }
8107 else {
8108 Copy(ptr, bp, cnt, char); /* this |
eat */
8109 bp += cnt; /* screams |
dust */
8110 ptr += cnt; /* louder |
sed :-) */
8111 cnt = 0;
8112 assert (!shortbuffered);
8113 goto cannot_be_shortbuffered;
8114 }
8115 }
8116

I haven't chased it any further. I didn't see it on perl-5.10.1, but I do
see it on perl-5.14.2.

-- Matthew Horsfall (alh)

$ ../perl.git/Porting/bisect.pl --start=v5.10.0 --end=v5.12.4 -e 'alarm
1; my $x = \42; open my $fh, "<", \$x; my $got = <$fh>;'
...
30c16bb is the first bad commit
commit 30c16bb
Author​: David Mitchell <davem@​iabyn.com>
Date​: Wed May 5 22​:39​:24 2010 +0100

  RT 43789​: "in memory" files don't call STORE
 
  The code in PerlIO-scalar that implements the open $fh, '>' \$buffer
  feature did not, apart from accidentally, support get/set magic and thus
  tied buffers. This patch remedies that​: mostly by just blindly
sprinkling
  SvGETMAGIC/SvSETMAGIC about, rather than doing any deep analysis and
  understanding of the code. One main change I did was to add a
  PerlIOScalar_read() function, rather than rely on the default behaviour
  (which implements it in terms of PerlIOScalar_get_ptr() etc), since that
  approach had a tendency to call FETCH multiple times

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 23, 2013

From @epa

In general I suggest that opening a filehandle giving a reference to a reference doesn't make any sense, and surely didn't do anything useful even before the change identified. It would be better to make it throw an exception.

Similarly I suggest that open $fh, '<', \undef should also throw.

______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http​://www.symanteccloud.com
______________________________________________________________________

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 17, 2013

From @iabyn

On Mon, Sep 23, 2013 at 06​:59​:09AM +0000, Ed Avis wrote​:

In general I suggest that opening a filehandle giving a reference to a
reference doesn't make any sense, and surely didn't do anything useful
even before the change identified. It would be better to make it throw
an exception.

The other approach is to treat it similarly to how other non-string vars
are treated when used as strings, e.g. $x = \1; $x .= 2, which sets
$x to "SCALAR(0x11fd5b0)2".

Similarly I suggest that open $fh, '<', \undef should also throw.

The test suite for PerlIO​::scalar already explicitly expects undef
to behave like an empty string *without* generating any warnings.

I've pushed the following, which fixes the original bug report by going
down the stringifying route.

commit 552908b
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Thu Oct 17 15​:35​:14 2013 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Thu Oct 17 15​:46​:05 2013 +0100

  PerlIO​::scalar​: stringify refs
 
  If $s in
  open my $fh, "<", \$s
  or similar is a ref, then
  stringify that ref​: i.e. convert it from a ref into the string
  "SCALAR(0x....)" or whatever.
 
  This fixes
 
  RT #119529 Filehandle opened from ref to ref hangs on reading
 
  which in this case was looping forever, since it kept thinking that
  the var was a string ("SCALAR.."), but whose length was 0.
 
  I haven't gone for a complete "always force var into a string" approach,
  since PerlIO​::scalar has quite a tolerance for "bad" vars; e.g.
  it won't warn if $var is undef or a read-only constant number etc;
  and it already normalises under some circumstances and not others.
  So I've just increased the cases somewhat where it normalises.
 
  Also, I didn't look to closely at the code that was looping (or to put it
  another way, I looked at it but didn't understand it), so it could
  conceivably still behave badly on some other strange type of variable that
  manages to avoid getting normalised.

--
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 17, 2013

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

@p5pRT p5pRT closed this Oct 17, 2013
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 17, 2013

From @epa

Thanks for fixing this. If silently treating undef as the empty string really is the desired behaviour,
it would be worth documenting that. (IMHO a warning would be more useful, but I guess
whoever wrote the PerlIO​::scalar test suite already weighed up the pros and cons of that behaviour.)

--
Ed Avis <eda@​waniasset.com>

______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http​://www.symanteccloud.com
______________________________________________________________________

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 25, 2013

From @cpansprout

On Thu Oct 17 07​:58​:17 2013, davem wrote​:

I've pushed the following, which fixes the original bug report by
going
down the stringifying route.

commit 552908b
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Thu Oct 17 15​:35​:14 2013 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Thu Oct 17 15​:46​:05 2013 +0100

...which unfortunately breaks when reading a read-only ref in "<" mode. I pushed a better fix as 5a2bc23.

I actually wrote something similar to that a month ago, but then sat on it as real life got in the way.

The amazing thing was that both Dave Mitchell and I used \42 it our tests. Great minds think alike!

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 25, 2013

From @iabyn

On Fri, Oct 25, 2013 at 02​:23​:10PM -0700, Father Chrysostomos via RT wrote​:

On Thu Oct 17 07​:58​:17 2013, davem wrote​:

I've pushed the following, which fixes the original bug report by
going
down the stringifying route.

commit 552908b
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Thu Oct 17 15​:35​:14 2013 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Thu Oct 17 15​:46​:05 2013 +0100

...which unfortunately breaks when reading a read-only ref in "<" mode. I pushed a better fix as 5a2bc23.

Ok thanks.

The amazing thing was that both Dave Mitchell and I used \42 it our
tests. Great minds think alike!

Er, actually the \42 was in the sample code in the original ticket!

--
That he said that that that that is is is debatable, is debatable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.