Skip Menu |
Report information
Id: 132008
Status: pending release
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: TODDR <toddr [at] cpan.org>
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: medium
Type: library
Perl Version: 5.26.0
Fixed In: (no value)



Date: Thu, 31 Aug 2017 16:54:29 -0500
To: perlbug [...] perl.org
From: Todd Rinaldo <toddr [...] cpan.org>
Subject: Term::ReadLine generates empty &STDERR files
Download (untitled) / with headers
text/plain 6.6k
This is a bug report for perl from toddr@cpan.org, generated with the help of perlbug 1.40 running under perl 5.26.0. ----------------------------------------------------------------- [Please describe your issue here] We just upgraded to 5.26.0 and noticed that an empty &STDERR file was getting created in one of our directories. The root cause was hard to track down because it only happened if /dev/tty was inaccessible. To re-produce this (probably only on POSIX systems), you can do: perl526 -MPOSIX -MTerm::ReadLine -E'exit if(fork); POSIX::setsid(); exit if(fork); my $t = Term::ReadLine->new("whatever")'; then ls -ld \&STDERR This problem bisects back to a conversion of a 2 arg open to a 3 arg open in 1ae6ead94905dfee43773cf3b18949c91b33f9d1 The problem is that redirecting file handles to STDERR (open (my $fh, ">&STDERR)) cannot be done as a 3 arg open or it'll actually try to write to that file. I suggest reverting the fix (Only for the one file dist/Term-ReadLine/lib/Term/ReadLine.pm) and also propose this be a maintenance fix in 5.26.1 Todd [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=library severity=medium module=Term::ReadLine --- Site configuration information for perl 5.26.0: Configured by cPanel at Tue Aug 22 13:29:24 CDT 2017. Summary of my perl5 (revision 5 version 26 subversion 0) configuration: Platform: osname=linux osvers=3.10.0-123.20.1.el7.x86_64 archname=x86_64-linux-64int uname='linux rpmbuild-64-centos-7.dev.cpanel.net 3.10.0-123.20.1.el7.x86_64 #1 smp thu jan 29 18:05:33 utc 2015 x86_64 x86_64 x86_64 gnulinux ' config_args='-des -Dusedevel -Darchname=x86_64-linux-64int -Dcc=/usr/bin/gcc -Dcpp=/usr/bin/cpp -Dusemymalloc=n -DDEBUGGING=none -Doptimize=-Os -Accflags=-m64 -Dccflags=-DPERL_DISABLE_PMC -fPIC -DPIC -I/usr/local/cpanel/3rdparty/perl/526/include -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -Duseshrplib -Duselargefiles=yes -Duseposix=true -Dhint=recommended -Duseperlio=yes -Dcppflags=-I/usr/local/cpanel/3rdparty/perl/526/include -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -Dldflags=-L/usr/local/cpanel/3rdparty/lib64 -Dprefix=/usr/local/cpanel/3rdparty/perl/526 -Dsiteprefix=/opt/cpanel/perl5/526 -Dsitebin=/opt/cpanel/perl5/526/bin -Dsitelib=/opt/cpanel/perl5/526/site_lib -Dusevendorprefix=true -Dvendorbin=/usr/local/cpanel/3rdparty/perl/526/bin -Dvendorprefix=/usr/local/cpanel/3rdparty/perl/526/lib64/perl5 -Dvendorlib=/usr/local/cpanel/3rdparty/perl/526/lib64/perl5/cpanel_lib -Dprivlib=/usr/local/cpanel/3rdparty/perl/526/lib64/perl5/5.26.0 -Dman1dir=none -Dman3dir=none -Dscriptdir=/usr/local/cpanel/3rdparty/perl/526/bin -Dscriptdirexp=/usr/local/cpanel/3rdparty/perl/526/bin -Dsiteman1dir=none -Dsiteman3dir=none -Dinstallman1dir=none -Dversiononly=no -Dinstallusrbinperl=no -Dcf_by=cPanel -Dmyhostname=localhost -Dperladmin=root@localhost -Dcf_email=support@cpanel.net -Di_dbm=/usr/local/cpanel/3rdparty/include -Di_gdbm=/usr/local/cpanel/3rdparty/include -Di_ndbm=/usr/local/cpanel/3rdparty/include -DDB_File=true -Ud_dosuid -Uuserelocatableinc -Umad -Uusethreads -Uusemultiplicity -Uusesocks -Uuselongdouble -Aldflags=-L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -lgdbm -Dlocincpth=/usr/local/cpanel/3rdparty/perl/526/include /usr/local/cpanel/3rdparty/include /usr/local/include -Duse64bitint -Uuse64bitall -Dlibpth=/usr/local/cpanel/3rdparty/lib64 /usr/local/lib64 /usr/local/lib /lib64 /usr/lib64 ' hint=recommended useposix=true d_sigaction=define useithreads=undef usemultiplicity=undef use64bitint=define use64bitall=undef uselongdouble=undef usemymalloc=n default_inc_excludes_dot=define bincompat5005=undef Compiler: cc='/usr/bin/gcc' ccflags ='-DPERL_DISABLE_PMC -fPIC -DPIC -I/usr/local/cpanel/3rdparty/perl/526/include -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -m64 -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2' optimize='-Os' cppflags='-I/usr/local/cpanel/3rdparty/perl/526/include -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -DPERL_DISABLE_PMC -fPIC -DPIC -I/usr/local/cpanel/3rdparty/perl/526/include -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -m64 -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='' gccversion='4.8.2 20140120 (Red Hat 4.8.2-16)' gccosandvers='' intsize=4 longsize=8 ptrsize=8 doublesize=8 byteorder=12345678 doublekind=3 d_longlong=define longlongsize=8 d_longdbl=define longdblsize=16 longdblkind=3 ivtype='long' ivsize=8 nvtype='double' nvsize=8 Off_t='off_t' lseeksize=8 alignbytes=8 prototype=define Linker and Libraries: ld='/usr/bin/gcc' ldflags ='-L/usr/local/cpanel/3rdparty/lib64 -L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -lgdbm -fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/cpanel/3rdparty/lib64 /usr/local/lib64 /usr/local/lib /lib64 /usr/lib64 /usr/local/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.17.so so=so useshrplib=true libperl=libperl.so gnulibc_version='2.17' Dynamic Linking: dlsrc=dl_dlopen.xs dlext=so d_dlsymun=undef ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/cpanel/3rdparty/perl/526/lib64/perl5/5.26.0/x86_64-linux-64int/CORE' cccdlflags='-fPIC' lddlflags='-shared -Os -L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -L/usr/local/lib -fstack-protector-strong' Locally applied patches: cPanel patches cPanel INC path changes cPanel performance improvements to modules cPanel Immortal COW cPanel B and O performance fixups cPanel B::C Declare Static Memory malloc patches cPanel Disable XS handshake --- @INC for perl 5.26.0: /usr/local/cpanel /usr/local/cpanel/3rdparty/perl/526/lib64/perl5/cpanel_lib/x86_64-linux-64int /usr/local/cpanel/3rdparty/perl/526/lib64/perl5/cpanel_lib /usr/local/cpanel/3rdparty/perl/526/lib64/perl5/5.26.0/x86_64-linux-64int /usr/local/cpanel/3rdparty/perl/526/lib64/perl5/5.26.0 /opt/cpanel/perl5/526/site_lib/x86_64-linux-64int /opt/cpanel/perl5/526/site_lib --- Environment for perl 5.26.0: HOME=/root LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin PERL_BADLANG (unset) SHELL=/bin/zsh
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.4k
On Thu, 31 Aug 2017 21:55:28 GMT, TODDR wrote: Show quoted text
> This is a bug report for perl from toddr@cpan.org, > generated with the help of perlbug 1.40 running under perl 5.26.0. > > > ----------------------------------------------------------------- > [Please describe your issue here] > > We just upgraded to 5.26.0 and noticed that an empty &STDERR file was > getting created in one of our directories. The root cause was hard to > track down because it only happened if /dev/tty was inaccessible. To > re-produce this (probably only on POSIX systems), you can do: > > perl526 -MPOSIX -MTerm::ReadLine -E'exit if(fork); POSIX::setsid(); > exit if(fork); my $t = Term::ReadLine->new("whatever")'; > > then > > ls -ld \&STDERR > > This problem bisects back to a conversion of a 2 arg open to a 3 arg > open in 1ae6ead94905dfee43773cf3b18949c91b33f9d1 > > The problem is that redirecting file handles to STDERR (open (my $fh, > ">&STDERR)) cannot be done as a 3 arg open or it'll actually try to > write to that file. > > I suggest reverting the fix (Only for the one file > dist/Term-ReadLine/lib/Term/ReadLine.pm) and also propose this be a > maintenance fix in 5.26.1 > > Todd >
Patch attached; also available in the smoke-me/jkeenan/132008-term-readline branch. But I don't consider it ready to apply to blead as it lacks a test. Todd: Can you add a test for the non-creation of '&STDERR' that fails in blead (or 5.26) but passes once patch is applied? Thank you very much. -- James E Keenan (jkeenan@cpan.org)
Subject: 132008-0001-Revert-to-2-arg-open-in-one-case.patch
From e4dc68d725b19f46c6fca9423e6e7a0eaeff47f4 Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Thu, 31 Aug 2017 22:57:06 -0400 Subject: [PATCH] Revert to 2-arg open in one case. If /dev/tty is inaccessible, redirecting file handles to STDERR: open (my $fh, ">&STDERR)) ... cannot be done as a 3 arg open or it'll actually try to write to that file. Bump $Term::ReadLine::VERSION. For: RT #132008 --- dist/Term-ReadLine/lib/Term/ReadLine.pm | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dist/Term-ReadLine/lib/Term/ReadLine.pm b/dist/Term-ReadLine/lib/Term/ReadLine.pm index 88d5a75..e0f4e1b 100644 --- a/dist/Term-ReadLine/lib/Term/ReadLine.pm +++ b/dist/Term-ReadLine/lib/Term/ReadLine.pm @@ -267,10 +267,13 @@ sub new { my($console, $consoleOUT) = $_[0]->findConsole; + + # the Windows CONIN$ needs GENERIC_WRITE mode to allow # a SetConsoleMode() if we end up using Term::ReadKey open FIN, (( $^O eq 'MSWin32' && $console eq 'CONIN$' ) ? '+<' : '<' ), $console; - open FOUT,'>', $consoleOUT; + # RT #132008: Still need 2-arg open here + open FOUT,">$consoleOUT"; #OUT->autoflush(1); # Conflicts with debugger? my $sel = select(FOUT); @@ -319,7 +322,7 @@ sub Features { \%features } package Term::ReadLine; # So late to allow the above code be defined? -our $VERSION = '1.16'; +our $VERSION = '1.17'; my ($which) = exists $ENV{PERL_RL} ? split /\s+/, $ENV{PERL_RL} : undef; if ($which) { -- 2.7.4
From: Bo Lindbergh <blgl [...] stacken.kth.se>
Subject: Re: [perl #132008] Term::ReadLine generates empty &STDERR files
To: perl5-porters [...] perl.org
Date: Fri, 1 Sep 2017 05:13:27 +0200
Download (untitled) / with headers
text/plain 439b
Quoth Todd Rinaldo: Show quoted text
> The problem is that redirecting file handles to STDERR (open (my $fh, > ">&STDERR)) cannot be done as a 3 arg open or it'll actually try to > write to that file.
Yes, it can! open(my $fh, ">&", \*STDERR) Getting Term::ReadLine to use this may involve breaking compatibility, since the findConsole method assumes 2-arg open. Is findConsole ever called from outside the Term::ReadLine::* space? /Bo Lindbergh
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 829b
On Fri, 01 Sep 2017 03:13:59 GMT, blgl@stacken.kth.se wrote: Show quoted text
> Quoth Todd Rinaldo:
> > The problem is that redirecting file handles to STDERR (open (my $fh, > > ">&STDERR)) cannot be done as a 3 arg open or it'll actually try to > > write to that file.
> > Yes, it can! > > open(my $fh, ">&", \*STDERR) > > Getting Term::ReadLine to use this may involve breaking compatibility, > since the findConsole method assumes 2-arg open. Is findConsole ever > called from outside the Term::ReadLine::* space? > >
findConsole is clearly documented, so we should assume that it's callable from outside that space. ##### "findConsole" returns an array with two strings that give most appropriate names for files for input and output using conventions "<$in", ">out". ##### Show quoted text
> /Bo Lindbergh
-- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.6k
You would find attached to this message the extra unit test (as patch #1) to show the failure before/after the fix from James. Note that the unit test required some minor refactor of Term::ReadLine to mock /dev/tty. I've pushed to my github branch the test + James commit on top of it to fix the issue, unfortunately I've not tested this on windows, so test are disabled for now on windows as the behavior is different. https://github.com/Perl/perl5/compare/99333cdb13969ed9b8675abca40b02543bd3c4bc...atoomic:devel/rt-132008 Note: looks like the upstream git repo from the cpan distribution points to https://github.com/rafl/term-readline which is outdated, not sure if there is a more recent git repo for upstream where we can submit these changes, and update the META informations On Thu, 31 Aug 2017 20:24:58 -0700, jkeenan wrote: Show quoted text
> On Fri, 01 Sep 2017 03:13:59 GMT, blgl@stacken.kth.se wrote:
> > Quoth Todd Rinaldo:
> > > The problem is that redirecting file handles to STDERR (open (my > > > $fh, > > > ">&STDERR)) cannot be done as a 3 arg open or it'll actually try to > > > write to that file.
> > > > Yes, it can! > > > > open(my $fh, ">&", \*STDERR) > > > > Getting Term::ReadLine to use this may involve breaking > > compatibility, > > since the findConsole method assumes 2-arg open. Is findConsole ever > > called from outside the Term::ReadLine::* space? > > > >
> > findConsole is clearly documented, so we should assume that it's > callable from outside that space. > > ##### > "findConsole" > returns an array with two strings that give most appropriate names for > files for input and output using conventions "<$in", ">out". > ##### >
> > /Bo Lindbergh
Subject: 0001-Add-unit-test-for-RT-132008.patch
From 05a3ecccf5b448be98bfd7befe474011db50db77 Mon Sep 17 00:00:00 2001 From: Nicolas R <atoomic@cpan.org> Date: Thu, 7 Sep 2017 11:12:26 -0600 Subject: [PATCH 1/2] Add unit test for RT #132008 Term::ReadLine generates empty &STDERR files --- dist/Term-ReadLine/lib/Term/ReadLine.pm | 12 ++++++---- dist/Term-ReadLine/t/ReadLine-STDERR.t | 41 +++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 dist/Term-ReadLine/t/ReadLine-STDERR.t diff --git a/dist/Term-ReadLine/lib/Term/ReadLine.pm b/dist/Term-ReadLine/lib/Term/ReadLine.pm index 88d5a75877..87fdfb86c4 100644 --- a/dist/Term-ReadLine/lib/Term/ReadLine.pm +++ b/dist/Term-ReadLine/lib/Term/ReadLine.pm @@ -229,12 +229,17 @@ sub readline { } sub addhistory {} +# used for testing purpose +sub devtty { return '/dev/tty' } + sub findConsole { my $console; my $consoleOUT; - if ($^O ne 'MSWin32' and -e "/dev/tty") { - $console = "/dev/tty"; + my $devtty = devtty(); + + if ($^O ne 'MSWin32' and -e $devtty) { + $console = $devtty; } elsif ($^O eq 'MSWin32' or $^O eq 'msys' or -e "con") { $console = 'CONIN$'; $consoleOUT = 'CONOUT$'; @@ -248,7 +253,7 @@ sub findConsole { $consoleOUT = $console unless defined $consoleOUT; $console = "&STDIN" unless defined $console; - if ($console eq "/dev/tty" && !open(my $fh, "<", $console)) { + if ($console eq $devtty && !open(my $fh, "<", $console)) { $console = "&STDIN"; undef($consoleOUT); } @@ -266,7 +271,6 @@ sub new { if (@_==2) { my($console, $consoleOUT) = $_[0]->findConsole; - # the Windows CONIN$ needs GENERIC_WRITE mode to allow # a SetConsoleMode() if we end up using Term::ReadKey open FIN, (( $^O eq 'MSWin32' && $console eq 'CONIN$' ) ? '+<' : '<' ), $console; diff --git a/dist/Term-ReadLine/t/ReadLine-STDERR.t b/dist/Term-ReadLine/t/ReadLine-STDERR.t new file mode 100644 index 0000000000..621530226e --- /dev/null +++ b/dist/Term-ReadLine/t/ReadLine-STDERR.t @@ -0,0 +1,41 @@ +#!./perl -w +use strict; + +use Test::More; + +## unit test for RT 132008 - https://rt.perl.org/Ticket/Display.html?id=132008 + +if ( $^O eq 'MSWin32' || !-e q{/dev/tty} ) { + plan skip_all => "Test not tested on windows or when /dev/tty do not exists"; +} +else { + plan tests => 9; +} + +if ( -e q[&STDERR] ) { + note q[Removing existing file &STDERR]; + unlink q[&STDERR] or die q{Cannot remove existign file &STDERR [probably created from a previous run]}; +} + +use_ok('Term::ReadLine'); +can_ok( 'Term::ReadLine::Stub', qw{new devtty findConsole} ); + +is( Term::ReadLine->devtty(), q{/dev/tty} ); +my @out = Term::ReadLine::Stub::findConsole(); +is_deeply \@out, [ q{/dev/tty}, q{/dev/tty} ], "findConsole is using /dev/tty"; + +{ + no warnings 'redefine'; + my $donotexist = q[/this/should/not/exist/hopefully]; + + ok !-e $donotexist, "File $donotexist does not exist"; + local *Term::ReadLine::Stub::devtty = sub { $donotexist }; + is( Term::ReadLine->devtty(), $donotexist, "devtty mocked" ); + + my @out = Term::ReadLine::Stub::findConsole(); + is_deeply \@out, [ q{&STDIN}, q{&STDERR} ], "findConsole is using /dev/tty" or diag explain \@out; + + ok !-e q[&STDERR], 'file &STDERR do not exist before Term::ReadLine call'; + my $tr = Term::ReadLine->new('whatever'); + ok !-e q[&STDERR], 'file &STDERR was not created by mistake'; +} -- 2.14.1
Subject: 0002-Revert-to-2-arg-open-in-one-case.patch
From 0ce978ecf961173f55b95ab5e78d06b5e8a3e1f6 Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Thu, 31 Aug 2017 22:57:06 -0400 Subject: [PATCH 2/2] Revert to 2-arg open in one case. If /dev/tty is inaccessible, redirecting file handles to STDERR: open (my $fh, ">&STDERR)) ... cannot be done as a 3 arg open or it'll actually try to write to that file. Bump $Term::ReadLine::VERSION. For: RT #132008 (cherry picked from commit e4dc68d725b19f46c6fca9423e6e7a0eaeff47f4) Signed-off-by: Nicolas R <atoomic@cpan.org> --- dist/Term-ReadLine/lib/Term/ReadLine.pm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dist/Term-ReadLine/lib/Term/ReadLine.pm b/dist/Term-ReadLine/lib/Term/ReadLine.pm index 87fdfb86c4..e00fb376cd 100644 --- a/dist/Term-ReadLine/lib/Term/ReadLine.pm +++ b/dist/Term-ReadLine/lib/Term/ReadLine.pm @@ -274,7 +274,8 @@ sub new { # the Windows CONIN$ needs GENERIC_WRITE mode to allow # a SetConsoleMode() if we end up using Term::ReadKey open FIN, (( $^O eq 'MSWin32' && $console eq 'CONIN$' ) ? '+<' : '<' ), $console; - open FOUT,'>', $consoleOUT; + # RT #132008: Still need 2-arg open here + open FOUT,">$consoleOUT"; #OUT->autoflush(1); # Conflicts with debugger? my $sel = select(FOUT); @@ -323,7 +324,7 @@ sub Features { \%features } package Term::ReadLine; # So late to allow the above code be defined? -our $VERSION = '1.16'; +our $VERSION = '1.17'; my ($which) = exists $ENV{PERL_RL} ? split /\s+/, $ENV{PERL_RL} : undef; if ($which) { -- 2.14.1
RT-Send-CC: perl5-porters [...] perl.org
Just pinging the case, as everything seem ready to be reviewed/merged, would appreciate if someone with merge permissions could have a look at it thanks On Thu, 31 Aug 2017 20:24:58 -0700, jkeenan wrote: Show quoted text
> On Fri, 01 Sep 2017 03:13:59 GMT, blgl@stacken.kth.se wrote:
> > Quoth Todd Rinaldo:
> > > The problem is that redirecting file handles to STDERR (open (my > > > $fh, > > > ">&STDERR)) cannot be done as a 3 arg open or it'll actually try to > > > write to that file.
> > > > Yes, it can! > > > > open(my $fh, ">&", \*STDERR) > > > > Getting Term::ReadLine to use this may involve breaking > > compatibility, > > since the findConsole method assumes 2-arg open. Is findConsole ever > > called from outside the Term::ReadLine::* space? > > > >
> > findConsole is clearly documented, so we should assume that it's > callable from outside that space. > > ##### > "findConsole" > returns an array with two strings that give most appropriate names for > files for input and output using conventions "<$in", ">out". > ##### >
> > /Bo Lindbergh
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 677b
On Wed, 13 Sep 2017 15:35:39 GMT, atoomic wrote: Show quoted text
> Just pinging the case, as everything seem ready to be reviewed/merged, > would appreciate if someone with merge permissions could have a look at it > > thanks >
atoomic, to be honest I'm not sure that this ticket and the patches so far proposed have been thoroughly reviewed yet. In particular: (a) I'm not sure that Bo Lindbergh's comment of Sep 01 has been addressed. (b) I wonder if there's any way of dealing with the original problem other than the special-case reversion implemented in my Aug 31 patch. Can we get additional eyeballs on this? Thank you very much. Jim Keenan -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 960b
After our recent discussion, it seems all the issues have been addressed, here is a consolidated patch with the revert to 2-arg open and unit test. Can someone else have a look at it & merge/close at this case. thanks On Wed, 13 Sep 2017 09:40:35 -0700, jkeenan wrote: Show quoted text
> On Wed, 13 Sep 2017 15:35:39 GMT, atoomic wrote:
> > Just pinging the case, as everything seem ready to be > > reviewed/merged, > > would appreciate if someone with merge permissions could have a look > > at it > > > > thanks > >
> > atoomic, to be honest I'm not sure that this ticket and the patches so > far proposed have been thoroughly reviewed yet. In particular: > > (a) I'm not sure that Bo Lindbergh's comment of Sep 01 has been > addressed. > > (b) I wonder if there's any way of dealing with the original problem > other than the special-case reversion implemented in my Aug 31 patch. > > Can we get additional eyeballs on this? > > Thank you very much. > Jim Keenan
Subject: 0001-Term-ReadLine-generates-empty-STDERR-files.patch
From d105368121006ef87ec74235dd033b79d0dbd519 Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Thu, 31 Aug 2017 22:57:06 -0400 Subject: [PATCH] Term::ReadLine generates empty &STDERR files Revert to 2-arg open in one case. If /dev/tty is inaccessible, redirecting file handles to STDERR: open (my $fh, ">&STDERR)) ... cannot be done as a 3 arg open or it'll actually try to write to that file. Bump $Term::ReadLine::VERSION. Add unit test for RT #132008 For: RT #132008 (cherry picked from commit e4dc68d725b19f46c6fca9423e6e7a0eaeff47f4) Signed-off-by: Nicolas R <atoomic@cpan.org> --- dist/Term-ReadLine/lib/Term/ReadLine.pm | 17 +++++++++----- dist/Term-ReadLine/t/ReadLine-STDERR.t | 41 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 dist/Term-ReadLine/t/ReadLine-STDERR.t diff --git a/dist/Term-ReadLine/lib/Term/ReadLine.pm b/dist/Term-ReadLine/lib/Term/ReadLine.pm index 88d5a75877..e00fb376cd 100644 --- a/dist/Term-ReadLine/lib/Term/ReadLine.pm +++ b/dist/Term-ReadLine/lib/Term/ReadLine.pm @@ -229,12 +229,17 @@ sub readline { } sub addhistory {} +# used for testing purpose +sub devtty { return '/dev/tty' } + sub findConsole { my $console; my $consoleOUT; - if ($^O ne 'MSWin32' and -e "/dev/tty") { - $console = "/dev/tty"; + my $devtty = devtty(); + + if ($^O ne 'MSWin32' and -e $devtty) { + $console = $devtty; } elsif ($^O eq 'MSWin32' or $^O eq 'msys' or -e "con") { $console = 'CONIN$'; $consoleOUT = 'CONOUT$'; @@ -248,7 +253,7 @@ sub findConsole { $consoleOUT = $console unless defined $consoleOUT; $console = "&STDIN" unless defined $console; - if ($console eq "/dev/tty" && !open(my $fh, "<", $console)) { + if ($console eq $devtty && !open(my $fh, "<", $console)) { $console = "&STDIN"; undef($consoleOUT); } @@ -266,11 +271,11 @@ sub new { if (@_==2) { my($console, $consoleOUT) = $_[0]->findConsole; - # the Windows CONIN$ needs GENERIC_WRITE mode to allow # a SetConsoleMode() if we end up using Term::ReadKey open FIN, (( $^O eq 'MSWin32' && $console eq 'CONIN$' ) ? '+<' : '<' ), $console; - open FOUT,'>', $consoleOUT; + # RT #132008: Still need 2-arg open here + open FOUT,">$consoleOUT"; #OUT->autoflush(1); # Conflicts with debugger? my $sel = select(FOUT); @@ -319,7 +324,7 @@ sub Features { \%features } package Term::ReadLine; # So late to allow the above code be defined? -our $VERSION = '1.16'; +our $VERSION = '1.17'; my ($which) = exists $ENV{PERL_RL} ? split /\s+/, $ENV{PERL_RL} : undef; if ($which) { diff --git a/dist/Term-ReadLine/t/ReadLine-STDERR.t b/dist/Term-ReadLine/t/ReadLine-STDERR.t new file mode 100644 index 0000000000..621530226e --- /dev/null +++ b/dist/Term-ReadLine/t/ReadLine-STDERR.t @@ -0,0 +1,41 @@ +#!./perl -w +use strict; + +use Test::More; + +## unit test for RT 132008 - https://rt.perl.org/Ticket/Display.html?id=132008 + +if ( $^O eq 'MSWin32' || !-e q{/dev/tty} ) { + plan skip_all => "Test not tested on windows or when /dev/tty do not exists"; +} +else { + plan tests => 9; +} + +if ( -e q[&STDERR] ) { + note q[Removing existing file &STDERR]; + unlink q[&STDERR] or die q{Cannot remove existign file &STDERR [probably created from a previous run]}; +} + +use_ok('Term::ReadLine'); +can_ok( 'Term::ReadLine::Stub', qw{new devtty findConsole} ); + +is( Term::ReadLine->devtty(), q{/dev/tty} ); +my @out = Term::ReadLine::Stub::findConsole(); +is_deeply \@out, [ q{/dev/tty}, q{/dev/tty} ], "findConsole is using /dev/tty"; + +{ + no warnings 'redefine'; + my $donotexist = q[/this/should/not/exist/hopefully]; + + ok !-e $donotexist, "File $donotexist does not exist"; + local *Term::ReadLine::Stub::devtty = sub { $donotexist }; + is( Term::ReadLine->devtty(), $donotexist, "devtty mocked" ); + + my @out = Term::ReadLine::Stub::findConsole(); + is_deeply \@out, [ q{&STDIN}, q{&STDERR} ], "findConsole is using /dev/tty" or diag explain \@out; + + ok !-e q[&STDERR], 'file &STDERR do not exist before Term::ReadLine call'; + my $tr = Term::ReadLine->new('whatever'); + ok !-e q[&STDERR], 'file &STDERR was not created by mistake'; +} -- 2.14.1
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 402b
On Thu, 14 Sep 2017 12:45:54 -0700, atoomic wrote: Show quoted text
> After our recent discussion, it seems all the issues have been addressed, > here is a consolidated patch with the revert to 2-arg open and unit test. > > Can someone else have a look at it & merge/close at this case.
Thanks, applied as d8b61909479178ddb668ad385988877d26f202f2, which a spelling error fix and adding the new test to MANIFEST. Tony
From: Todd E Rinaldo <toddr [...] cpanel.net>
Subject: Re: [perl #132008] Term::ReadLine generates empty &STDERR files
To: perlbug-followup [...] perl.org
CC: toddr [...] cpan.org
Date: Mon, 18 Sep 2017 07:56:49 -0500
Download (untitled) / with headers
text/plain 607b
Show quoted text
> On Sep 18, 2017, at 12:16 AM, Tony Cook via RT <perlbug-followup@perl.org> wrote: >
>> On Thu, 14 Sep 2017 12:45:54 -0700, atoomic wrote: >> After our recent discussion, it seems all the issues have been addressed, >> here is a consolidated patch with the revert to 2-arg open and unit test. >> >> Can someone else have a look at it & merge/close at this case.
> > Thanks, applied as d8b61909479178ddb668ad385988877d26f202f2, which a spelling error fix and adding the new test to MANIFEST. > > Tony
Thanks for the review Tony. What do we need to do to get it on the maintenance backport list? Todd
Download smime.p7s
application/pkcs7-signature 2.3k

Message body not shown because it is not plain text.

Date: Tue, 19 Sep 2017 09:18:22 +1000
Subject: Re: [perl #132008] Term::ReadLine generates empty &STDERR files
From: Tony Cook <tony [...] develop-help.com>
To: Todd E Rinaldo <toddr [...] cpanel.net>
CC: perlbug-followup [...] perl.org, toddr [...] cpan.org
Download (untitled) / with headers
text/plain 787b
On Mon, Sep 18, 2017 at 07:56:49AM -0500, Todd E Rinaldo wrote: Show quoted text
> >
> > On Sep 18, 2017, at 12:16 AM, Tony Cook via RT <perlbug-followup@perl.org> wrote: > >
> >> On Thu, 14 Sep 2017 12:45:54 -0700, atoomic wrote: > >> After our recent discussion, it seems all the issues have been addressed, > >> here is a consolidated patch with the revert to 2-arg open and unit test. > >> > >> Can someone else have a look at it & merge/close at this case.
> > > > Thanks, applied as d8b61909479178ddb668ad385988877d26f202f2, which a spelling error fix and adding the new test to MANIFEST. > > > > Tony
> > Thanks for the review Tony. What do we need to do to get it on the maintenance backport list?
https://perl5.git.perl.org/perl.git/commit/f734222aca96cf491ce1a3fab8d76650a9275cc9 Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
On Mon, 18 Sep 2017 05:16:07 GMT, tonyc wrote: Show quoted text
> On Thu, 14 Sep 2017 12:45:54 -0700, atoomic wrote:
> > After our recent discussion, it seems all the issues have been > > addressed, > > here is a consolidated patch with the revert to 2-arg open and unit > > test. > > > > Can someone else have a look at it & merge/close at this case.
> > Thanks, applied as d8b61909479178ddb668ad385988877d26f202f2, which a > spelling error fix and adding the new test to MANIFEST. > > Tony
This ticket needs to be re-opened because one of the newly added tests is failing on all of tux's Linux smokers. Example: http://perl5.test-smoke.org/report/57994 The failing test is test 4 in newly added file dist/Term-ReadLine/t/ReadLine-STDERR.t. (Note that though 'git log' and 'git blame' attribute this file to me, it was actually contributed by someone else. I didn't write any tests for this ticket and only two lines survive out of what I originally contributed (which was just a reversion). I have been unable to reproduce this test failure locally in either Linux or FreeBSD. There must be some reason why the following test (#4 in the file) fails in tux's smokers' environments but passes elsewhere. ##### my @out = Term::ReadLine::Stub::findConsole(); is_deeply \@out, [ q{/dev/tty}, q{/dev/tty} ], "findConsole is using /dev/tty"; ##### Test (4) is repeated farther down (7) in a mocking context, but there appears to pass on tux's smokers (as it does for me on both OSes mentioned above). Thank you very much. -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.7k
On Mon, 18 Sep 2017 19:15:13 -0700, jkeenan wrote: Show quoted text
> On Mon, 18 Sep 2017 05:16:07 GMT, tonyc wrote:
> > On Thu, 14 Sep 2017 12:45:54 -0700, atoomic wrote:
> > > After our recent discussion, it seems all the issues have been > > > addressed, > > > here is a consolidated patch with the revert to 2-arg open and unit > > > test. > > > > > > Can someone else have a look at it & merge/close at this case.
> > > > Thanks, applied as d8b61909479178ddb668ad385988877d26f202f2, which a > > spelling error fix and adding the new test to MANIFEST. > > > > Tony
> > This ticket needs to be re-opened because one of the newly added tests > is failing on all of tux's Linux smokers. > > Example: http://perl5.test-smoke.org/report/57994 > > The failing test is test 4 in newly added file dist/Term- > ReadLine/t/ReadLine-STDERR.t. (Note that though 'git log' and 'git > blame' attribute this file to me, it was actually contributed by > someone else. I didn't write any tests for this ticket and only two > lines survive out of what I originally contributed (which was just a > reversion). > > I have been unable to reproduce this test failure locally in either > Linux or FreeBSD. > > There must be some reason why the following test (#4 in the file) > fails in tux's smokers' environments but passes elsewhere. > > ##### > my @out = Term::ReadLine::Stub::findConsole(); > is_deeply \@out, [ q{/dev/tty}, q{/dev/tty} ], "findConsole is using > /dev/tty"; > ##### > > Test (4) is repeated farther down (7) in a mocking context, but there > appears to pass on tux's smokers (as it does for me on both OSes > mentioned above).
Fixed in 1d217c696857b2bf41d87a7e927c43d20cc556e5 The problem was the smokers run the tests without a tty, and findConsole() doesn't return "/dev/tty" in that case. Tony


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org