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

Term::ReadLine generates empty &STDERR files #16134

Closed
p5pRT opened this issue Aug 31, 2017 · 23 comments
Closed

Term::ReadLine generates empty &STDERR files #16134

p5pRT opened this issue Aug 31, 2017 · 23 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 31, 2017

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

Searchable as RT132008$

@p5pRT
Copy link
Author

p5pRT commented Aug 31, 2017

From @toddr

Created by @toddr

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 1ae6ead

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

Perl Info

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

@p5pRT
Copy link
Author

p5pRT commented Sep 1, 2017

From @jkeenan

On Thu, 31 Aug 2017 21​:55​:28 GMT, TODDR wrote​:

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 1ae6ead

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)

@p5pRT
Copy link
Author

p5pRT commented Sep 1, 2017

From @jkeenan

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

@p5pRT
Copy link
Author

p5pRT commented Sep 1, 2017

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

@p5pRT
Copy link
Author

p5pRT commented Sep 1, 2017

From blgl@stacken.kth.se

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?

/Bo Lindbergh

@p5pRT
Copy link
Author

p5pRT commented Sep 1, 2017

From @jkeenan

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

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2017

From @atoomic

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.

99333cd...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​:

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

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2017

From @atoomic

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

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2017

From @atoomic

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

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2017

From @atoomic

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​:

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

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2017

From @jkeenan

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

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2017

From @atoomic

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​:

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

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2017

From @atoomic

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

@p5pRT
Copy link
Author

p5pRT commented Sep 18, 2017

From @tonycoz

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 d8b6190, which a spelling error fix and adding the new test to MANIFEST.

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 18, 2017

@tonycoz - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented Sep 18, 2017

From @toddr

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 d8b6190, 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

@p5pRT
Copy link
Author

p5pRT commented Sep 18, 2017

From @tonycoz

On Mon, Sep 18, 2017 at 07​:56​:49AM -0500, Todd E Rinaldo wrote​:

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 d8b6190, 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

@p5pRT
Copy link
Author

p5pRT commented Sep 19, 2017

From @jkeenan

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 d8b6190, 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)

@p5pRT
Copy link
Author

p5pRT commented Sep 19, 2017

@jkeenan - Status changed from 'pending release' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Sep 19, 2017

From @tonycoz

On Mon, 18 Sep 2017 19​:15​:13 -0700, jkeenan wrote​:

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 d8b6190, 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 1d217c6

The problem was the smokers run the tests without a tty, and findConsole() doesn't return "/dev/tty" in that case.

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 19, 2017

@tonycoz - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2018

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release yesterday of Perl 5.28.0, this and 185 other issues have been
resolved.

Perl 5.28.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.28.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2018

@khwilliamson - Status changed from 'pending release' 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